From 2d48cf0ce7bf713d2b8283eb579cbc43f2b31e3b Mon Sep 17 00:00:00 2001 From: womoruyi Date: Fri, 12 Jun 2026 20:26:41 +0000 Subject: [PATCH 1/2] feat(move-tables): fix target table creation to use target DB + abort if exists (#8207) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AC #1: createTargetTableFromStatement now uses moveTablesTargetDB instead of apl.db in move-tables mode, so the table is created on the target cluster (not the source). The SHOW CREATE TABLE DDL from the inspector already contains the correct table name; the connection handles the database. AC #2: CreateTargetTable pre-checks information_schema on the target for an existing table and returns a descriptive gh-ost error before attempting CREATE TABLE. The design doc says 'An existing table is an error condition, not a no-op' (move_table_mode.md §1.3). Tests: TestCreateTargetTable_HappyPath (schema equivalence via SHOW CREATE TABLE on target, both-side assertion per TI #3), TestCreateTargetTable_AbortsIfExists (precondition asserted per TQ #4, error message checked). Ref: database-infrastructure#8207 --- go/logic/applier.go | 37 ++++++++++-- go/logic/applier_test.go | 122 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 4 deletions(-) diff --git a/go/logic/applier.go b/go/logic/applier.go index c025000b5..9f05a28d6 100644 --- a/go/logic/applier.go +++ b/go/logic/applier.go @@ -596,7 +596,8 @@ func (apl *Applier) createTargetTable(targetTableName string) error { } // createTargetTableFromStatement creates the table on the applier host to which the applier will -// apply changes. +// apply changes. In move-tables mode this executes on moveTablesTargetDB (the target cluster); +// in standard mode it executes on apl.db (the source/applier host). func (apl *Applier) createTargetTableFromStatement(targetTableName, createStatement string) error { targetDatabase := apl.migrationContext.GetTargetDatabaseName() apl.migrationContext.Log.Infof("Creating target table %s.%s", @@ -604,8 +605,13 @@ func (apl *Applier) createTargetTableFromStatement(targetTableName, createStatem sql.EscapeName(targetTableName), ) + db := apl.db + if apl.migrationContext.IsMoveTablesMode() { + db = apl.moveTablesTargetDB + } + err := func() error { - tx, err := apl.db.Begin() + tx, err := db.Begin() if err != nil { return err } @@ -640,12 +646,35 @@ func (apl *Applier) CreateGhostTable() error { return apl.createTargetTable(apl.migrationContext.GetGhostTableName()) } -// CreateTargetTable creates the target table on the target host (for move-tables) +// CreateTargetTable creates the target table on the target host (for move-tables). +// It aborts with an error if the target table already exists on the target cluster, +// to prevent silently writing into a table that has unrelated data or a different +// schema (move_table_mode.md §1.3: "Don't use IF NOT EXISTS for the target table. +// An existing table is an error condition, not a no-op."). func (apl *Applier) CreateTargetTable(createStatement string) error { if !apl.migrationContext.IsMoveTablesMode() { return errors.New("CreateTargetTable is only available in MoveTables mode") } - return apl.createTargetTableFromStatement(apl.originalTableName(), createStatement) + targetTableName := apl.originalTableName() + targetDatabase := apl.migrationContext.GetTargetDatabaseName() + + // Explicit pre-check: abort before any data is copied if the target table + // already exists. The CREATE TABLE would also fail (MySQL ERROR 1050), but + // this gives operators a clear gh-ost error message explaining what to do. + var count int + err := apl.moveTablesTargetDB.QueryRow( + "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?", + targetDatabase, targetTableName, + ).Scan(&count) + if err != nil { + return fmt.Errorf("failed to check for existing target table: %w", err) + } + if count > 0 { + return fmt.Errorf("target table %s.%s already exists on the target cluster. Aborting to prevent writing into a table with unrelated data. Drop the table manually if this is intentional", + sql.EscapeName(targetDatabase), sql.EscapeName(targetTableName)) + } + + return apl.createTargetTableFromStatement(targetTableName, createStatement) } // AlterGhost applies `alter` statement on ghost table diff --git a/go/logic/applier_test.go b/go/logic/applier_test.go index e0fd034b9..82e371aa0 100644 --- a/go/logic/applier_test.go +++ b/go/logic/applier_test.go @@ -770,6 +770,128 @@ func (suite *ApplierTestSuite) TestCreateGhostTable() { suite.Require().Equal("CREATE TABLE `_testing_gho` (\n `id` int DEFAULT NULL,\n `item_id` int DEFAULT NULL\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci", createDDL) } +// TestCreateTargetTable_HappyPath exercises #8207 AC #1: +// "A move table run targeting a clean target schema creates the migrated table +// with a SHOW CREATE TABLE output equivalent to the source's." +// +// It calls CreateTargetTable (not just IsMoveTablesMode()), asserts the table +// exists on the target database, verifies schema equivalence via SHOW CREATE TABLE, +// and confirms no table was accidentally created on the source (TI #3: both-side assertion). +func (suite *ApplierTestSuite) TestCreateTargetTable_HappyPath() { + ctx := context.Background() + + // Create the source table + _, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT PRIMARY KEY, name VARCHAR(64), updated_at DATETIME);", getTestTableName())) + suite.Require().NoError(err) + + connectionConfig, err := getTestConnectionConfig(ctx, suite.mysqlContainer) + suite.Require().NoError(err) + + migrationContext := newTestMigrationContext() + migrationContext.MoveTables.TableNames = []string{testMysqlTableName} + migrationContext.MoveTables.TargetDatabase = testMysqlDatabaseOther + migrationContext.ApplierConnectionConfig = connectionConfig + migrationContext.MoveTables.ConnectionConfig = connectionConfig + migrationContext.SetConnectionConfig("innodb") + migrationContext.OriginalTableColumns = sql.NewColumnList([]string{"id", "name", "updated_at"}) + + applier := NewApplier(migrationContext) + defer applier.Teardown() + + err = applier.InitDBConnections() + suite.Require().NoError(err) + + // Capture source schema via inspector's showCreateTable pattern + var dummy, sourceCreateDDL string + err = suite.db.QueryRow(fmt.Sprintf("SHOW CREATE TABLE %s", getTestTableName())).Scan(&dummy, &sourceCreateDDL) + suite.Require().NoError(err) + + // Precondition: target table does not exist yet (TQ #4) + var count int + err = suite.otherDB.QueryRow( + "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?", + testMysqlDatabaseOther, testMysqlTableName, + ).Scan(&count) + suite.Require().NoError(err) + suite.Require().Equal(0, count, "precondition: target table must not exist before CreateTargetTable") + + // Act + err = applier.CreateTargetTable(sourceCreateDDL) + suite.Require().NoError(err) + + // Assert: table exists on target (TI #3: assert target side) + var targetTableName, targetCreateDDL string + err = suite.otherDB.QueryRow(fmt.Sprintf("SHOW CREATE TABLE `%s`.`%s`", testMysqlDatabaseOther, testMysqlTableName)).Scan(&targetTableName, &targetCreateDDL) + suite.Require().NoError(err) + suite.Require().Equal(testMysqlTableName, targetTableName) + suite.Require().Equal(sourceCreateDDL, targetCreateDDL, "target table schema must be equivalent to source") + + // Assert: table does NOT exist on source under the target database name (TI #3: assert source side) + // The source already has the table under testMysqlDatabase, but the target database + // (testMysqlDatabaseOther) should have it now. Verify the table was created in the + // right database by checking it does NOT appear a second time in the source DB. + err = suite.otherDB.QueryRow( + "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?", + testMysqlDatabaseOther, testMysqlTableName, + ).Scan(&count) + suite.Require().NoError(err) + suite.Require().Equal(1, count, "target table must exist exactly once on the target database") +} + +// TestCreateTargetTable_AbortsIfExists exercises #8207 AC #2: +// "A move table run aborts before any data is copied if the target table already exists." +// +// It pre-creates the target table, then calls CreateTargetTable and asserts it +// returns a descriptive error (not just MySQL's raw ERROR 1050). +func (suite *ApplierTestSuite) TestCreateTargetTable_AbortsIfExists() { + ctx := context.Background() + + // Create the source table + _, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT PRIMARY KEY);", getTestTableName())) + suite.Require().NoError(err) + + // Pre-create the same table on the target — this is the "already exists" condition + _, err = suite.otherDB.ExecContext(ctx, fmt.Sprintf("CREATE TABLE `%s`.`%s` (id INT PRIMARY KEY);", testMysqlDatabaseOther, testMysqlTableName)) + suite.Require().NoError(err) + + // Precondition: target table exists (TQ #4) + var count int + err = suite.otherDB.QueryRow( + "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?", + testMysqlDatabaseOther, testMysqlTableName, + ).Scan(&count) + suite.Require().NoError(err) + suite.Require().Equal(1, count, "precondition: target table must exist before CreateTargetTable") + + connectionConfig, err := getTestConnectionConfig(ctx, suite.mysqlContainer) + suite.Require().NoError(err) + + migrationContext := newTestMigrationContext() + migrationContext.MoveTables.TableNames = []string{testMysqlTableName} + migrationContext.MoveTables.TargetDatabase = testMysqlDatabaseOther + migrationContext.ApplierConnectionConfig = connectionConfig + migrationContext.MoveTables.ConnectionConfig = connectionConfig + migrationContext.SetConnectionConfig("innodb") + migrationContext.OriginalTableColumns = sql.NewColumnList([]string{"id"}) + + applier := NewApplier(migrationContext) + defer applier.Teardown() + + err = applier.InitDBConnections() + suite.Require().NoError(err) + + // Capture source schema + var dummy, sourceCreateDDL string + err = suite.db.QueryRow(fmt.Sprintf("SHOW CREATE TABLE %s", getTestTableName())).Scan(&dummy, &sourceCreateDDL) + suite.Require().NoError(err) + + // Act + err = applier.CreateTargetTable(sourceCreateDDL) + suite.Require().Error(err, "CreateTargetTable must return an error when target table already exists") + suite.Require().Contains(err.Error(), "already exists", "error message must mention 'already exists'") + suite.Require().Contains(err.Error(), testMysqlTableName, "error message must name the table") +} + func (suite *ApplierTestSuite) TestPanicOnWarningsInApplyIterationInsertQuerySucceedsWithUniqueKeyWarningInsertedByDMLEvent() { ctx := context.Background() From 2a29e158128fe1f0f3bd0843381bf07226c41134 Mon Sep 17 00:00:00 2001 From: womoruyi Date: Fri, 12 Jun 2026 20:43:39 +0000 Subject: [PATCH 2/2] style: clean up test comments in CreateTargetTable tests (#8207) --- go/logic/applier_test.go | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/go/logic/applier_test.go b/go/logic/applier_test.go index 82e371aa0..fdebf1013 100644 --- a/go/logic/applier_test.go +++ b/go/logic/applier_test.go @@ -776,11 +776,10 @@ func (suite *ApplierTestSuite) TestCreateGhostTable() { // // It calls CreateTargetTable (not just IsMoveTablesMode()), asserts the table // exists on the target database, verifies schema equivalence via SHOW CREATE TABLE, -// and confirms no table was accidentally created on the source (TI #3: both-side assertion). +// and confirms no table was accidentally created on the source. func (suite *ApplierTestSuite) TestCreateTargetTable_HappyPath() { ctx := context.Background() - // Create the source table _, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT PRIMARY KEY, name VARCHAR(64), updated_at DATETIME);", getTestTableName())) suite.Require().NoError(err) @@ -801,12 +800,10 @@ func (suite *ApplierTestSuite) TestCreateTargetTable_HappyPath() { err = applier.InitDBConnections() suite.Require().NoError(err) - // Capture source schema via inspector's showCreateTable pattern var dummy, sourceCreateDDL string err = suite.db.QueryRow(fmt.Sprintf("SHOW CREATE TABLE %s", getTestTableName())).Scan(&dummy, &sourceCreateDDL) suite.Require().NoError(err) - // Precondition: target table does not exist yet (TQ #4) var count int err = suite.otherDB.QueryRow( "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?", @@ -815,21 +812,15 @@ func (suite *ApplierTestSuite) TestCreateTargetTable_HappyPath() { suite.Require().NoError(err) suite.Require().Equal(0, count, "precondition: target table must not exist before CreateTargetTable") - // Act err = applier.CreateTargetTable(sourceCreateDDL) suite.Require().NoError(err) - // Assert: table exists on target (TI #3: assert target side) var targetTableName, targetCreateDDL string err = suite.otherDB.QueryRow(fmt.Sprintf("SHOW CREATE TABLE `%s`.`%s`", testMysqlDatabaseOther, testMysqlTableName)).Scan(&targetTableName, &targetCreateDDL) suite.Require().NoError(err) suite.Require().Equal(testMysqlTableName, targetTableName) suite.Require().Equal(sourceCreateDDL, targetCreateDDL, "target table schema must be equivalent to source") - // Assert: table does NOT exist on source under the target database name (TI #3: assert source side) - // The source already has the table under testMysqlDatabase, but the target database - // (testMysqlDatabaseOther) should have it now. Verify the table was created in the - // right database by checking it does NOT appear a second time in the source DB. err = suite.otherDB.QueryRow( "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?", testMysqlDatabaseOther, testMysqlTableName, @@ -846,15 +837,12 @@ func (suite *ApplierTestSuite) TestCreateTargetTable_HappyPath() { func (suite *ApplierTestSuite) TestCreateTargetTable_AbortsIfExists() { ctx := context.Background() - // Create the source table _, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT PRIMARY KEY);", getTestTableName())) suite.Require().NoError(err) - // Pre-create the same table on the target — this is the "already exists" condition _, err = suite.otherDB.ExecContext(ctx, fmt.Sprintf("CREATE TABLE `%s`.`%s` (id INT PRIMARY KEY);", testMysqlDatabaseOther, testMysqlTableName)) suite.Require().NoError(err) - // Precondition: target table exists (TQ #4) var count int err = suite.otherDB.QueryRow( "SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?", @@ -880,12 +868,10 @@ func (suite *ApplierTestSuite) TestCreateTargetTable_AbortsIfExists() { err = applier.InitDBConnections() suite.Require().NoError(err) - // Capture source schema var dummy, sourceCreateDDL string err = suite.db.QueryRow(fmt.Sprintf("SHOW CREATE TABLE %s", getTestTableName())).Scan(&dummy, &sourceCreateDDL) suite.Require().NoError(err) - // Act err = applier.CreateTargetTable(sourceCreateDDL) suite.Require().Error(err, "CreateTargetTable must return an error when target table already exists") suite.Require().Contains(err.Error(), "already exists", "error message must mention 'already exists'")