config/database: update queries to improve reads/writes (#19975)

* config/database: update queries to improve reads/writes

* reflect review comments

* move tx initialization

* add comments and fix psql migration

* fix spelling

* refactor

* add more descriptive comment

config/database.go

@@ -6,8 +6,10 @@ package config
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"database/sql"
 	"embed"
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"path/filepath"
@@ -219,48 +221,81 @@ func (ds *DatabaseStore) persist(cfg *model.Config) error {
 		return errors.Wrap(err, "failed to serialize")
 	}
 
-	id := model.NewId()
 	value := string(b)
-	createAt := model.GetMillis()
-
 	err = ds.checkLength(len(value))
 	if err != nil {
 		return errors.Wrap(err, "marshalled configuration failed length check")
 	}
 
+	sum := sha256.Sum256(b)
+
+	// Skip the persist altogether if we're effectively writing the same configuration.
+	var oldValue string
+	var row *sql.Row
+	if ds.driverName == model.DatabaseDriverMysql {
+		// We use a sub-query to get the Id first because selecting the Id column using
+		// active uses the index, but selecting SHA column using active does not use the index.
+		// The sub-query uses the active index, and then the top-level query uses the primary key.
+		// This takes 2 queries, but it is actually faster than one slow query for MySQL
+		row = ds.db.QueryRow("SELECT SHA FROM Configurations WHERE Id = (select Id from Configurations Where Active)")
+	} else {
+		row = ds.db.QueryRow("SELECT SHA FROM Configurations WHERE Active")
+	}
+	if err = row.Scan(&oldValue); err != nil && err != sql.ErrNoRows {
+		return errors.Wrap(err, "failed to query active configuration")
+	}
+
+	// postgres retruns blank-padded therefore we trim the space
+	oldSum, err := hex.DecodeString(strings.TrimSpace(oldValue))
+	if err != nil {
+		return errors.Wrap(err, "could not encode value")
+	}
+
+	// compare checksums, it's more efficient rather than comparing entire config itself
+	if bytes.Equal(oldSum, sum[0:]) {
+		return nil
+	}
+
 	tx, err := ds.db.Beginx()
 	if err != nil {
 		return errors.Wrap(err, "failed to begin transaction")
 	}
 	defer func() {
 		// Rollback after Commit just returns sql.ErrTxDone.
-		if err := tx.Rollback(); err != nil && err != sql.ErrTxDone {
+		if err = tx.Rollback(); err != nil && err != sql.ErrTxDone {
 			mlog.Error("Failed to rollback configuration transaction", mlog.Err(err))
 		}
 	}()
 
+	var oldId string
+	if ds.driverName == model.DatabaseDriverMysql {
+		// the query doesn't use active index if we query for value (mysql, no surprise)
+		// we select Id column which triggers using index hence we do quicker reads
+		// that's the reason we select id first then query against id to get the value.
+		row = tx.QueryRow("SELECT Id FROM Configurations WHERE Active")
+		if err = row.Scan(&oldId); err != nil && err != sql.ErrNoRows {
+			return errors.Wrap(err, "failed to query active configuration")
+		}
+		if oldId != "" {
+			if _, err := tx.NamedExec("UPDATE Configurations SET Active = NULL WHERE Id = :id", map[string]interface{}{"id": oldId}); err != nil {
+				return errors.Wrap(err, "failed to deactivate current configuration")
+			}
+		}
+	} else {
+		if _, err := tx.Exec("UPDATE Configurations SET Active = NULL WHERE Active"); err != nil {
+			return errors.Wrap(err, "failed to deactivate current configuration")
+		}
+	}
+
 	params := map[string]interface{}{
-		"id":        id,
+		"id":        model.NewId(),
 		"value":     value,
-		"create_at": createAt,
+		"create_at": model.GetMillis(),
 		"key":       "ConfigurationId",
+		"sha":       hex.EncodeToString(sum[0:]),
 	}
 
-	// Skip the persist altogether if we're effectively writing the same configuration.
+	if _, err := tx.NamedExec("INSERT INTO Configurations (Id, Value, CreateAt, Active, SHA) VALUES (:id, :value, :create_at, TRUE, :sha)", params); err != nil {
-	var oldValue []byte
-	row := ds.db.QueryRow("SELECT Value FROM Configurations WHERE Active")
-	if err := row.Scan(&oldValue); err != nil && err != sql.ErrNoRows {
-		return errors.Wrap(err, "failed to query active configuration")
-	}
-	if bytes.Equal(oldValue, b) {
-		return nil
-	}
-
-	if _, err := tx.Exec("UPDATE Configurations SET Active = NULL WHERE Active"); err != nil {
-		return errors.Wrap(err, "failed to deactivate current configuration")
-	}
-
-	if _, err := tx.NamedExec("INSERT INTO Configurations (Id, Value, CreateAt, Active) VALUES (:id, :value, :create_at, TRUE)", params); err != nil {
 		return errors.Wrap(err, "failed to record new configuration")
 	}
 

config/migrations/mysql/000003_update_configurations_sha.down.sql

@@ -0,0 +1,14 @@
+SET @preparedStatement = (SELECT IF(
+    (
+        SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS
+        WHERE table_name = 'Configurations'
+        AND table_schema = DATABASE()
+        AND column_name = 'SHA'
+    ) > 0,
+    'ALTER TABLE Configurations DROP COLUMN SHA;',
+    'SELECT 1'
+));
+
+PREPARE alterIfExists FROM @preparedStatement;
+EXECUTE alterIfExists;
+DEALLOCATE PREPARE alterIfExists;

config/migrations/mysql/000003_update_configurations_sha.up.sql

@@ -0,0 +1,14 @@
+SET @preparedStatement = (SELECT IF(
+    (
+        SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS
+        WHERE table_name = 'Configurations'
+        AND table_schema = DATABASE()
+        AND column_name = 'SHA'
+    ) > 0,
+    'SELECT 1',
+    'ALTER TABLE Configurations ADD COLUMN SHA char(64) DEFAULT "";'
+));
+
+PREPARE alterIfNotExists FROM @preparedStatement;
+EXECUTE alterIfNotExists;
+DEALLOCATE PREPARE alterIfNotExists;

config/migrations/postgres/000003_update_configurations_sha.down.sql

@@ -0,0 +1 @@
+ALTER TABLE Configurations DROP COLUMN IF EXISTS SHA;

config/migrations/postgres/000003_update_configurations_sha.up.sql

@@ -0,0 +1 @@
+ALTER TABLE Configurations ADD COLUMN IF NOT EXISTS SHA CHAR(64) DEFAULT '';