Add opt-in --analyze-ghost-table-before-cutover - #1747
Merged
ericyan merged 2 commits intoAug 7, 2026
Merged
Conversation
Add --analyze-ghost-table-before-cutover. When set, cutOver() runs an explicit ANALYZE TABLE on the ghost table after the postpone gate releases — before atomicCutOver() takes the source lock and before --test-on-replica stops replication — logs the elapsed milliseconds on success, and aborts the migration (fatal) if the ANALYZE fails, rather than swap in a table with stale InnoDB statistics. The abort exits synchronously (Log.Fatale), not via a retriable return — a plain return re-runs cutOver() and the ANALYZE up to --default-retries. Because ANALYZE TABLE reports table-level failures (missing table, storage-engine errors) as Msg_type Error rows in its result set while succeeding at the protocol level, the result rows are inspected and cut-over is refused unless ANALYZE reports status OK with no Error rows; privilege-style failures surface as statement errors on the same abort path. Without this, a freshly swapped table can briefly serve traffic with a near-zero row estimate, which the optimizer may cost as a free full scan on hot query paths, flipping plans until statistics are recomputed. Issue github#1418 / PR github#1419 propose an ANALYZE for the same reason; this variant corrects two defects there — the ANALYZE runs after the postpone gate (so a postponed cut-over still gets fresh statistics) and a failed ANALYZE aborts instead of being ignored. Opt-in, matching the maintainers' ask on github#1419 (ANALYZE cost grows with partition count, and the statement replicates). The result-row inspection is extracted as classifyAnalyzeTableResult, a pure, DB-free function, and covered by: - TestClassifyAnalyzeTableResult: a table test over status-OK, case folding, an error row (alone and alongside a status-OK row), a status-OK row followed by a later error row (rows are scanned fully, not short-circuited), a non-OK status, and an empty result. Each refusal asserts the underlying cause via ErrorContains. - ApplierTestSuite.TestAnalyzeGhostTable (real MySQL): happy path; the fail-open regression (dropping the ghost table makes ANALYZE return an Error row with no statement error, which the row inspection must refuse); and the statement-error branch (a closed connection is refused via the distinct error path). Also fixes a pre-existing suite bug surfaced while adding the test above: testify's suite runner calls TearDownSuite() (capital D), but the applier, migrator, and streamer suites all spelled it TeardownSuite(), so the method never matched the interface and the MySQL testcontainer was never terminated. Renamed in all three suites. Co-authored-by: wangzihuacool <wangzihuacool@163.com> Signed-off-by: Vandhana Selvaprakash <vandhana.selvaprakash@airtable.com>
VandhanaSelvaprakash-at
requested a review
from timvaillancourt
as a code owner
August 5, 2026 21:21
ericyan
reviewed
Aug 6, 2026
ericyan
left a comment
ericyan
left a comment
Contributor
There was a problem hiding this comment.
Hi @VandhanaSelvaprakash-at, thanks for the PR! This looks great overall. Just need a tiny change to make the linter happy.
Could we add a small, deterministic Migrator-level test for this as well?
The applier tests cover ANALYZE execution and result parsing well, but they do not verify the cut-over orchestration contract: that the flag gates the call, it runs only after postpone releases, and an analysis failure stops before replica-stop or cut-over locking/retry work begins. A narrow test seam around the analysis invocation is fine; no need to abstract the full Applier just for this.
Adding localtest for this case would also be nice, but that is not required.
- applier: name the AnalyzeGhostTable receiver `apl`, consistent with the rest of applier.go (staticcheck ST1016) — fixes the golangci-lint failure. - migrator: extract the pre-cut-over ANALYZE gating into analyzeGhostTableBeforeCutOver(analyze func() error), a narrow injectable seam (behavior unchanged), so the orchestration contract is unit-testable without a live applier or the process-exiting Log.Fatale path. - migrator test: TestAnalyzeGhostTableBeforeCutOver covers the flag gating, the happy path (ANALYZE runs once), and fail-closed (a failed ANALYZE propagates so cut-over aborts before replica-stop / cut-over locking). - localtest: analyze-ghost-table-before-cutover exercises the flag end-to-end against live MySQL with ongoing DML. Signed-off-by: Vandhana Selvaprakash <vandhana.selvaprakash@airtable.com>
Contributor
Author
Thanks @ericyan! All three addressed in the latest commit:
|
ericyan
approved these changes
Aug 7, 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
2 participants
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.
Supersedes #1419 (original approach and credit to @wangzihuacool) and closes the gap discussed in #1418. Thanks @ericyan / @timvaillancourt for the go-ahead.
What
Adds an opt-in
--analyze-ghost-table-before-cutoverflag (default off). When set, gh-ost runs an explicitANALYZE TABLEon the ghost table immediately before cut-over, so the freshly swapped table doesn't briefly serve traffic with near-zero InnoDB row estimates — which the optimizer can cost as a free full scan, flipping plans on hot paths until statistics recompute.Why opt-in
Per @shaohk and @timvaillancourt on #1419: on partitioned tables
ANALYZEcost grows with partition count and the statement replicates. So it's gated behind a flag, default off, intended for small non-partitioned tables — opt-in users can report on performance.How it differs from #1419
--test-on-replicastops replication) — a postponed cut-over doesn't re-stale its statistics before the swap.ANALYZE TABLEsurfaces table-level failures (missing table, storage-engine errors) asMsg_type=Errorresult rows while the statement succeeds at the protocol level, so the rows are inspected and cut-over is refused unless status is OK with no Error rows.Tests
TestClassifyAnalyzeTableResult— DB-free table test of the result-row classifier (status-OK, case folding, error rows, full-scan ordering, non-OK status, empty result).ApplierTestSuite.TestAnalyzeGhostTable— real-MySQL test covering the happy path, the fail-open regression, and the statement-error branch.TestAnalyzeGhostTableBeforeCutOver— migrator-level test of the cut-over orchestration contract: the flag gates the call, a successful ANALYZE runs exactly once, and a failed ANALYZE propagates socutOver()aborts (viaLog.Fatale) before replica-stop or cut-over locking begins. The gating lives in a narrowMigrator.analyzeGhostTableBeforeCutOver(analyze func() error)seam so the contract is testable without a live applier.localtests/analyze-ghost-table-before-cutover— end-to-end migration with the flag enabled and ongoing DML, exercising the real cut-over ANALYZE path against MySQL.Also folds in a separable pre-existing fix:
TeardownSuite→TearDownSuiteacross the applier/migrator/streamer suites (testify never invoked the misspelled method, leaking a testcontainer per suite). Happy to split it into its own PR if you'd prefer.DCO signed off.