Revert "chore(db): update schema version in the same transaction as migration (#10321)"

This reverts commit 4459438245.

internal/db/sqlite/basedb.go

@@ -7,14 +7,11 @@
 package sqlite
 
 import (
-	"cmp"
 	"database/sql"
 	"embed"
 	"io/fs"
-	"log/slog"
 	"net/url"
 	"path/filepath"
-	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -22,7 +19,6 @@ import (
 	"time"
 
 	"github.com/jmoiron/sqlx"
-	"github.com/syncthing/syncthing/internal/slogutil"
 	"github.com/syncthing/syncthing/lib/build"
 	"github.com/syncthing/syncthing/lib/protocol"
 )
@@ -46,7 +42,6 @@ type baseDB struct {
 	tplInput      map[string]any
 }
 
-//nolint:noctx
 func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScripts []string) (*baseDB, error) {
 	// Open the database with options to enable foreign keys and recursive
 	// triggers (needed for the delete+insert triggers on row replace).
@@ -95,29 +90,20 @@ func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScript
 	ver, _ := db.getAppliedSchemaVersion()
 	shouldVacuum := false
 	if ver.SchemaVersion > 0 {
-		type migration struct {
+		filter := func(scr string) bool {
-			script  string
+			scr = filepath.Base(scr)
-			version int
+			nstr, _, ok := strings.Cut(scr, "-")
-		}
-		migrations := make([]migration, 0, len(migrationScripts))
-		for _, script := range migrationScripts {
-			base := filepath.Base(script)
-			nstr, _, ok := strings.Cut(base, "-")
 			if !ok {
-				continue
+				return false
 			}
 			n, err := strconv.ParseInt(nstr, 10, 32)
 			if err != nil {
-				continue
+				return false
 			}
-			migrations = append(migrations, migration{
+			return int(n) > ver.SchemaVersion
-				script:  script,
-				version: int(n),
-			})
 		}
-		slices.SortFunc(migrations, func(m1, m2 migration) int { return cmp.Compare(m1.version, m2.version) })
+		for _, script := range migrationScripts {
-		for _, m := range migrations {
+			if err := db.runScripts(script, filter); err != nil {
-			if err := db.applyMigration(m.version, m.script); err != nil {
 				return nil, wrap(err)
 			}
 			shouldVacuum = true
@@ -125,7 +111,7 @@ func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScript
 	}
 
 	// Set the current schema version, if not already set
-	if err := setAppliedSchemaVersion(currentSchemaVersion, db.sql); err != nil {
+	if err := db.setAppliedSchemaVersion(currentSchemaVersion); err != nil {
 		return nil, wrap(err)
 	}
 
@@ -242,7 +228,6 @@ func (f failedStmt) Get(_ any, _ ...any) error           { return f.err }
 func (f failedStmt) Queryx(_ ...any) (*sqlx.Rows, error) { return nil, f.err }
 func (f failedStmt) Select(_ any, _ ...any) error        { return f.err }
 
-//nolint:noctx
 func (s *baseDB) runScripts(glob string, filter ...func(s string) bool) error {
 	scripts, err := fs.Glob(embedded, glob)
 	if err != nil {
@@ -262,53 +247,24 @@ nextScript:
 				continue nextScript
 			}
 		}
-		if err := s.execScript(tx, scr); err != nil {
+		bs, err := fs.ReadFile(embedded, scr)
-			return wrap(err)
+		if err != nil {
+			return wrap(err, scr)
+		}
+		// SQLite requires one statement per exec, so we split the init
+		// files on lines containing only a semicolon and execute them
+		// separately. We require it on a separate line because there are
+		// also statement-internal semicolons in the triggers.
+		for _, stmt := range strings.Split(string(bs), "\n;") {
+			if _, err := tx.Exec(s.expandTemplateVars(stmt)); err != nil {
+				return wrap(err, stmt)
+			}
 		}
 	}
 
 	return wrap(tx.Commit())
 }
 
-//nolint:noctx
-func (s *baseDB) applyMigration(ver int, script string) error {
-	tx, err := s.sql.Begin()
-	if err != nil {
-		return wrap(err)
-	}
-	defer tx.Rollback() //nolint:errcheck
-
-	slog.Info("Applying database migration", slogutil.FilePath(s.baseName), "toSchema", ver, "script", script)
-
-	if err := s.execScript(tx, script); err != nil {
-		return wrap(err)
-	}
-
-	if err := setAppliedSchemaVersion(ver, tx); err != nil {
-		return wrap(err)
-	}
-
-	return wrap(tx.Commit())
-}
-
-//nolint:noctx
-func (s *baseDB) execScript(tx *sql.Tx, scr string) error {
-	bs, err := fs.ReadFile(embedded, scr)
-	if err != nil {
-		return wrap(err, scr)
-	}
-	// SQLite requires one statement per exec, so we split the init
-	// files on lines containing only a semicolon and execute them
-	// separately. We require it on a separate line because there are
-	// also statement-internal semicolons in the triggers.
-	for _, stmt := range strings.Split(string(bs), "\n;") {
-		if _, err := tx.Exec(s.expandTemplateVars(stmt)); err != nil {
-			return wrap(err, stmt)
-		}
-	}
-	return nil
-}
-
 type schemaVersion struct {
 	SchemaVersion    int
 	AppliedAt        int64
@@ -319,11 +275,11 @@ func (s *schemaVersion) AppliedTime() time.Time {
 	return time.Unix(0, s.AppliedAt)
 }
 
-func setAppliedSchemaVersion(ver int, execer sqlx.Execer) error {
+func (s *baseDB) setAppliedSchemaVersion(ver int) error {
-	_, err := execer.Exec(`
+	_, err := s.stmt(`
 		INSERT OR IGNORE INTO schemamigrations (schema_version, applied_at, syncthing_version)
 		VALUES (?, ?, ?)
-	`, ver, time.Now().UnixNano(), build.LongVersion)
+	`).Exec(ver, time.Now().UnixNano(), build.LongVersion)
 	return wrap(err)
 }
 

internal/db/sqlite/sql/README.md

@@ -2,8 +2,7 @@ These SQL scripts are embedded in the binary.
 
 Scripts in `schema/` are run at every startup, in alphanumerical order.
 
-Scripts in `migrations/` are run when a migration is needed; they must begin
+Scripts in `migrations/` are run when a migration is needed; the must begin
 with a number that equals the schema version that results from that
-migration. Only one script per schema version must exist.
+migration. Migrations are not run on initial database creation, so the
-Migrations are not run on initial database creation, so the scripts in `schema/`
+scripts in `schema/` should create the latest version.
-should create the latest version.