From 4459438245de4e114047756bb9debfe1634116eb Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Sat, 30 Aug 2025 13:18:31 +0200 Subject: [PATCH] chore(db): update schema version in the same transaction as migration (#10321) Just to be entirely sure that if the migration succeeds the schema version is always also updated. Currently if a migration succeeds but a later migration doesn't, the changes of the migration apply but the version stays - if the migration is breaking/non-idempotent, it will fail when it tries to rerun it next time (otherwise it's just a pointless re-execution). Unfortunately with the current `db.runScripts` it wasn't that easy to do, so I had to do quite a bit of refactoring. I am also ensuring the right order of transactions now, though I assume that was already the case lexicographically - can't hurt to be safe. --- internal/db/sqlite/basedb.go | 88 +++++++++++++++++++++++--------- internal/db/sqlite/sql/README.md | 7 +-- 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/internal/db/sqlite/basedb.go b/internal/db/sqlite/basedb.go index b421bd9fa..3c42f8360 100644 --- a/internal/db/sqlite/basedb.go +++ b/internal/db/sqlite/basedb.go @@ -7,11 +7,13 @@ package sqlite import ( + "cmp" "database/sql" "embed" "io/fs" "net/url" "path/filepath" + "slices" "strconv" "strings" "sync" @@ -42,6 +44,7 @@ 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). @@ -89,27 +92,36 @@ func openBase(path string, maxConns int, pragmas, schemaScripts, migrationScript ver, _ := db.getAppliedSchemaVersion() if ver.SchemaVersion > 0 { - filter := func(scr string) bool { - scr = filepath.Base(scr) - nstr, _, ok := strings.Cut(scr, "-") + 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, "-") if !ok { - return false + continue } n, err := strconv.ParseInt(nstr, 10, 32) if err != nil { - return false + continue } - return int(n) > ver.SchemaVersion + migrations = append(migrations, migration{ + script: script, + version: int(n), + }) } - for _, script := range migrationScripts { - if err := db.runScripts(script, filter); err != nil { + 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 { return nil, wrap(err) } } } // Set the current schema version, if not already set - if err := db.setAppliedSchemaVersion(currentSchemaVersion); err != nil { + if err := setAppliedSchemaVersion(currentSchemaVersion, db.sql); err != nil { return nil, wrap(err) } @@ -204,6 +216,7 @@ 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 { @@ -223,24 +236,51 @@ nextScript: continue nextScript } } - 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) - } + if err := s.execScript(tx, scr); err != nil { + return wrap(err) } } 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 + + 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 @@ -251,11 +291,11 @@ func (s *schemaVersion) AppliedTime() time.Time { return time.Unix(0, s.AppliedAt) } -func (s *baseDB) setAppliedSchemaVersion(ver int) error { - _, err := s.stmt(` +func setAppliedSchemaVersion(ver int, execer sqlx.Execer) error { + _, err := execer.Exec(` INSERT OR IGNORE INTO schemamigrations (schema_version, applied_at, syncthing_version) VALUES (?, ?, ?) - `).Exec(ver, time.Now().UnixNano(), build.LongVersion) + `, 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 42929fd51..06cef63fe 100644 --- a/internal/db/sqlite/sql/README.md +++ b/internal/db/sqlite/sql/README.md @@ -2,7 +2,8 @@ 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; the must begin +Scripts in `migrations/` are run when a migration is needed; they must begin with a number that equals the schema version that results from that -migration. Migrations are not run on initial database creation, so the -scripts in `schema/` should create the latest version. +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.