From 2425722c227d61f0726f3546834a62e1f7a44cfa Mon Sep 17 00:00:00 2001 From: Vandhana Selvaprakash Date: Wed, 5 Aug 2026 14:11:29 -0700 Subject: [PATCH] Add opt-in ANALYZE TABLE on the ghost table before cut-over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #1418 / PR #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 #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 Signed-off-by: Vandhana Selvaprakash --- doc/command-line-flags.md | 4 ++ go/base/context.go | 6 +++ go/cmd/gh-ost/main.go | 1 + go/logic/applier.go | 72 +++++++++++++++++++++++++ go/logic/applier_test.go | 111 +++++++++++++++++++++++++++++++++++++- go/logic/migrator.go | 13 +++++ go/logic/migrator_test.go | 2 +- go/logic/streamer_test.go | 2 +- 8 files changed, 208 insertions(+), 3 deletions(-) diff --git a/doc/command-line-flags.md b/doc/command-line-flags.md index f2938dc52..ff781b250 100644 --- a/doc/command-line-flags.md +++ b/doc/command-line-flags.md @@ -6,6 +6,10 @@ A more in-depth discussion of various `gh-ost` command line flags: implementatio Add this flag when executing on Aliyun RDS. +### analyze-ghost-table-before-cutover + +Run an explicit `ANALYZE TABLE` on the ghost table immediately before cut-over — after a postponed cut-over is released, before the atomic swap takes its locks — and abort the migration if the `ANALYZE` fails, rather than swap in a table with stale InnoDB statistics. Without it, the 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. This is the same rationale as issue #1418 / PR #1419; this flag is a corrected variant: the `ANALYZE` runs after the postpone gate releases (so a postponed cut-over still gets fresh statistics) and a failed `ANALYZE` aborts the migration instead of being ignored. Opt-in; intended for small, non-partitioned tables that are non-empty at copy (`ANALYZE TABLE` cost grows with partition count, and its statement replicates to replicas). + ### allow-zero-in-date Allows the user to make schema changes that include a zero date or zero in date (e.g. adding a `datetime default '0000-00-00 00:00:00'` column), even if global `sql_mode` on MySQL has `NO_ZERO_IN_DATE,NO_ZERO_DATE`. diff --git a/go/base/context.go b/go/base/context.go index 4d0c3b09d..b2ec8ff1e 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -267,6 +267,12 @@ type MigrationContext struct { TriggerSuffix string Triggers []mysql.Trigger + // AnalyzeGhostTableBeforeCutOver makes cutOver() run ANALYZE TABLE on the ghost table + // immediately before the atomic swap, and abort the migration if the ANALYZE errors, + // rather than swap in a table with stale statistics. Opt-in: the operator enables it + // only for eligible tables — small, non-partitioned, non-empty at copy. + AnalyzeGhostTableBeforeCutOver bool + recentBinlogCoordinates mysql.BinlogCoordinates BinlogSyncerMaxReconnectAttempts int diff --git a/go/cmd/gh-ost/main.go b/go/cmd/gh-ost/main.go index d77046231..cd1f5993f 100644 --- a/go/cmd/gh-ost/main.go +++ b/go/cmd/gh-ost/main.go @@ -167,6 +167,7 @@ func main() { flag.BoolVar(&migrationContext.Resume, "resume", false, "Attempt to resume migration from checkpoint") flag.BoolVar(&migrationContext.Revert, "revert", false, "Attempt to revert completed migration") flag.StringVar(&migrationContext.OldTableName, "old-table", "", "The name of the old table when using --revert, e.g. '_mytable_del'") + flag.BoolVar(&migrationContext.AnalyzeGhostTableBeforeCutOver, "analyze-ghost-table-before-cutover", false, "Run ANALYZE TABLE on the ghost table immediately before cut-over; abort the migration (fatal) if the ANALYZE fails, rather than swapping in a table with stale statistics. Opt-in; intended for small, non-partitioned tables that are non-empty at copy. Default false") maxLoad := flag.String("max-load", "", "Comma delimited status-name=threshold. e.g: 'Threads_running=100,Threads_connected=500'. When status exceeds threshold, app throttles writes") criticalLoad := flag.String("critical-load", "", "Comma delimited status-name=threshold, same format as --max-load. When status exceeds threshold, app panics and quits") diff --git a/go/logic/applier.go b/go/logic/applier.go index b1559a678..8f9067796 100644 --- a/go/logic/applier.go +++ b/go/logic/applier.go @@ -526,6 +526,78 @@ func (apl *Applier) CreateGhostTable() error { return err } +// analyzeTableResultRow is the subset of an `ANALYZE TABLE` result-set row that gh-ost inspects +// to decide whether the analyze succeeded. +type analyzeTableResultRow struct { + msgType string + msgText string +} + +// classifyAnalyzeTableResult decides whether an `ANALYZE TABLE` succeeded from its result rows. +// ANALYZE TABLE reports table-level failures (missing table, storage-engine errors) as +// Msg_type "Error" rows while still succeeding at the protocol level, so the statement error +// alone cannot be trusted — the rows must be inspected. Cut-over is refused unless the result +// carries a status-OK row and no error row (fail-closed: an empty or status-less result also +// refuses). tableName is used only to build the error message. +func classifyAnalyzeTableResult(tableName string, rows []analyzeTableResultRow) error { + sawStatusOk := false + var resultErrors []string + for _, row := range rows { + msgType := strings.ToLower(row.msgType) + if msgType == "error" { + resultErrors = append(resultErrors, row.msgText) + } + if msgType == "status" && strings.EqualFold(row.msgText, "OK") { + sawStatusOk = true + } + } + if len(resultErrors) > 0 || !sawStatusOk { + return fmt.Errorf("ANALYZE TABLE on ghost %s did not report status OK; refusing cut-over: %s", sql.EscapeName(tableName), strings.Join(resultErrors, "; ")) + } + return nil +} + +// AnalyzeGhostTable runs an explicit ANALYZE TABLE on the ghost table, forcing a +// synchronous InnoDB persistent-statistics recompute before cut-over. Without it the +// freshly swapped table can serve traffic with a near-zero row estimate, which the +// optimizer costs as a free full scan — the failure mode motivating upstream #1419. +// No row-count assertion follows the ANALYZE: on a freshly built, compact ghost a +// successful ANALYZE yields correct statistics by construction, and a row count +// cannot prove plan safety — plan checks belong to the orchestrating layer, which +// knows the table's context. The caller must treat a returned error as fatal, +// not retriable. +func (this *Applier) AnalyzeGhostTable() error { + query := fmt.Sprintf(`analyze /* gh-ost */ table %s.%s`, + sql.EscapeName(this.migrationContext.DatabaseName), + sql.EscapeName(this.migrationContext.GetGhostTableName()), + ) + this.migrationContext.Log.Infof("Running ANALYZE TABLE on ghost table %s.%s before cut-over", + sql.EscapeName(this.migrationContext.DatabaseName), + sql.EscapeName(this.migrationContext.GetGhostTableName()), + ) + analyzeStartTime := time.Now() + var rows []analyzeTableResultRow + err := sqlutils.QueryRowsMap(this.db, query, func(rowMap sqlutils.RowMap) error { + rows = append(rows, analyzeTableResultRow{ + msgType: rowMap.GetString("Msg_type"), + msgText: rowMap.GetString("Msg_text"), + }) + return nil + }) + if err != nil { + return fmt.Errorf("ANALYZE TABLE on ghost %s failed; refusing cut-over: %w", sql.EscapeName(this.migrationContext.GetGhostTableName()), err) + } + if err := classifyAnalyzeTableResult(this.migrationContext.GetGhostTableName(), rows); err != nil { + return err + } + this.migrationContext.Log.Infof("ANALYZE TABLE on ghost table %s.%s completed in %dms", + sql.EscapeName(this.migrationContext.DatabaseName), + sql.EscapeName(this.migrationContext.GetGhostTableName()), + time.Since(analyzeStartTime).Milliseconds(), + ) + return nil +} + // AlterGhost applies `alter` statement on ghost table func (apl *Applier) AlterGhost() error { query := fmt.Sprintf(`alter /* gh-ost */ table %s.%s %s`, diff --git a/go/logic/applier_test.go b/go/logic/applier_test.go index 85a5a01d3..f1fa28bc8 100644 --- a/go/logic/applier_test.go +++ b/go/logic/applier_test.go @@ -266,6 +266,73 @@ func TestRetryOnLockWaitTimeout(t *testing.T) { }) } +func TestClassifyAnalyzeTableResult(t *testing.T) { + tests := []struct { + name string + rows []analyzeTableResultRow + // errContains is the substring the refusal error must carry; empty means expect success. + // Asserting the substring proves which branch refused and that the underlying cause + // propagates to the operator, rather than accepting any error. + errContains string + }{ + { + name: "status OK passes", + rows: []analyzeTableResultRow{{msgType: "status", msgText: "OK"}}, + }, + { + // gh-ost lowercases Msg_type and folds Msg_text, so a differently-cased OK still passes. + name: "status OK is matched case-insensitively", + rows: []analyzeTableResultRow{{msgType: "Status", msgText: "ok"}}, + }, + { + // The fail-open the PR fixes: MySQL reports a table-level failure as an Error row while + // the statement succeeds at the protocol level. An error row must refuse cut-over. + name: "error row refuses cut-over", + rows: []analyzeTableResultRow{{msgType: "Error", msgText: "Table 'test._testing_gho' doesn't exist"}}, + errContains: "doesn't exist", + }, + { + // An error row must refuse even when a status-OK row is also present — this is the case + // that exercises the error-row clause independently of the missing-status-OK clause. + name: "error row refuses even alongside status OK", + rows: []analyzeTableResultRow{ + {msgType: "Error", msgText: "Incorrect key file for table"}, + {msgType: "status", msgText: "OK"}, + }, + errContains: "Incorrect key file", + }, + { + // All rows are scanned: a status-OK row must not short-circuit a later error row. + name: "status OK before a later error row still refuses", + rows: []analyzeTableResultRow{ + {msgType: "status", msgText: "OK"}, + {msgType: "Error", msgText: "late corruption error"}, + }, + errContains: "late corruption error", + }, + { + name: "status row that is not OK refuses cut-over", + rows: []analyzeTableResultRow{{msgType: "status", msgText: "Operation failed"}}, + errContains: "did not report status OK", + }, + { + name: "empty result refuses cut-over (fail-closed)", + rows: nil, + errContains: "did not report status OK", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := classifyAnalyzeTableResult("_testing_gho", tc.rows) + if tc.errContains == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tc.errContains) + } + }) + } +} + type ApplierTestSuite struct { suite.Suite @@ -295,7 +362,7 @@ func (suite *ApplierTestSuite) SetupSuite() { suite.db = db } -func (suite *ApplierTestSuite) TeardownSuite() { +func (suite *ApplierTestSuite) TearDownSuite() { suite.Assert().NoError(suite.db.Close()) suite.Assert().NoError(testcontainers.TerminateContainer(suite.mysqlContainer)) } @@ -627,6 +694,48 @@ 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) } +func (suite *ApplierTestSuite) TestAnalyzeGhostTable() { + ctx := context.Background() + + _, err := suite.db.ExecContext(ctx, "CREATE TABLE test.testing (id INT, item_id INT);") + suite.Require().NoError(err) + + connectionConfig, err := getTestConnectionConfig(ctx, suite.mysqlContainer) + suite.Require().NoError(err) + + migrationContext := base.NewMigrationContext() + migrationContext.ApplierConnectionConfig = connectionConfig + migrationContext.DatabaseName = "test" + migrationContext.SkipPortValidation = true + migrationContext.OriginalTableName = "testing" + migrationContext.SetConnectionConfig("innodb") + migrationContext.InitiallyDropGhostTable = true + + applier := NewApplier(migrationContext) + defer applier.Teardown() + + suite.Require().NoError(applier.InitDBConnections()) + suite.Require().NoError(applier.CreateGhostTable()) + + // Happy path: ANALYZE on the freshly-created ghost table succeeds. + suite.Require().NoError(applier.AnalyzeGhostTable()) + + // Fail-closed regression: if the ghost table is gone at cut-over time, MySQL reports the missing + // table as a Msg_type=Error result row while the statement itself succeeds at the protocol + // level. A naive statement-error check would fail open and swap in a broken table; the + // row-inspection guard must refuse instead. ErrorContains pins the refusal to that guard rather + // than to any incidental error. + _, err = suite.db.ExecContext(ctx, "DROP TABLE test._testing_gho") + suite.Require().NoError(err) + suite.Require().ErrorContains(applier.AnalyzeGhostTable(), "did not report status OK") + + // Statement-error path: a failure at the protocol level (here, a closed connection) rather than + // a result row is refused through the distinct statement-error branch. This closes the applier's + // connections, so the deferred Teardown above becomes a harmless second close. + applier.Teardown() + suite.Require().ErrorContains(applier.AnalyzeGhostTable(), "failed; refusing cut-over") +} + func (suite *ApplierTestSuite) TestPanicOnWarningsInApplyIterationInsertQuerySucceedsWithUniqueKeyWarningInsertedByDMLEvent() { ctx := context.Background() diff --git a/go/logic/migrator.go b/go/logic/migrator.go index 017a0c302..70fa46b66 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -904,6 +904,19 @@ func (mgtr *Migrator) cutOver() (err error) { mgtr.migrationContext.MarkPointOfInterest() mgtr.migrationContext.Log.Debugf("checking for cut-over postpone: complete") + if mgtr.migrationContext.AnalyzeGhostTableBeforeCutOver { + // Force a synchronous ANALYZE on the ghost table here — after the postpone gate + // releases, before atomicCutOver() takes the source write lock, and before + // --test-on-replica stops replication (so a failure cannot strand a stopped + // replica). A failure must be fatal, not retried: a plain `return err` re-runs + // cutOver() — and the ANALYZE — up to --default-retries, and a PanicAbort send + // races the retrier. Log.Fatale exits synchronously without ever locking the + // source. + if err := mgtr.applier.AnalyzeGhostTable(); err != nil { + return mgtr.migrationContext.Log.Fatale(err) + } + } + if mgtr.migrationContext.TestOnReplica { // With `--test-on-replica` we stop replication thread, and then proceed to use // the same cut-over phase as the master would use. That means we take locks diff --git a/go/logic/migrator_test.go b/go/logic/migrator_test.go index f4d458235..d860dc56d 100644 --- a/go/logic/migrator_test.go +++ b/go/logic/migrator_test.go @@ -647,7 +647,7 @@ func (suite *MigratorTestSuite) SetupSuite() { suite.db = db } -func (suite *MigratorTestSuite) TeardownSuite() { +func (suite *MigratorTestSuite) TearDownSuite() { suite.Assert().NoError(suite.db.Close()) suite.Assert().NoError(testcontainers.TerminateContainer(suite.mysqlContainer)) } diff --git a/go/logic/streamer_test.go b/go/logic/streamer_test.go index b887eb098..5b28c47c2 100644 --- a/go/logic/streamer_test.go +++ b/go/logic/streamer_test.go @@ -42,7 +42,7 @@ func (suite *EventsStreamerTestSuite) SetupSuite() { suite.db = db } -func (suite *EventsStreamerTestSuite) TeardownSuite() { +func (suite *EventsStreamerTestSuite) TearDownSuite() { suite.Assert().NoError(suite.db.Close()) suite.Assert().NoError(testcontainers.TerminateContainer(suite.mysqlContainer)) }