Skip to content

Commit bb029da

Browse files
zacharysierakowskizacharysierakowski
authored
Merge pull request #1710 from github/womoruyi/move-tables-1.3-target-table-crud
feat(move-tables): fix target table creation to use target DB + abort if exists (#8207)
2 parents 865282c + 2a29e15 commit bb029da

2 files changed

Lines changed: 141 additions & 4 deletions

File tree

‎go/logic/applier.go‎

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -596,16 +596,22 @@ func (apl *Applier) createTargetTable(targetTableName string) error {
596596
}
597597

598598
// createTargetTableFromStatement creates the table on the applier host to which the applier will
599-
// apply changes.
599+
// apply changes. In move-tables mode this executes on moveTablesTargetDB (the target cluster);
600+
// in standard mode it executes on apl.db (the source/applier host).
600601
func (apl *Applier) createTargetTableFromStatement(targetTableName, createStatement string) error {
601602
targetDatabase := apl.migrationContext.GetTargetDatabaseName()
602603
apl.migrationContext.Log.Infof("Creating target table %s.%s",
603604
sql.EscapeName(targetDatabase),
604605
sql.EscapeName(targetTableName),
605606
)
606607

608+
db := apl.db
609+
if apl.migrationContext.IsMoveTablesMode() {
610+
db = apl.moveTablesTargetDB
611+
}
612+
607613
err := func() error {
608-
tx, err := apl.db.Begin()
614+
tx, err := db.Begin()
609615
if err != nil {
610616
return err
611617
}
@@ -640,12 +646,35 @@ func (apl *Applier) CreateGhostTable() error {
640646
return apl.createTargetTable(apl.migrationContext.GetGhostTableName())
641647
}
642648

643-
// CreateTargetTable creates the target table on the target host (for move-tables)
649+
// CreateTargetTable creates the target table on the target host (for move-tables).
650+
// It aborts with an error if the target table already exists on the target cluster,
651+
// to prevent silently writing into a table that has unrelated data or a different
652+
// schema (move_table_mode.md §1.3: "Don't use IF NOT EXISTS for the target table.
653+
// An existing table is an error condition, not a no-op.").
644654
func (apl *Applier) CreateTargetTable(createStatement string) error {
645655
if !apl.migrationContext.IsMoveTablesMode() {
646656
return errors.New("CreateTargetTable is only available in MoveTables mode")
647657
}
648-
return apl.createTargetTableFromStatement(apl.originalTableName(), createStatement)
658+
targetTableName := apl.originalTableName()
659+
targetDatabase := apl.migrationContext.GetTargetDatabaseName()
660+
661+
// Explicit pre-check: abort before any data is copied if the target table
662+
// already exists. The CREATE TABLE would also fail (MySQL ERROR 1050), but
663+
// this gives operators a clear gh-ost error message explaining what to do.
664+
var count int
665+
err := apl.moveTablesTargetDB.QueryRow(
666+
"SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?",
667+
targetDatabase, targetTableName,
668+
).Scan(&count)
669+
if err != nil {
670+
return fmt.Errorf("failed to check for existing target table: %w", err)
671+
}
672+
if count > 0 {
673+
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",
674+
sql.EscapeName(targetDatabase), sql.EscapeName(targetTableName))
675+
}
676+
677+
return apl.createTargetTableFromStatement(targetTableName, createStatement)
649678
}
650679

651680
// AlterGhost applies `alter` statement on ghost table

‎go/logic/applier_test.go‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -770,6 +770,114 @@ func (suite *ApplierTestSuite) TestCreateGhostTable() {
770770
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)
771771
}
772772

773+
// TestCreateTargetTable_HappyPath exercises #8207 AC #1:
774+
// "A move table run targeting a clean target schema creates the migrated table
775+
// with a SHOW CREATE TABLE output equivalent to the source's."
776+
//
777+
// It calls CreateTargetTable (not just IsMoveTablesMode()), asserts the table
778+
// exists on the target database, verifies schema equivalence via SHOW CREATE TABLE,
779+
// and confirms no table was accidentally created on the source.
780+
func (suite *ApplierTestSuite) TestCreateTargetTable_HappyPath() {
781+
ctx := context.Background()
782+
783+
_, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT PRIMARY KEY, name VARCHAR(64), updated_at DATETIME);", getTestTableName()))
784+
suite.Require().NoError(err)
785+
786+
connectionConfig, err := getTestConnectionConfig(ctx, suite.mysqlContainer)
787+
suite.Require().NoError(err)
788+
789+
migrationContext := newTestMigrationContext()
790+
migrationContext.MoveTables.TableNames = []string{testMysqlTableName}
791+
migrationContext.MoveTables.TargetDatabase = testMysqlDatabaseOther
792+
migrationContext.ApplierConnectionConfig = connectionConfig
793+
migrationContext.MoveTables.ConnectionConfig = connectionConfig
794+
migrationContext.SetConnectionConfig("innodb")
795+
migrationContext.OriginalTableColumns = sql.NewColumnList([]string{"id", "name", "updated_at"})
796+
797+
applier := NewApplier(migrationContext)
798+
defer applier.Teardown()
799+
800+
err = applier.InitDBConnections()
801+
suite.Require().NoError(err)
802+
803+
var dummy, sourceCreateDDL string
804+
err = suite.db.QueryRow(fmt.Sprintf("SHOW CREATE TABLE %s", getTestTableName())).Scan(&dummy, &sourceCreateDDL)
805+
suite.Require().NoError(err)
806+
807+
var count int
808+
err = suite.otherDB.QueryRow(
809+
"SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?",
810+
testMysqlDatabaseOther, testMysqlTableName,
811+
).Scan(&count)
812+
suite.Require().NoError(err)
813+
suite.Require().Equal(0, count, "precondition: target table must not exist before CreateTargetTable")
814+
815+
err = applier.CreateTargetTable(sourceCreateDDL)
816+
suite.Require().NoError(err)
817+
818+
var targetTableName, targetCreateDDL string
819+
err = suite.otherDB.QueryRow(fmt.Sprintf("SHOW CREATE TABLE `%s`.`%s`", testMysqlDatabaseOther, testMysqlTableName)).Scan(&targetTableName, &targetCreateDDL)
820+
suite.Require().NoError(err)
821+
suite.Require().Equal(testMysqlTableName, targetTableName)
822+
suite.Require().Equal(sourceCreateDDL, targetCreateDDL, "target table schema must be equivalent to source")
823+
824+
err = suite.otherDB.QueryRow(
825+
"SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?",
826+
testMysqlDatabaseOther, testMysqlTableName,
827+
).Scan(&count)
828+
suite.Require().NoError(err)
829+
suite.Require().Equal(1, count, "target table must exist exactly once on the target database")
830+
}
831+
832+
// TestCreateTargetTable_AbortsIfExists exercises #8207 AC #2:
833+
// "A move table run aborts before any data is copied if the target table already exists."
834+
//
835+
// It pre-creates the target table, then calls CreateTargetTable and asserts it
836+
// returns a descriptive error (not just MySQL's raw ERROR 1050).
837+
func (suite *ApplierTestSuite) TestCreateTargetTable_AbortsIfExists() {
838+
ctx := context.Background()
839+
840+
_, err := suite.db.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s (id INT PRIMARY KEY);", getTestTableName()))
841+
suite.Require().NoError(err)
842+
843+
_, err = suite.otherDB.ExecContext(ctx, fmt.Sprintf("CREATE TABLE `%s`.`%s` (id INT PRIMARY KEY);", testMysqlDatabaseOther, testMysqlTableName))
844+
suite.Require().NoError(err)
845+
846+
var count int
847+
err = suite.otherDB.QueryRow(
848+
"SELECT COUNT(*) FROM information_schema.tables WHERE table_schema=? AND table_name=?",
849+
testMysqlDatabaseOther, testMysqlTableName,
850+
).Scan(&count)
851+
suite.Require().NoError(err)
852+
suite.Require().Equal(1, count, "precondition: target table must exist before CreateTargetTable")
853+
854+
connectionConfig, err := getTestConnectionConfig(ctx, suite.mysqlContainer)
855+
suite.Require().NoError(err)
856+
857+
migrationContext := newTestMigrationContext()
858+
migrationContext.MoveTables.TableNames = []string{testMysqlTableName}
859+
migrationContext.MoveTables.TargetDatabase = testMysqlDatabaseOther
860+
migrationContext.ApplierConnectionConfig = connectionConfig
861+
migrationContext.MoveTables.ConnectionConfig = connectionConfig
862+
migrationContext.SetConnectionConfig("innodb")
863+
migrationContext.OriginalTableColumns = sql.NewColumnList([]string{"id"})
864+
865+
applier := NewApplier(migrationContext)
866+
defer applier.Teardown()
867+
868+
err = applier.InitDBConnections()
869+
suite.Require().NoError(err)
870+
871+
var dummy, sourceCreateDDL string
872+
err = suite.db.QueryRow(fmt.Sprintf("SHOW CREATE TABLE %s", getTestTableName())).Scan(&dummy, &sourceCreateDDL)
873+
suite.Require().NoError(err)
874+
875+
err = applier.CreateTargetTable(sourceCreateDDL)
876+
suite.Require().Error(err, "CreateTargetTable must return an error when target table already exists")
877+
suite.Require().Contains(err.Error(), "already exists", "error message must mention 'already exists'")
878+
suite.Require().Contains(err.Error(), testMysqlTableName, "error message must name the table")
879+
}
880+
773881
func (suite *ApplierTestSuite) TestPanicOnWarningsInApplyIterationInsertQuerySucceedsWithUniqueKeyWarningInsertedByDMLEvent() {
774882
ctx := context.Background()
775883

0 commit comments

Comments
 (0)