migrations: Better present migration_logs over schema_migrations (sourcegraph#30309)

Author: efritz

internal/database/migration/store/store.go

@@ -19,8 +19,8 @@ import (
 
 type Store struct {
 	*basestore.Store
-	migrationsTable string
-	operations      *Operations
+	schemaName string
+	operations *Operations
 }
 
 // IndexStatus describes the state of an index. Is{Valid,Ready,Live} is taken
@@ -64,17 +64,17 @@ var CreateIndexConcurrentlyPhases = []string{
 
 func NewWithDB(db dbutil.DB, migrationsTable string, operations *Operations) *Store {
 	return &Store{
-		Store:           basestore.NewWithDB(db, sql.TxOptions{}),
-		migrationsTable: migrationsTable,
-		operations:      operations,
+		Store:      basestore.NewWithDB(db, sql.TxOptions{}),
+		schemaName: migrationsTable,
+		operations: operations,
 	}
 }
 
 func (s *Store) With(other basestore.ShareableStore) *Store {
 	return &Store{
-		Store:           s.Store.With(other),
-		migrationsTable: s.migrationsTable,
-		operations:      s.operations,
+		Store:      s.Store.With(other),
+		schemaName: s.schemaName,
+		operations: s.operations,
 	}
 }
 
@@ -85,9 +85,9 @@ func (s *Store) Transact(ctx context.Context) (*Store, error) {
 	}
 
 	return &Store{
-		Store:           txBase,
-		migrationsTable: s.migrationsTable,
-		operations:      s.operations,
+		Store:      txBase,
+		schemaName: s.schemaName,
+		operations: s.operations,
 	}, nil
 }
 
@@ -98,8 +98,8 @@ func (s *Store) EnsureSchemaTable(ctx context.Context) (err error) {
 	defer endObservation(1, observation.Args{})
 
 	queries := []*sqlf.Query{
-		sqlf.Sprintf(`CREATE TABLE IF NOT EXISTS %s(version bigint NOT NULL PRIMARY KEY)`, quote(s.migrationsTable)),
-		sqlf.Sprintf(`ALTER TABLE %s ADD COLUMN IF NOT EXISTS dirty boolean NOT NULL`, quote(s.migrationsTable)),
+		sqlf.Sprintf(`CREATE TABLE IF NOT EXISTS %s(version bigint NOT NULL PRIMARY KEY)`, quote(s.schemaName)),
+		sqlf.Sprintf(`ALTER TABLE %s ADD COLUMN IF NOT EXISTS dirty boolean NOT NULL`, quote(s.schemaName)),
 
 		sqlf.Sprintf(`CREATE TABLE IF NOT EXISTS migration_logs(id SERIAL PRIMARY KEY)`),
 		sqlf.Sprintf(`ALTER TABLE migration_logs ADD COLUMN IF NOT EXISTS migration_logs_schema_version integer NOT NULL`),
@@ -131,7 +131,7 @@ func (s *Store) Version(ctx context.Context) (version int, dirty bool, ok bool,
 	ctx, endObservation := s.operations.version.With(ctx, &err, observation.Args{})
 	defer endObservation(1, observation.Args{})
 
-	rows, err := s.Query(ctx, sqlf.Sprintf(`SELECT version, dirty FROM %s`, quote(s.migrationsTable)))
+	rows, err := s.Query(ctx, sqlf.Sprintf(`SELECT version, dirty FROM %s`, quote(s.schemaName)))
 	if err != nil {
 		return 0, false, false, err
 	}
@@ -212,7 +212,7 @@ func (s *Store) TryLock(ctx context.Context) (_ bool, _ func(err error) error, e
 }
 
 func (s *Store) lockKey() int32 {
-	return locker.StringKey(fmt.Sprintf("%s:migrations", s.migrationsTable))
+	return locker.StringKey(fmt.Sprintf("%s:migrations", s.schemaName))
 }
 
 // Up runs the given definition's up query.
@@ -276,16 +276,21 @@ func (s *Store) WithMigrationLog(ctx context.Context, definition definition.Defi
 		expectedCurrentVersion = definitionVersion
 	}
 
-	logID, err := s.setVersion(ctx, up, expectedCurrentVersion, targetVersion, definitionVersion)
+	logID, err := s.createMigrationLog(ctx, up, expectedCurrentVersion, targetVersion, definitionVersion)
 	if err != nil {
 		return err
 	}
 
+	defer func() {
+		if err == nil {
+			err = s.Exec(ctx, sqlf.Sprintf(`UPDATE %s SET dirty = false`, quote(s.schemaName)))
+		}
+	}()
 	defer func() {
 		if execErr := s.Exec(ctx, sqlf.Sprintf(
 			`UPDATE migration_logs SET finished_at = NOW(), success = %s, error_message = %s WHERE id = %d`,
 			err == nil,
-			strPtr(err),
+			errMsgPtr(err),
 			logID,
 		)); execErr != nil {
 			err = multierror.Append(err, execErr)
@@ -296,14 +301,10 @@ func (s *Store) WithMigrationLog(ctx context.Context, definition definition.Defi
 		return err
 	}
 
-	if err := s.Exec(ctx, sqlf.Sprintf(`UPDATE %s SET dirty=false`, quote(s.migrationsTable))); err != nil {
-		return err
-	}
-
 	return nil
 }
 
-func (s *Store) setVersion(ctx context.Context, up bool, expectedCurrentVersion, targetVersion, sourceVersion int) (_ int, err error) {
+func (s *Store) createMigrationLog(ctx context.Context, up bool, expectedCurrentVersion, targetVersion, sourceVersion int) (_ int, err error) {
 	tx, err := s.Transact(ctx)
 	if err != nil {
 		return 0, err
@@ -314,7 +315,6 @@ func (s *Store) setVersion(ctx context.Context, up bool, expectedCurrentVersion,
 		cta := "This condition should not be reachable by normal use of the migration store via the runner and indicates a bug. Please report this issue."
 		return errors.Errorf(description+"\n\n"+cta+"\n", args...)
 	}
-
 	if currentVersion, dirty, ok, err := tx.Version(ctx); err != nil {
 		return 0, err
 	} else if dirty {
@@ -324,12 +324,12 @@ func (s *Store) setVersion(ctx context.Context, up bool, expectedCurrentVersion,
 			return 0, assertionFailure("expected schema to have version %d, but has version %d\n", expectedCurrentVersion, currentVersion)
 		}
 
-		if err := tx.Exec(ctx, sqlf.Sprintf(`DELETE FROM %s`, quote(s.migrationsTable))); err != nil {
+		if err := tx.Exec(ctx, sqlf.Sprintf(`DELETE FROM %s`, quote(s.schemaName))); err != nil {
 			return 0, err
 		}
 	}
 
-	if err := tx.Exec(ctx, sqlf.Sprintf(`INSERT INTO %s (version, dirty) VALUES (%s, true)`, quote(s.migrationsTable), targetVersion)); err != nil {
+	if err := tx.Exec(ctx, sqlf.Sprintf(`INSERT INTO %s (version, dirty) VALUES (%s, true)`, quote(s.schemaName), targetVersion)); err != nil {
 		return 0, err
 	}
 
@@ -345,7 +345,7 @@ func (s *Store) setVersion(ctx context.Context, up bool, expectedCurrentVersion,
 			RETURNING id
 		`,
 		currentMigrationLogSchemaVersion,
-		s.migrationsTable,
+		s.schemaName,
 		sourceVersion,
 		up,
 	)))
@@ -358,7 +358,7 @@ func (s *Store) setVersion(ctx context.Context, up bool, expectedCurrentVersion,
 
 var quote = sqlf.Sprintf
 
-func strPtr(err error) *string {
+func errMsgPtr(err error) *string {
 	if err == nil {
 		return nil
 	}