From 5d6035cba81b4547df243c92b7f5151612d2ec32 Mon Sep 17 00:00:00 2001 From: Amp Date: Fri, 18 Sep 2026 20:27:48 +0000 Subject: [PATCH] Fix: --resume without --execute drops the ghost/changelog tables finalCleanup gated the ghost table and changelog table drops only on Noop, not on whether the run was a --resume. When resuming a --checkpoint migration without --execute (a dry-run resume), the run correctly skips creating/altering the ghost table (since --resume reuses the state from an interrupted --execute run), but cleanup then unconditionally dropped both the ghost table and the changelog table anyway -- destroying real progress and breaking any later `--resume --execute`. Gate both drops on `!(Noop && Resume)` so a dry-run resume leaves the ghost/changelog tables untouched. Fixes #1769 Amp-Thread-ID: https://ampcode.com/threads/T-01a0b54f-3b21-765b-8d14-6896d57fd738 Co-authored-by: Andrew Mason --- go/logic/migrator.go | 8 ++- go/logic/migrator_test.go | 122 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 3 deletions(-) diff --git a/go/logic/migrator.go b/go/logic/migrator.go index f2f6b3f20..e4468a826 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -2015,8 +2015,10 @@ func (mgtr *Migrator) finalCleanup() error { mgtr.migrationContext.Log.Errore(err) } - if err := mgtr.retryOperation(mgtr.applier.DropChangelogTable); err != nil { - return err + if !mgtr.migrationContext.Noop || !mgtr.migrationContext.Resume { + if err := mgtr.retryOperation(mgtr.applier.DropChangelogTable); err != nil { + return err + } } if mgtr.migrationContext.OkToDropTable && !mgtr.migrationContext.TestOnReplica { if err := mgtr.retryOperation(mgtr.applier.DropOldTable); err != nil { @@ -2033,7 +2035,7 @@ func (mgtr *Migrator) finalCleanup() error { mgtr.migrationContext.Log.Infof("-- drop table %s.%s", sql.EscapeName(mgtr.migrationContext.DatabaseName), sql.EscapeName(mgtr.migrationContext.GetCheckpointTableName())) } } - if mgtr.migrationContext.Noop { + if mgtr.migrationContext.Noop && !mgtr.migrationContext.Resume { if err := mgtr.retryOperation(mgtr.applier.DropGhostTable); err != nil { return err } diff --git a/go/logic/migrator_test.go b/go/logic/migrator_test.go index ad068691c..2450189dc 100644 --- a/go/logic/migrator_test.go +++ b/go/logic/migrator_test.go @@ -1428,6 +1428,128 @@ func (suite *MigratorTestSuite) TestRevert() { suite.Require().Equal(checksum1, checksum2) } +// TestResumeWithoutExecuteDoesNotDropGhostTable is a regression test for +// https://github.com/github/gh-ost/issues/1769: resuming a --checkpoint +// migration without --execute (i.e. a dry-run resume) must not drop the +// ghost table, since it holds real progress from the interrupted --execute +// run and a later `--resume --execute` still needs it. +func (suite *MigratorTestSuite) TestResumeWithoutExecuteDoesNotDropGhostTable() { + ctx := context.Background() + + _, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT PRIMARY KEY AUTO_INCREMENT, name TEXT)", getTestTableName())) + suite.Require().NoError(err) + + // Seed enough rows, and slow the copy down via --nice-ratio (SetNiceRatio), + // so row-copy reliably outlasts the first checkpoint and we get a window + // to interrupt the migration mid-copy. + _, err = suite.db.ExecContext(ctx, "INSERT INTO "+getTestTableName()+" (name) VALUES ('a'),('a'),('a'),('a')") + suite.Require().NoError(err) + for range 12 { // 4 * 2^12 = 16384 rows + _, err = suite.db.ExecContext(ctx, "INSERT INTO "+getTestTableName()+" (name) SELECT name FROM "+getTestTableName()) + suite.Require().NoError(err) + } + + connectionConfig, err := getTestConnectionConfig(ctx, suite.mysqlContainer) + suite.Require().NoError(err) + + tableExists := func(name string) bool { + var one int + err := suite.db.QueryRowContext(ctx, + "SELECT 1 FROM information_schema.tables WHERE table_schema=? AND table_name=?", + testMysqlDatabase, name).Scan(&one) + return err == nil + } + + // --- first run: interrupt mid-copy, leaving checkpoint/ghost tables behind --- + migrationContext := newTestMigrationContext() + migrationContext.ApplierConnectionConfig = connectionConfig + migrationContext.InspectorConnectionConfig = connectionConfig + migrationContext.SetConnectionConfig("innodb") + migrationContext.AlterStatement = "ADD COLUMN newcol INT" + migrationContext.AlterStatementOptions = "ADD COLUMN newcol INT" + migrationContext.Checkpoint = true + migrationContext.CheckpointIntervalSeconds = 1 + migrationContext.DropServeSocket = true + migrationContext.UseGTIDs = true + migrationContext.SetChunkSize(50) + migrationContext.SetNiceRatio(50) + + migrator := NewMigrator(migrationContext, "0.0.0") + + migrateErrCh := make(chan error, 1) + go func() { + migrateErrCh <- migrator.Migrate() + }() + + checkpointTable := fmt.Sprintf("`%s`.`%s`", testMysqlDatabase, migrationContext.GetCheckpointTableName()) + changelogTable := fmt.Sprintf("`%s`.`%s`", testMysqlDatabase, migrationContext.GetChangelogTableName()) + defer func() { + _, _ = suite.db.ExecContext(ctx, "DROP TABLE IF EXISTS "+checkpointTable) + _, _ = suite.db.ExecContext(ctx, "DROP TABLE IF EXISTS "+changelogTable) + }() + + checkpointed := false + for range 100 { + var count int + if err := suite.db.QueryRowContext(ctx, "SELECT COUNT(*) FROM "+checkpointTable).Scan(&count); err == nil && count > 0 { + checkpointed = true + break + } + time.Sleep(100 * time.Millisecond) + } + suite.Require().True(checkpointed, "expected a checkpoint to be written before row copy completed") + + // Abort without cleanup, like the interactive `panic` command: this leaves + // the ghost, changelog and checkpoint tables in place for the resume. + migrationContext.PanicAbort <- errors.New("simulated interruption for TestResumeWithoutExecuteDoesNotDropGhostTable") + suite.Require().Error(<-migrateErrCh) + + suite.Require().True(tableExists(migrationContext.GetGhostTableName()), "ghost table should exist after interrupted migration") + + // --- second run: dry-run resume (--resume without --execute) must not touch the ghost table --- + resumeContext := newTestMigrationContext() + resumeContext.ApplierConnectionConfig = connectionConfig + resumeContext.InspectorConnectionConfig = connectionConfig + resumeContext.SetConnectionConfig("innodb") + resumeContext.AlterStatement = migrationContext.AlterStatement + resumeContext.Checkpoint = true + resumeContext.CheckpointIntervalSeconds = 1 + resumeContext.DropServeSocket = true + resumeContext.UseGTIDs = true + resumeContext.Resume = true + resumeContext.Noop = true // no --execute + + resumeMigrator := NewMigrator(resumeContext, "0.0.0") + err = resumeMigrator.Migrate() + suite.Require().NoError(err) + + suite.Require().True(tableExists(migrationContext.GetGhostTableName()), "dry-run --resume must not drop the ghost table") + + // --- third run: a real `--resume --execute` should still pick up where the aborted run left off --- + finishContext := newTestMigrationContext() + finishContext.ApplierConnectionConfig = connectionConfig + finishContext.InspectorConnectionConfig = connectionConfig + finishContext.SetConnectionConfig("innodb") + finishContext.AlterStatement = migrationContext.AlterStatement + finishContext.Checkpoint = true + finishContext.CheckpointIntervalSeconds = 1 + finishContext.DropServeSocket = true + finishContext.UseGTIDs = true + finishContext.Resume = true + finishContext.OkToDropTable = true + finishContext.InitiallyDropOldTable = true + + finishMigrator := NewMigrator(finishContext, "0.0.0") + suite.Require().NoError(finishMigrator.Migrate()) + + var colCount int + err = suite.db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM information_schema.columns WHERE table_schema=? AND table_name=? AND column_name='newcol'", + testMysqlDatabase, testMysqlTableName).Scan(&colCount) + suite.Require().NoError(err) + suite.Require().Equal(1, colCount, "resumed migration should have completed the ALTER") +} + func TestMigrator(t *testing.T) { if testing.Short() { t.Skip("skipping migrator test suite in short mode")