From 21ad99c80af29239cd6881c4e33abfac73e7fac6 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sun, 31 Aug 2025 12:04:15 +0200 Subject: [PATCH] Revert "chore(db): update schema version in the same transaction as migration (#10321)" This reverts commit 4459438245de4e114047756bb9debfe1634116eb. --- internal/db/sqlite/basedb.go | 92 +++++++++----------------------- internal/db/sqlite/sql/README.md | 7 ++- 2 files changed, 27 insertions(+), 72 deletions(-) diff --git a/internal/db/sqlite/basedb.go b/internal/db/sqlite/basedb.go index 8eb91713b..85f6a7cb5 100644 --- a/internal/db/sqlite/basedb.go +++ b/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 { - script string - version int - } - migrations := make([]migration, 0, len(migrationScripts)) - for _, script := range migrationScripts { - base := filepath.Base(script) - nstr, _, ok := strings.Cut(base, "-") + filter := func(scr string) bool { + scr = filepath.Base(scr) + nstr, _, ok := strings.Cut(scr, "-") if !ok { - continue + return false } n, err := strconv.ParseInt(nstr, 10, 32) if err != nil { - continue + return false } - migrations = append(migrations, migration{ - script: script, - version: int(n), - }) + return int(n) > ver.SchemaVersion } - slices.SortFunc(migrations, func(m1, m2 migration) int { return cmp.Compare(m1.version, m2.version) }) - for _, m := range migrations { - if err := db.applyMigration(m.version, m.script); err != nil { + for _, script := range migrationScripts { + if err := db.runScripts(script, filter); 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 { - return wrap(err) + 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 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 { - _, err := execer.Exec(` +func (s *baseDB) setAppliedSchemaVersion(ver int) error { + _, 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) } diff --git a/internal/db/sqlite/sql/README.md b/internal/db/sqlite/sql/README.md index 06cef63fe..42929fd51 100644 --- a/internal/db/sqlite/sql/README.md +++ b/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. -Migrations are not run on initial database creation, so the scripts in `schema/` -should create the latest version. +migration. Migrations are not run on initial database creation, so the +scripts in `schema/` should create the latest version.