From 26e4439adc804961ac7d691c133a494f59aae209 Mon Sep 17 00:00:00 2001 From: Eric Yan Date: Tue, 18 Aug 2026 12:29:26 +0000 Subject: [PATCH] move-tables: ensure target tables are dropped in noop mode --- go/logic/migrator.go | 33 ++++++++++++----- go/logic/migrator_test.go | 76 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 8 deletions(-) diff --git a/go/logic/migrator.go b/go/logic/migrator.go index 77059a64f..b1f0483ab 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -3218,13 +3218,18 @@ func (mgtr *Migrator) moveTablesFinalCleanup() error { targetDatabaseName := mgtr.migrationContext.GetTargetDatabaseName() checkpointTableName := mgtr.migrationContext.GetCheckpointTableName() - if mgtr.migrationContext.OkToDropTable { - // The source `__del` rollback handle only exists after a real cutover, - // never in Noop runs. It must be dropped on the source primary: the - // inspector/streamer source connections may point at a read replica, so the - // drop goes through the dedicated source-primary handle. - if !mgtr.migrationContext.Noop { - if err := mgtr.retryOperation(mgtr.dropMoveTablesSourceOldTables); err != nil { + // A resumed noop reuses the target tables and checkpoint from the interrupted + // migration, so it must preserve both. A fresh noop creates target tables and + // a checkpoint only for schema validation, so it must remove those artifacts + // regardless of --ok-to-drop-table. + if mgtr.migrationContext.Noop { + if mgtr.migrationContext.Resume { + return nil + } + for _, mt := range mgtr.migrationContext.OrderedMoveTables() { + if err := mgtr.retryOperation(func() error { + return mgtr.applier.dropTable(mt.TargetTableName) + }); err != nil { return err } } @@ -3236,7 +3241,19 @@ func (mgtr *Migrator) moveTablesFinalCleanup() error { return nil } - if mgtr.migrationContext.Noop { + if mgtr.migrationContext.OkToDropTable { + // The source `__del` rollback handle only exists after a real cutover, + // never in Noop runs. It must be dropped on the source primary: the + // inspector/streamer source connections may point at a read replica, so the + // drop goes through the dedicated source-primary handle. + if err := mgtr.retryOperation(mgtr.dropMoveTablesSourceOldTables); err != nil { + return err + } + if mgtr.migrationContext.Checkpoint { + if err := mgtr.retryOperation(mgtr.applier.DropCheckpointTable); err != nil { + return err + } + } return nil } diff --git a/go/logic/migrator_test.go b/go/logic/migrator_test.go index 06bdc1b4e..a70fe5361 100644 --- a/go/logic/migrator_test.go +++ b/go/logic/migrator_test.go @@ -622,6 +622,82 @@ func (suite *MigratorTestSuite) TestMoveTablesStateInitializesColumnMetadata() { }) } +func (suite *MigratorTestSuite) TestMoveTablesNoopDropsTargetTables() { + ctx := context.Background() + targetTableNames := []string{testMysqlTableName, "test_noop_target"} + + migrationContext := newTestMigrationContext() + migrationContext.Noop = true + migrationContext.Checkpoint = true + migrationContext.MoveTables.TableNames = targetTableNames + migrationContext.MoveTables.TargetDatabase = testMysqlDatabase + migrationContext.InitMoveTableContainers() + + migrator := NewMigrator(migrationContext, "test") + migrator.applier = NewApplier(migrationContext) + migrator.applier.moveTablesTargetDB = suite.db + suite.Require().NoError(migrator.applier.CreateCheckpointTable()) + + for _, tableName := range targetTableNames { + _, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s.%s (id INT PRIMARY KEY)", + sql.EscapeName(testMysqlDatabase), sql.EscapeName(tableName))) + suite.Require().NoError(err) + } + + suite.Require().NoError(migrator.moveTablesFinalCleanup()) + + for _, tableName := range targetTableNames { + exists, err := migrator.applier.targetTableExists(tableName) + suite.Require().NoError(err) + suite.Require().False(exists, "noop cleanup must drop target table %s", tableName) + } + checkpointExists, err := migrator.applier.targetTableExists(migrationContext.GetCheckpointTableName()) + suite.Require().NoError(err) + suite.Require().False(checkpointExists, "fresh noop cleanup must drop its checkpoint table") +} + +func (suite *MigratorTestSuite) TestMoveTablesResumedNoopPreservesTargetTables() { + ctx := context.Background() + targetTableNames := []string{testMysqlTableName, "test_noop_resume_target"} + + migrationContext := newTestMigrationContext() + migrationContext.Noop = true + migrationContext.Resume = true + migrationContext.Checkpoint = true + migrationContext.MoveTables.TableNames = targetTableNames + migrationContext.MoveTables.TargetDatabase = testMysqlDatabase + migrationContext.InitMoveTableContainers() + + migrator := NewMigrator(migrationContext, "test") + migrator.applier = NewApplier(migrationContext) + migrator.applier.moveTablesTargetDB = suite.db + suite.Require().NoError(migrator.applier.CreateCheckpointTable()) + + for _, tableName := range targetTableNames { + _, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s.%s (id INT PRIMARY KEY)", + sql.EscapeName(testMysqlDatabase), sql.EscapeName(tableName))) + suite.Require().NoError(err) + } + defer func() { + for _, tableName := range append(targetTableNames, migrationContext.GetCheckpointTableName()) { + _, err := suite.db.ExecContext(ctx, fmt.Sprintf("DROP TABLE IF EXISTS %s.%s", + sql.EscapeName(testMysqlDatabase), sql.EscapeName(tableName))) + suite.Require().NoError(err) + } + }() + + suite.Require().NoError(migrator.moveTablesFinalCleanup()) + + for _, tableName := range targetTableNames { + exists, err := migrator.applier.targetTableExists(tableName) + suite.Require().NoError(err) + suite.Require().True(exists, "resumed noop cleanup must preserve target table %s", tableName) + } + checkpointExists, err := migrator.applier.targetTableExists(migrationContext.GetCheckpointTableName()) + suite.Require().NoError(err) + suite.Require().True(checkpointExists, "resumed noop cleanup must preserve its checkpoint table") +} + func (suite *MigratorTestSuite) TestRetryBatchCopyWithHooks() { ctx := context.Background()