feat(move-tables): fix target table creation to use target DB + abort if exists (#8207)#1710
Merged
zacharysierakowski merged 2 commits intoJun 12, 2026
Conversation
… if exists (#8207) 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
womoruyi
added
the
feature-move-tables
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes move-tables mode behavior so that target table creation runs against the target cluster connection (not the source/applier connection), and adds an explicit pre-check that aborts early if the target table already exists. Standard --alter migrations remain unaffected because CreateTargetTable is gated behind IsMoveTablesMode().
Changes:
- Update
createTargetTableFromStatementto usemoveTablesTargetDBin move-tables mode instead of always usingapl.db. - Add an
information_schema.tablesexistence pre-check inCreateTargetTablewith a descriptive operator-facing error message. - Add integration tests covering successful target table creation and the “abort if exists” behavior.
Show a summary per file
| File | Description |
|---|---|
| go/logic/applier.go | Selects the correct DB handle in move-tables mode and adds an explicit “abort if exists” pre-check before creating the target table. |
| go/logic/applier_test.go | Adds integration tests for move-tables target table creation and abort-on-exists behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
Comment on lines
+608
to
+612
| db := apl.db | ||
| if apl.migrationContext.IsMoveTablesMode() { | ||
| db = apl.moveTablesTargetDB | ||
| } | ||
|
|
Comment on lines
+661
to
+668
| // 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) |
Comment on lines
+875
to
+878
| 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") |
zacharysierakowski
approved these changes
Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Behavior change for move-tables mode: fixes
createTargetTableFromStatementto execute on the target cluster (not the source), and adds an explicit "abort if exists" pre-check before creating the target table. Standard--altermigrations are unaffected.What approach did you choose and why?
The prototype's
createTargetTableFromStatement(applier.go:600) usedapl.db.Begin()to open a transaction — butapl.dbis the source (applier) connection. In move-tables mode, the target table must be created onapl.moveTablesTargetDB(the target cluster). This PR fixes the DB handle selection:createTargetTableFromStatementnow checksIsMoveTablesMode()and usesmoveTablesTargetDBwhen true.For the "abort if exists" behavior, the design doc says "Don't use
IF NOT EXISTSfor the target table. An existing table is an error condition, not a no-op" (move_table_mode.md §1.3). Rather than relying on MySQL's implicitERROR 1050,CreateTargetTablenow pre-checksinformation_schema.tableson the target and returns a descriptive gh-ost error message telling the operator what to do. This matches the existingValidateOrDropExistingTablespattern used for ghost tables (applier.go:483).Scope (#8207 only):
go/logic/applier.go:608-612—createTargetTableFromStatementnow selectsmoveTablesTargetDBin move-tables mode instead of always usingapl.db.go/logic/applier.go:643-673—CreateTargetTableadds an explicitinformation_schemapre-check on the target cluster before attemptingCREATE TABLE.go/logic/applier_test.go— Two new integration tests:TestCreateTargetTable_HappyPath(schema equivalence viaSHOW CREATE TABLE, both-side assertion) andTestCreateTargetTable_AbortsIfExists(pre-creates target, asserts descriptive error).Explicitly NOT in this PR (belongs to adjacent issues):
ApplyDMLEventQueriesmove-tables branch atapplier.go:2067) — #8175moveTablesCopySelectFirstQueryBuilderetc.) — #8175 / #8163moveTablesCutOver) — #8209moveTablesTargetDBinitialization block atapplier.go:163-179— pre-existing prototype duplication, not introduced or fixed hereWhich feature flags are involved in this change?
IsMoveTablesMode()predicate (#8167).Which environments does this change target?
script/build.Risk assessment
IsMoveTablesMode(). Standard--altermigrations never reachCreateTargetTable(it returns an error if called outside move-tables mode). ThecreateTargetTableFromStatementchange adds a conditional DB handle selection but preserves the originalapl.dbpath for non-move-tables callers.How did/will you validate this change?
go test -run 'TestApplier/TestCreateTargetTable' -v ./go/logic/...— both PASS.go build ./...clean.gofmt -l ./go/clean.go vet ./go/logic/...clean.go/logic/applier.goandgo/logic/applier_test.gomodified.Known pre-existing test failures (not introduced by this PR)
TestEventsStreamer— intermittent testcontainers startup flake (same root cause as #8206 / #8209 PRs).Are there related full stack changes?
If something goes wrong, what are the mitigation and rollback strategies?
feature-move-tables. The change is contained togo/logic/applier.go.--move-tables. Operators on standard--alterare unaffected.Reviewers: The key change is the DB handle fix at
applier.go:608-612— verify thatmoveTablesTargetDBis the correct connection for target table creation. The "abort if exists" pre-check atapplier.go:655-665is a design choice (explicit error vs implicit MySQL 1050); if you prefer the implicit approach, the pre-check block is self-contained and removable. DML routing (#8175) and cutover (#8209) are not in this PR by design.