Merge pull request #1747 from VandhanaSelvaprakash-at/vaselvap-upstream-analyze-cutover

Add opt-in --analyze-ghost-table-before-cutover

Author: ericyan

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`.

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

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

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 (apl *Applier) AnalyzeGhostTable() error {
+	query := fmt.Sprintf(`analyze /* gh-ost */ table %s.%s`,
+		sql.EscapeName(apl.migrationContext.DatabaseName),
+		sql.EscapeName(apl.migrationContext.GetGhostTableName()),
+	)
+	apl.migrationContext.Log.Infof("Running ANALYZE TABLE on ghost table %s.%s before cut-over",
+		sql.EscapeName(apl.migrationContext.DatabaseName),
+		sql.EscapeName(apl.migrationContext.GetGhostTableName()),
+	)
+	analyzeStartTime := time.Now()
+	var rows []analyzeTableResultRow
+	err := sqlutils.QueryRowsMap(apl.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(apl.migrationContext.GetGhostTableName()), err)
+	}
+	if err := classifyAnalyzeTableResult(apl.migrationContext.GetGhostTableName(), rows); err != nil {
+		return err
+	}
+	apl.migrationContext.Log.Infof("ANALYZE TABLE on ghost table %s.%s completed in %dms",
+		sql.EscapeName(apl.migrationContext.DatabaseName),
+		sql.EscapeName(apl.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`,

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

go/logic/migrator.go

@@ -854,6 +854,20 @@ func (mgtr *Migrator) handleCutOverResult(cutOverError error) (err error) {
 	return nil
 }
 
+// analyzeGhostTableBeforeCutOver runs the opt-in pre-cut-over ANALYZE TABLE via the
+// injected analyze operation, gated on --analyze-ghost-table-before-cutover. It
+// returns nil (proceed) when the flag is off, and otherwise returns whatever analyze
+// returns; the caller must treat a non-nil error as fatal and abort cut-over before
+// any source lock, replica stop, or cut-over retry. analyze is a parameter so the
+// gating and fail-closed contract is testable without a live applier (whose ANALYZE
+// requires a real MySQL) or the process-exiting Log.Fatale path.
+func (mgtr *Migrator) analyzeGhostTableBeforeCutOver(analyze func() error) error {
+	if !mgtr.migrationContext.AnalyzeGhostTableBeforeCutOver {
+		return nil
+	}
+	return analyze()
+}
+
 // cutOver performs the final step of migration, based on migration
 // type (on replica? atomic? safe?)
 func (mgtr *Migrator) cutOver() (err error) {
@@ -904,6 +918,18 @@ func (mgtr *Migrator) cutOver() (err error) {
 	mgtr.migrationContext.MarkPointOfInterest()
 	mgtr.migrationContext.Log.Debugf("checking for cut-over postpone: complete")
 
+	// 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. Gating and the injectable analyze op live in
+	// analyzeGhostTableBeforeCutOver so this ordering contract is unit-testable.
+	if err := mgtr.analyzeGhostTableBeforeCutOver(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

go/logic/migrator_test.go

@@ -496,6 +496,57 @@ func TestCutOverOperationWithMetricsAbort(t *testing.T) {
 	assert.Equal(t, [][]string{{"outcome:" + metrics.CutOverOutcomeAbort}}, spy.histogramTags)
 }
 
+// TestAnalyzeGhostTableBeforeCutOver covers the cut-over orchestration contract for
+// the opt-in pre-cut-over ANALYZE: the --analyze-ghost-table-before-cutover flag
+// gates the call, and a failed ANALYZE surfaces as an error so cutOver() aborts
+// (via Log.Fatale) before it reaches replica-stop or the cut-over locking/retry
+// switch. The applier's ANALYZE execution and result parsing are covered separately
+// by ApplierTestSuite.TestAnalyzeGhostTable against a real MySQL.
+func TestAnalyzeGhostTableBeforeCutOver(t *testing.T) {
+	t.Run("flag off: ANALYZE is not invoked, cut-over proceeds", func(t *testing.T) {
+		migrator := NewMigrator(base.NewMigrationContext(), "test")
+		migrator.migrationContext.AnalyzeGhostTableBeforeCutOver = false
+
+		invoked := false
+		err := migrator.analyzeGhostTableBeforeCutOver(func() error {
+			invoked = true
+			return nil
+		})
+
+		require.NoError(t, err)
+		assert.False(t, invoked, "ANALYZE must not run when the flag is off")
+	})
+
+	t.Run("flag on, ANALYZE succeeds: invoked once, cut-over proceeds", func(t *testing.T) {
+		migrator := NewMigrator(base.NewMigrationContext(), "test")
+		migrator.migrationContext.AnalyzeGhostTableBeforeCutOver = true
+
+		calls := 0
+		err := migrator.analyzeGhostTableBeforeCutOver(func() error {
+			calls++
+			return nil
+		})
+
+		require.NoError(t, err)
+		assert.Equal(t, 1, calls)
+	})
+
+	t.Run("flag on, ANALYZE fails: error propagates so cut-over aborts fail-closed", func(t *testing.T) {
+		migrator := NewMigrator(base.NewMigrationContext(), "test")
+		migrator.migrationContext.AnalyzeGhostTableBeforeCutOver = true
+
+		analyzeErr := errors.New("ANALYZE TABLE on ghost failed; refusing cut-over")
+		calls := 0
+		err := migrator.analyzeGhostTableBeforeCutOver(func() error {
+			calls++
+			return analyzeErr
+		})
+
+		require.ErrorIs(t, err, analyzeErr)
+		assert.Equal(t, 1, calls, "a failed ANALYZE must not be retried inside the seam")
+	})
+}
+
 func TestReportStatusEmitsProgressGaugesEveryTick(t *testing.T) {
 	spy := &progressGaugeSpy{}
 	ctx := base.NewMigrationContext()
@@ -647,7 +698,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))
 }

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

localtests/analyze-ghost-table-before-cutover/create.sql

@@ -0,0 +1,22 @@
+drop table if exists gh_ost_test;
+create table gh_ost_test (
+  id int auto_increment,
+  i int not null,
+  color varchar(32),
+  primary key(id)
+) auto_increment=1;
+
+drop event if exists gh_ost_test;
+delimiter ;;
+create event gh_ost_test
+  on schedule every 1 second
+  starts current_timestamp
+  ends current_timestamp + interval 60 second
+  on completion not preserve
+  enable
+  do
+begin
+  insert into gh_ost_test values (null, 11, 'red');
+  insert into gh_ost_test values (null, 13, 'green');
+  update gh_ost_test set color = 'gray' where i = 11;
+end ;;

localtests/analyze-ghost-table-before-cutover/extra_args

@@ -0,0 +1 @@
+--analyze-ghost-table-before-cutover