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)) }