From 66711a61c52e3212daa214766e53ab00ed49fa99 Mon Sep 17 00:00:00 2001 From: Chris Wendt Date: Mon, 20 Jan 2020 17:39:05 -0700 Subject: [PATCH 1/5] atomic migrations --- internal/db/dbutil/dbutil.go | 23 ++++++++++++++++++++++- migrations/migrations_test.go | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/internal/db/dbutil/dbutil.go b/internal/db/dbutil/dbutil.go index 1a72b1f9a899..1d0bce7641d7 100644 --- a/internal/db/dbutil/dbutil.go +++ b/internal/db/dbutil/dbutil.go @@ -8,7 +8,9 @@ import ( "fmt" "net/url" "os" + "path" "strconv" + "strings" "time" // Register driver @@ -110,8 +112,27 @@ func NewDB(dsn, app string) (*sql.DB, error) { return db, nil } +// InjectVersionUpdate fixes the dirty state (set by golang-migrate) after a +// successful migration. See +// https://github.com/golang-migrate/migrate/issues/325 +func InjectVersionUpdate(f bindata.AssetFunc) bindata.AssetFunc { + return func(name string) ([]byte, error) { + oldContents, err := f(name) + if err != nil { + return nil, err + } + versionParts := strings.Split(path.Base(name), "_") + if len(versionParts) < 2 { + return nil, errors.New("expected migration filename to be of the form _") + } + version := versionParts[0] + newContents := strings.Replace(string(oldContents), "COMMIT;", fmt.Sprintf("UPDATE schema_migrations SET version='%s', dirty=false;\nCOMMIT;", version), 1) + return []byte(newContents), nil + } +} + func NewMigrationSourceLoader(dataSource string) *bindata.AssetSource { - return bindata.Resource(migrations.AssetNames(), migrations.Asset) + return bindata.Resource(migrations.AssetNames(), InjectVersionUpdate(migrations.Asset)) } func NewMigrate(db *sql.DB, dataSource string) (*migrate.Migrate, error) { diff --git a/migrations/migrations_test.go b/migrations/migrations_test.go index 006c6b16764f..152512f409aa 100644 --- a/migrations/migrations_test.go +++ b/migrations/migrations_test.go @@ -1,6 +1,7 @@ package migrations_test import ( + "io/ioutil" "path/filepath" "reflect" "sort" @@ -39,6 +40,26 @@ func TestIDConstraints(t *testing.T) { } } +// Makes sure that every migration contains exactly one `COMMIT;` so that +// `InjectVersionUpdate` in internal/db/dbutil/dbutil.go is guaranteed to succeed. +func TestTransactions(t *testing.T) { + ups, err := filepath.Glob("*.up.sql") + if err != nil { + t.Fatal(err) + } + + for _, name := range ups { + contents, err := ioutil.ReadFile(name) + if err != nil { + t.Fatalf("failed to read migration file %q: %v", name, err) + } + commitCount := strings.Count(string(contents), "COMMIT;") + if commitCount != 1 { + t.Fatalf("expected migration %q to contain exactly one COMMIT; but it contains %d", name, commitCount) + } + } +} + func TestNeedsGenerate(t *testing.T) { want, err := filepath.Glob("*.sql") if err != nil { From d8a150f3afe2d3aa9b9d9bd0e2ddab1af7f295bf Mon Sep 17 00:00:00 2001 From: Chris Wendt Date: Mon, 20 Jan 2020 19:46:26 -0700 Subject: [PATCH 2/5] better docstring --- internal/db/dbutil/dbutil.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/db/dbutil/dbutil.go b/internal/db/dbutil/dbutil.go index 1d0bce7641d7..8368fc625fe1 100644 --- a/internal/db/dbutil/dbutil.go +++ b/internal/db/dbutil/dbutil.go @@ -113,8 +113,14 @@ func NewDB(dsn, app string) (*sql.DB, error) { } // InjectVersionUpdate fixes the dirty state (set by golang-migrate) after a -// successful migration. See -// https://github.com/golang-migrate/migrate/issues/325 +// successful migration. If the frontend starts a migration that will turn out +// to be successful but does not stay alive for the duration of the query due to +// a startup timeout, there will be no chance to set the new version or unset +// the dirty flag. This function ensures that each successful migration sets the +// version and dirty flag itself, without requiring the frontend to be alive +// once the migration is committed. +// +// See https://github.com/golang-migrate/migrate/issues/325. func InjectVersionUpdate(f bindata.AssetFunc) bindata.AssetFunc { return func(name string) ([]byte, error) { oldContents, err := f(name) From f342c50ee0e2b0b8ef9444ee85f635e54a259b11 Mon Sep 17 00:00:00 2001 From: Chris Wendt Date: Tue, 21 Jan 2020 13:11:45 -0700 Subject: [PATCH 3/5] do not set the version (was redundant) --- internal/db/dbutil/dbutil.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/db/dbutil/dbutil.go b/internal/db/dbutil/dbutil.go index 8368fc625fe1..c9ffe9593be0 100644 --- a/internal/db/dbutil/dbutil.go +++ b/internal/db/dbutil/dbutil.go @@ -8,7 +8,6 @@ import ( "fmt" "net/url" "os" - "path" "strconv" "strings" "time" @@ -127,12 +126,7 @@ func InjectVersionUpdate(f bindata.AssetFunc) bindata.AssetFunc { if err != nil { return nil, err } - versionParts := strings.Split(path.Base(name), "_") - if len(versionParts) < 2 { - return nil, errors.New("expected migration filename to be of the form _") - } - version := versionParts[0] - newContents := strings.Replace(string(oldContents), "COMMIT;", fmt.Sprintf("UPDATE schema_migrations SET version='%s', dirty=false;\nCOMMIT;", version), 1) + newContents := strings.Replace(string(oldContents), "COMMIT;", fmt.Sprintf("UPDATE schema_migrations SET dirty=false;\nCOMMIT;"), 1) return []byte(newContents), nil } } From 6e1ed36c4dcc9c172528baed812d67665cae9d08 Mon Sep 17 00:00:00 2001 From: Chris Wendt Date: Tue, 21 Jan 2020 13:11:54 -0700 Subject: [PATCH 4/5] unexport --- internal/db/dbutil/dbutil.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/db/dbutil/dbutil.go b/internal/db/dbutil/dbutil.go index c9ffe9593be0..63e417155dfa 100644 --- a/internal/db/dbutil/dbutil.go +++ b/internal/db/dbutil/dbutil.go @@ -111,7 +111,7 @@ func NewDB(dsn, app string) (*sql.DB, error) { return db, nil } -// InjectVersionUpdate fixes the dirty state (set by golang-migrate) after a +// injectVersionUpdate fixes the dirty state (set by golang-migrate) after a // successful migration. If the frontend starts a migration that will turn out // to be successful but does not stay alive for the duration of the query due to // a startup timeout, there will be no chance to set the new version or unset @@ -120,7 +120,7 @@ func NewDB(dsn, app string) (*sql.DB, error) { // once the migration is committed. // // See https://github.com/golang-migrate/migrate/issues/325. -func InjectVersionUpdate(f bindata.AssetFunc) bindata.AssetFunc { +func injectVersionUpdate(f bindata.AssetFunc) bindata.AssetFunc { return func(name string) ([]byte, error) { oldContents, err := f(name) if err != nil { @@ -132,7 +132,7 @@ func InjectVersionUpdate(f bindata.AssetFunc) bindata.AssetFunc { } func NewMigrationSourceLoader(dataSource string) *bindata.AssetSource { - return bindata.Resource(migrations.AssetNames(), InjectVersionUpdate(migrations.Asset)) + return bindata.Resource(migrations.AssetNames(), injectVersionUpdate(migrations.Asset)) } func NewMigrate(db *sql.DB, dataSource string) (*migrate.Migrate, error) { From d6e1c4b4f9ee0b1739140ed993f6bdefd2888775 Mon Sep 17 00:00:00 2001 From: Chris Wendt Date: Tue, 21 Jan 2020 13:28:58 -0700 Subject: [PATCH 5/5] add unit test --- internal/db/dbutil/dbutil_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/db/dbutil/dbutil_test.go b/internal/db/dbutil/dbutil_test.go index 1b8d626d95e3..c6e22443f42e 100644 --- a/internal/db/dbutil/dbutil_test.go +++ b/internal/db/dbutil/dbutil_test.go @@ -67,3 +67,15 @@ func TestPostgresDSN(t *testing.T) { }) } } + +func TestInjectVersionUpdate(t *testing.T) { + gotContents, err := injectVersionUpdate(func(name string) ([]byte, error) { return []byte("BEGIN;\n-- some statements...\nCOMMIT;"), nil })("migrations/100_dummy.up.sql") + if err != nil { + t.Fatal(err) + } + got := string(gotContents) + want := "BEGIN;\n-- some statements...\nUPDATE schema_migrations SET dirty=false;\nCOMMIT;" + if got != want { + t.Errorf("incorrect contents: got != want\ngot: %v\nwant: %v", got, want) + } +}