From bf1608d70ecda8dac4e90534e5b1ff2051c27a6f Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 27 Aug 2026 19:04:48 +0800 Subject: [PATCH 01/18] fix(frontend): publish analyzed stats by table generation --- pkg/frontend/compiler_context.go | 27 +- pkg/frontend/computation_wrapper.go | 16 +- pkg/frontend/mysql_cmd_executor.go | 92 ++++++- pkg/frontend/mysql_cmd_executor_test.go | 291 +++++++++++++++++++++ pkg/frontend/plan_cache.go | 34 +++ pkg/frontend/plan_cache_test.go | 41 +++ pkg/frontend/server.go | 114 +++++++- pkg/frontend/session.go | 108 +++++++- pkg/frontend/types.go | 6 + pkg/sql/plan/stats.go | 23 +- pkg/sql/plan/stats_test.go | 13 + pkg/vm/engine/disttae/engine.go | 19 ++ pkg/vm/engine/disttae/engine_stats_test.go | 94 +++++++ pkg/vm/engine/disttae/stats.go | 43 +++ pkg/vm/engine/types.go | 8 + 15 files changed, 914 insertions(+), 15 deletions(-) create mode 100644 pkg/vm/engine/disttae/engine_stats_test.go diff --git a/pkg/frontend/compiler_context.go b/pkg/frontend/compiler_context.go index 55c70200a86e7..fce8578be65f0 100644 --- a/pkg/frontend/compiler_context.go +++ b/pkg/frontend/compiler_context.go @@ -162,6 +162,21 @@ func (tcc *TxnCompilerContext) GetStatsCache() *plan2.StatsCache { return tcc.execCtx.ses.GetStatsCache() } +func (tcc *TxnCompilerContext) getStatsCacheVersion(tableID uint64) (*Session, *plan2.StatsCache, uint64) { + tcc.mu.Lock() + feSes := tcc.execCtx.ses + txnWrapper, _ := tcc.tcw.(*TxnComputationWrapper) + tcc.mu.Unlock() + if ses, ok := feSes.(*Session); ok { + cache, version := ses.getStatsCacheWithVersion(tableID) + if txnWrapper != nil { + txnWrapper.recordOptimizerStatsVersion(tableID, version) + } + return ses, cache, version + } + return nil, feSes.GetStatsCache(), 0 +} + func InitTxnCompilerContext(db string) *TxnCompilerContext { return &TxnCompilerContext{dbName: db} } @@ -1125,10 +1140,11 @@ func (tcc *TxnCompilerContext) Stats(obj *plan2.ObjectRef, snapshot *plan2.Snaps }() tableID := uint64(obj.Obj) + ses, statsCache, statsVersion := tcc.getStatsCacheVersion(tableID) // Fast path: return cached result if visited within 3 seconds AND stats is valid // Stats is valid if AccurateObjectNumber > 0 (meaning we have real data) - if w := tcc.GetStatsCache().Get(tableID); w.Exists() { + if w := statsCache.Get(tableID); w.Exists() { if time.Now().Unix()-w.GetLastVisit() < 3 { s := w.GetStats() if s != nil && s.AccurateObjectNumber > 0 { @@ -1144,8 +1160,13 @@ func (tcc *TxnCompilerContext) Stats(obj *plan2.ObjectRef, snapshot *plan2.Snaps return nil, err } - // Cache the result - tcc.GetStatsCache().Set(tableID, result) + // A refresh may have completed while the slow path was reading storage. Do + // not let work from the old generation repopulate the new session cache. + if ses == nil { + statsCache.Set(tableID, result) + } else { + ses.cacheStatsIfCurrent(tableID, statsVersion, result) + } return result, nil } diff --git a/pkg/frontend/computation_wrapper.go b/pkg/frontend/computation_wrapper.go index 89cc51d6ffce9..03ad7053f9fcc 100644 --- a/pkg/frontend/computation_wrapper.go +++ b/pkg/frontend/computation_wrapper.go @@ -108,7 +108,8 @@ type TxnComputationWrapper struct { // protocolVersion is captured when plan is built. The session plan cache // uses it instead of the version observed later when execution completes. - protocolVersion int64 + protocolVersion int64 + optimizerStatsVersions map[uint64]uint64 } func InitTxnComputationWrapper( @@ -205,9 +206,21 @@ func (cwft *TxnComputationWrapper) Clear() { cwft.preparedSchedulingSQLMode = "" cwft.hasPreparedSchedulingSQLMode = false cwft.preparedSchedulingSQL = "" + cwft.optimizerStatsVersions = nil cwft.schedulingTrace.Reset() } +func (cwft *TxnComputationWrapper) recordOptimizerStatsVersion(tableID, version uint64) { + if cwft.optimizerStatsVersions == nil { + cwft.optimizerStatsVersions = make(map[uint64]uint64) + } + // Keep the first observed version. If publication happens between repeated + // reads, admission against the newer current version will reject the plan. + if _, exists := cwft.optimizerStatsVersions[tableID]; !exists { + cwft.optimizerStatsVersions[tableID] = version + } +} + func (cwft *TxnComputationWrapper) ParamVals() []any { return cwft.paramVals } @@ -317,6 +330,7 @@ func (cwft *TxnComputationWrapper) Compile(any any, fill func(*batch.Batch, *per cacheHit := cwft.plan != nil if !cacheHit { cwft.protocolVersion = currentProtocolVersion(cwft.proc) + clear(cwft.optimizerStatsVersions) cwft.plan, err = buildPlanWithPrepareMode( execCtx.reqCtx, cwft.ses, diff --git a/pkg/frontend/mysql_cmd_executor.go b/pkg/frontend/mysql_cmd_executor.go index bdd2a8cdb8adf..abdac57cf3479 100644 --- a/pkg/frontend/mysql_cmd_executor.go +++ b/pkg/frontend/mysql_cmd_executor.go @@ -55,6 +55,7 @@ import ( "github.com/matrixorigin/matrixone/pkg/logutil" "github.com/matrixorigin/matrixone/pkg/pb/metadata" "github.com/matrixorigin/matrixone/pkg/pb/plan" + pbstats "github.com/matrixorigin/matrixone/pkg/pb/statsinfo" pbtxn "github.com/matrixorigin/matrixone/pkg/pb/txn" "github.com/matrixorigin/matrixone/pkg/perfcounter" "github.com/matrixorigin/matrixone/pkg/sql/colexec" @@ -2073,12 +2074,95 @@ func handleAnalyzeStmt(ses *Session, execCtx *ExecCtx, stmt *tree.AnalyzeStmt) e if err != nil { return err } + if err := refreshAnalyzeTableStats(ses, execCtx.reqCtx, entry); err != nil { + return err + } results = append(results, result) } execCtx.results = results return nil } +func refreshAnalyzeTableStats(ses *Session, ctx context.Context, entry *tree.AnalyzeTableEntry) error { + if entry == nil || entry.Table == nil || entry.Table.AtTsExpr != nil { + return nil + } + + refresher, ok := getPu(ses.GetService()).StorageEngine.(engine.StatsRefresher) + if !ok { + // Engines without persistent optimizer statistics retain the legacy + // ANALYZE result behavior. + return nil + } + + tcc := ses.GetTxnCompileCtx() + dbName := resolveAnalyzeDatabase(tcc, entry.Table) + if dbName == "" { + return moerr.NewNoDB(ctx) + } + obj, tableDef, err := tcc.Resolve(dbName, string(entry.Table.Name()), nil) + if err != nil { + return err + } + if obj == nil || tableDef == nil { + return moerr.NewNoSuchTable(ctx, dbName, string(entry.Table.Name())) + } + // Historical snapshots, temporary tables, and publication-backed tables do + // not own the current local engine statistics generation. + if tableDef.IsTemporary || obj.PubInfo != nil { + return nil + } + + accountID, err := defines.GetAccountId(ctx) + if err != nil { + return err + } + databaseID := tableDef.DbId + if databaseID == 0 { + databaseID, err = tcc.GetDatabaseId(obj.SchemaName, nil) + if err != nil { + return err + } + } + key := pbstats.StatsInfoKey{ + AccId: accountID, + DatabaseID: databaseID, + TableID: uint64(obj.Obj), + DbName: obj.SchemaName, + TableName: obj.ObjName, + } + return publishAnalyzeTableStats(ses, ctx, key, refresher) +} + +func publishAnalyzeTableStats( + ses *Session, + ctx context.Context, + key pbstats.StatsInfoKey, + refresher engine.StatsRefresher, +) error { + tableKey := optimizerStatsTableKey{accountID: key.AccId, tableID: key.TableID} + release, err := acquireOptimizerStatsPublisher(ctx, ses.GetService(), tableKey) + if err != nil { + return err + } + defer release() + + stats, err := refresher.RefreshTableStats(ctx, key) + if err != nil { + return err + } + if stats == nil { + return moerr.NewInternalErrorf(ctx, "ANALYZE TABLE did not publish statistics for %s.%s", key.DbName, key.TableName) + } + + // The engine cache swap above is the data publication boundary. Advancing + // this table's version invalidates only dependent session entries; unrelated + // table statistics and plans remain reusable. + version := advanceOptimizerStatsVersion(ses.GetService(), tableKey) + ses.cachePublishedStats(key.TableID, version, stats) + return nil +} + func inheritAnalyzeRewriteHint(outerSQL, derivedSQL string) string { content, ok := leadingHintContent(outerSQL) if !ok || !strings.HasPrefix(strings.TrimSpace(content), "{") { @@ -5708,6 +5792,7 @@ func doComQuery(ses *Session, execCtx *ExecCtx, input *UserInput) (retErr error) } // end of for cacheProtocolVersion := currentProtocolVersion(proc) + cacheStatsVersions := make(map[uint64]uint64) if canCache && !ses.isCached(input.getHash()) { for _, cw := range cws { tcw, ok := cw.(*TxnComputationWrapper) @@ -5715,6 +5800,10 @@ func doComQuery(ses *Session, execCtx *ExecCtx, input *UserInput) (retErr error) canCache = false break } + if !mergeOptimizerStatsVersions(cacheStatsVersions, tcw.optimizerStatsVersions) { + canCache = false + break + } } } if canCache && !ses.isCached(input.getHash()) { @@ -5730,7 +5819,8 @@ func doComQuery(ses *Session, execCtx *ExecCtx, input *UserInput) (retErr error) cw.Clear() } Cached = true - ses.cachePlan(input.getHash(), stmts, plans, cacheProtocolVersion) + ses.cachePlanWithStatsVersions( + input.getHash(), stmts, plans, cacheStatsVersions, cacheProtocolVersion) } return nil diff --git a/pkg/frontend/mysql_cmd_executor_test.go b/pkg/frontend/mysql_cmd_executor_test.go index ab8c77780337b..e9b802fbd86e9 100644 --- a/pkg/frontend/mysql_cmd_executor_test.go +++ b/pkg/frontend/mysql_cmd_executor_test.go @@ -50,6 +50,7 @@ import ( "github.com/matrixorigin/matrixone/pkg/geo" "github.com/matrixorigin/matrixone/pkg/pb/metadata" plan0 "github.com/matrixorigin/matrixone/pkg/pb/plan" + pbstats "github.com/matrixorigin/matrixone/pkg/pb/statsinfo" "github.com/matrixorigin/matrixone/pkg/pb/timestamp" "github.com/matrixorigin/matrixone/pkg/pb/txn" "github.com/matrixorigin/matrixone/pkg/perfcounter" @@ -4779,6 +4780,296 @@ func TestHandleAnalyzeStmtRestoresOuterExecCtxOnError(t *testing.T) { require.Same(t, outerExecCtx, ses.GetTxnCompileCtx().execCtx) } +type analyzeStatsRefresherFunc func(context.Context, pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) + +func (f analyzeStatsRefresherFunc) RefreshTableStats( + ctx context.Context, key pbstats.StatsInfoKey, +) (*pbstats.StatsInfo, error) { + return f(ctx, key) +} + +func isolateOptimizerStatsTest(t *testing.T, sessions ...*Session) { + t.Helper() + service := "optimizer-stats-" + t.Name() + InitServerLevelVars(service) + previous := make([]string, len(sessions)) + for i, ses := range sessions { + previous[i] = ses.GetService() + ses.feSessionImpl.service = service + } + t.Cleanup(func() { + for i, ses := range sessions { + ses.feSessionImpl.service = previous[i] + } + serverVarsMap.Delete(service) + }) +} + +func optimizerStatsTestPlan(tableIDs ...uint64) *plan0.Plan { + nodes := make([]*plan0.Node, 0, len(tableIDs)) + for _, tableID := range tableIDs { + nodes = append(nodes, &plan0.Node{TableDef: &plan0.TableDef{TblId: tableID}}) + } + return &plan0.Plan{Plan: &plan0.Plan_Query{Query: &plan0.Query{Nodes: nodes}}} +} + +func optimizerStatsVersionsForTest(ses *Session, tableIDs ...uint64) map[uint64]uint64 { + versions := make(map[uint64]uint64, len(tableIDs)) + for _, tableID := range tableIDs { + versions[tableID] = currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) + } + return versions +} + +func cacheOptimizerPlanForTest(ses *Session, sql string, tableIDs ...uint64) { + ses.cachePlanWithStatsVersions( + sql, + []tree.Statement{&tree.Select{}}, + []*plan0.Plan{optimizerStatsTestPlan(tableIDs...)}, + optimizerStatsVersionsForTest(ses, tableIDs...), + ) +} + +func cacheOptimizerStatsForTest(t *testing.T, ses *Session, tableID uint64, stats *pbstats.StatsInfo) uint64 { + t.Helper() + version := currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) + require.True(t, ses.cacheStatsIfCurrent(tableID, version, stats)) + return version +} + +func TestCompilerContextRecordsTheStatsVersionActuallyRead(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ses, execCtx := newAnalyzeHandlerTestSession(t, ctrl) + isolateOptimizerStatsTest(t, ses) + + const tableID = uint64(42) + wrapper := InitTxnComputationWrapper(ses, &tree.Select{}, execCtx.proc) + tcc := ses.GetTxnCompileCtx() + tcc.SetExecCtx(execCtx) + tcc.tcw = wrapper + + _, firstVersion := ses.getStatsCacheWithVersion(tableID) + require.NotContains(t, ses.statsCacheVersions, tableID, + "a failed or uncached stats read must not grow session version metadata") + _, _, recordedVersion := tcc.getStatsCacheVersion(tableID) + require.Equal(t, firstVersion, recordedVersion) + require.Equal(t, firstVersion, wrapper.optimizerStatsVersions[tableID]) + + advanceOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) + _, _, currentVersion := tcc.getStatsCacheVersion(tableID) + require.NotEqual(t, firstVersion, currentVersion) + require.Equal(t, firstVersion, wrapper.optimizerStatsVersions[tableID], + "a plan that read both sides of publication must retain its stale dependency and be rejected") +} + +func TestOptimizerStatsVersionsCompactWithoutRevalidatingOldEntries(t *testing.T) { + vars := &ServerLevelVariables{ + optimizerStatsVersions: make(map[optimizerStatsTableKey]uint64), + } + first := optimizerStatsTableKey{accountID: 1, tableID: 10} + second := optimizerStatsTableKey{accountID: 1, tableID: 20} + third := optimizerStatsTableKey{accountID: 1, tableID: 30} + + firstVersion := advanceOptimizerStatsVersionLocked(vars, first, 2) + secondVersion := advanceOptimizerStatsVersionLocked(vars, second, 2) + require.Equal(t, uint64(1), firstVersion) + require.Equal(t, uint64(2), secondVersion) + + thirdVersion := advanceOptimizerStatsVersionLocked(vars, third, 2) + require.Equal(t, uint64(4), thirdVersion) + require.Len(t, vars.optimizerStatsVersions, 1) + require.Equal(t, uint64(3), currentOptimizerStatsVersionLocked(vars, first)) + require.Equal(t, uint64(3), currentOptimizerStatsVersionLocked(vars, second)) + require.NotEqual(t, firstVersion, currentOptimizerStatsVersionLocked(vars, first)) + require.NotEqual(t, secondVersion, currentOptimizerStatsVersionLocked(vars, second)) +} + +func TestPublishAnalyzeTableStatsDefinesCacheBoundary(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ses, execCtx := newAnalyzeHandlerTestSession(t, ctrl) + otherSes, _ := newAnalyzeHandlerTestSession(t, ctrl) + otherTenantSes, _ := newAnalyzeHandlerTestSession(t, ctrl) + isolateOptimizerStatsTest(t, ses, otherSes, otherTenantSes) + otherTenantSes.SetAccountId(7) + + const ( + tableID = uint64(42) + otherTableID = uint64(84) + ) + key := pbstats.StatsInfoKey{ + AccId: catalog.System_Account, + DatabaseID: 7, + TableID: tableID, + DbName: "db", + TableName: "events", + } + oldStats := plan.NewStatsInfo() + oldStats.NdvMap["url"] = 1 + otherStats := plan.NewStatsInfo() + otherStats.NdvMap["id"] = 84 + oldVersion := cacheOptimizerStatsForTest(t, otherSes, tableID, oldStats) + cacheOptimizerStatsForTest(t, otherSes, otherTableID, otherStats) + cacheOptimizerStatsForTest(t, ses, tableID, oldStats) + cacheOptimizerStatsForTest(t, otherTenantSes, tableID, oldStats) + + dependentPlan := optimizerStatsTestPlan(tableID) + compileVersions := optimizerStatsVersionsForTest(otherSes, tableID) + cacheOptimizerPlanForTest(otherSes, "select url from events", tableID) + cacheOptimizerPlanForTest(otherSes, "select id from other_table", otherTableID) + cacheOptimizerPlanForTest(otherTenantSes, "select url from tenant_events", tableID) + + freshStats := plan.NewStatsInfo() + freshStats.AccurateObjectNumber = 8 + freshStats.NdvMap["url"] = 1_000_000 + var gotKey pbstats.StatsInfoKey + refresher := analyzeStatsRefresherFunc(func(_ context.Context, key pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) { + gotKey = key + return freshStats, nil + }) + + require.NoError(t, publishAnalyzeTableStats(ses, execCtx.reqCtx, key, refresher)) + require.Equal(t, key, gotKey) + cache, _ := ses.getStatsCacheWithVersion(tableID) + wrapper := cache.Get(tableID) + require.Same(t, freshStats, wrapper.GetStats()) + otherSes.cachePlanWithStatsVersions("compiled before analyze completed", + []tree.Statement{&tree.Select{}}, []*plan0.Plan{dependentPlan}, compileVersions) + require.Nil(t, otherSes.getCachedPlan("compiled before analyze completed"), + "a plan compiled across the publication boundary must not enter the cache") + require.Nil(t, otherSes.getCachedPlan("select url from events")) + require.NotNil(t, otherSes.getCachedPlan("select id from other_table"), + "an unrelated table publication must not flush the session plan cache") + require.NotNil(t, otherTenantSes.getCachedPlan("select url from tenant_events"), + "the same table ID in another account must keep its plan") + require.False(t, otherSes.cacheStatsIfCurrent(tableID, oldVersion, oldStats), + "a stats read started before publication must not repopulate the table entry") + otherCache, currentVersion := otherSes.getStatsCacheWithVersion(tableID) + otherWrapper := otherCache.Get(tableID) + require.False(t, otherWrapper.Exists()) + otherTableWrapper := otherCache.Get(otherTableID) + require.Same(t, otherStats, otherTableWrapper.GetStats(), + "invalidating one table must retain unrelated statistics") + require.True(t, otherSes.cacheStatsIfCurrent(tableID, currentVersion, freshStats)) + currentWrapper := otherCache.Get(tableID) + require.Same(t, freshStats, currentWrapper.GetStats()) +} + +func TestPublishAnalyzeTableStatsDoesNotExposeFailedRefresh(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ses, execCtx := newAnalyzeHandlerTestSession(t, ctrl) + isolateOptimizerStatsTest(t, ses) + + const tableID = uint64(42) + key := pbstats.StatsInfoKey{TableID: tableID, DbName: "db", TableName: "events"} + oldStats := plan.NewStatsInfo() + cacheOptimizerStatsForTest(t, ses, tableID, oldStats) + cacheOptimizerPlanForTest(ses, "select url from events", tableID) + wantErr := moerr.NewInternalError(execCtx.reqCtx, "refresh failed") + refresher := analyzeStatsRefresherFunc(func(context.Context, pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) { + return nil, wantErr + }) + + err := publishAnalyzeTableStats(ses, execCtx.reqCtx, key, refresher) + require.ErrorIs(t, err, wantErr) + cache, _ := ses.getStatsCacheWithVersion(tableID) + wrapper := cache.Get(tableID) + require.Same(t, oldStats, wrapper.GetStats()) + require.NotNil(t, ses.getCachedPlan("select url from events")) +} + +func TestPublishAnalyzeTableStatsRejectsMissingRefreshResult(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ses, execCtx := newAnalyzeHandlerTestSession(t, ctrl) + isolateOptimizerStatsTest(t, ses) + + const tableID = uint64(42) + key := pbstats.StatsInfoKey{TableID: tableID, DbName: "db", TableName: "events"} + oldStats := plan.NewStatsInfo() + cacheOptimizerStatsForTest(t, ses, tableID, oldStats) + cacheOptimizerPlanForTest(ses, "select url from events", tableID) + version := currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) + clock := currentOptimizerStatsClock(ses.GetService()) + refresher := analyzeStatsRefresherFunc(func(context.Context, pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) { + return nil, nil + }) + + err := publishAnalyzeTableStats(ses, execCtx.reqCtx, key, refresher) + require.Error(t, err) + require.Equal(t, version, + currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID))) + require.Equal(t, clock, currentOptimizerStatsClock(ses.GetService())) + cache, _ := ses.getStatsCacheWithVersion(tableID) + wrapper := cache.Get(tableID) + require.Same(t, oldStats, wrapper.GetStats()) + require.NotNil(t, ses.getCachedPlan("select url from events")) +} + +func TestPublishAnalyzeTableStatsSerializesAndCancelsAdmission(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + firstSes, firstExecCtx := newAnalyzeHandlerTestSession(t, ctrl) + secondSes, secondExecCtx := newAnalyzeHandlerTestSession(t, ctrl) + isolateOptimizerStatsTest(t, firstSes, secondSes) + + key := pbstats.StatsInfoKey{ + AccId: catalog.System_Account, TableID: 42, DbName: "db", TableName: "events", + } + entered := make(chan struct{}) + unblock := make(chan struct{}, 1) + releaseFirst := func() { + select { + case unblock <- struct{}{}: + default: + } + } + t.Cleanup(releaseFirst) + firstDone := make(chan error, 1) + go func() { + firstDone <- publishAnalyzeTableStats(firstSes, firstExecCtx.reqCtx, key, + analyzeStatsRefresherFunc(func(context.Context, pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) { + close(entered) + <-unblock + return plan.NewStatsInfo(), nil + })) + }() + <-entered + + // A publication for a different table must not queue behind this table. + otherKey := key + otherKey.TableID = 43 + otherKey.TableName = "other_events" + var otherCalled atomic.Bool + require.NoError(t, publishAnalyzeTableStats(secondSes, secondExecCtx.reqCtx, otherKey, + analyzeStatsRefresherFunc(func(context.Context, pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) { + otherCalled.Store(true) + return plan.NewStatsInfo(), nil + }))) + require.True(t, otherCalled.Load()) + + secondCtx, cancel := context.WithCancel(secondExecCtx.reqCtx) + cancel() + var secondCalled atomic.Bool + err := publishAnalyzeTableStats(secondSes, secondCtx, key, + analyzeStatsRefresherFunc(func(context.Context, pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) { + secondCalled.Store(true) + return plan.NewStatsInfo(), nil + })) + require.ErrorIs(t, err, context.Canceled) + require.False(t, secondCalled.Load()) + + releaseFirst() + select { + case err = <-firstDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("first statistics publication did not finish") + } +} + func TestSetExecCtxClearsPreviousStatementViews(t *testing.T) { tcc := &TxnCompilerContext{} tcc.SetViews([]string{"db#stale_view"}) diff --git a/pkg/frontend/plan_cache.go b/pkg/frontend/plan_cache.go index 94c9be50934e7..e90d16f17ef65 100644 --- a/pkg/frontend/plan_cache.go +++ b/pkg/frontend/plan_cache.go @@ -26,6 +26,7 @@ type cachedPlan struct { stmts []tree.Statement plans []*plan.Plan protocolVersion int64 + statsVersions map[uint64]uint64 } // planCache uses LRU to cache plan for the same sql @@ -52,6 +53,16 @@ func freeStmts(stmts []tree.Statement) { } func (pc *planCache) cache(sql string, stmts []tree.Statement, plans []*plan.Plan, versions ...int64) { + pc.cacheWithStatsVersions(sql, stmts, plans, nil, versions...) +} + +func (pc *planCache) cacheWithStatsVersions( + sql string, + stmts []tree.Statement, + plans []*plan.Plan, + statsVersions map[uint64]uint64, + versions ...int64, +) { protocolVersion := currentProtocolVersion(nil) if len(versions) > 0 { protocolVersion = versions[0] @@ -74,6 +85,7 @@ func (pc *planCache) cache(sql string, stmts []tree.Statement, plans []*plan.Pla stmts: stmts, plans: plans, protocolVersion: protocolVersion, + statsVersions: cloneStatsVersions(statsVersions), } pc.lruList.MoveToFront(element) return @@ -83,6 +95,7 @@ func (pc *planCache) cache(sql string, stmts []tree.Statement, plans []*plan.Pla stmts: stmts, plans: plans, protocolVersion: protocolVersion, + statsVersions: cloneStatsVersions(statsVersions), }) pc.cachePool[sql] = element if pc.lruList.Len() > pc.capacity { @@ -93,6 +106,27 @@ func (pc *planCache) cache(sql string, stmts []tree.Statement, plans []*plan.Pla } } +func cloneStatsVersions(versions map[uint64]uint64) map[uint64]uint64 { + if len(versions) == 0 { + return nil + } + cloned := make(map[uint64]uint64, len(versions)) + for tableID, version := range versions { + cloned[tableID] = version + } + return cloned +} + +func mergeOptimizerStatsVersions(dst, src map[uint64]uint64) bool { + for tableID, version := range src { + if prior, exists := dst[tableID]; exists && prior != version { + return false + } + dst[tableID] = version + } + return true +} + func (pc *planCache) remove(sql string) { if pc.cachePool == nil { return diff --git a/pkg/frontend/plan_cache_test.go b/pkg/frontend/plan_cache_test.go index 0996776887740..ee3f80e896f26 100644 --- a/pkg/frontend/plan_cache_test.go +++ b/pkg/frontend/plan_cache_test.go @@ -278,6 +278,47 @@ func Test_SessionAccessorsWithNilPlanCache(t *testing.T) { require.NotPanics(t, func() { ses.releasePlanCache() }) } +func TestMergeOptimizerStatsVersionsRejectsMixedGenerations(t *testing.T) { + versions := map[uint64]uint64{1: 10} + require.True(t, mergeOptimizerStatsVersions(versions, map[uint64]uint64{1: 10, 2: 20})) + require.Equal(t, map[uint64]uint64{1: 10, 2: 20}, versions) + require.False(t, mergeOptimizerStatsVersions(versions, map[uint64]uint64{1: 11})) + require.Equal(t, uint64(10), versions[1]) +} + +var optimizerStatsVersionsCurrentSink bool + +func BenchmarkOptimizerStatsVersionsCurrent(b *testing.B) { + const ( + service = "optimizer-stats-version-benchmark" + accountID = uint32(7) + ) + InitServerLevelVars(service) + b.Cleanup(func() { serverVarsMap.Delete(service) }) + + for _, tc := range []struct { + name string + dependency int + }{ + {name: "no-dependency", dependency: 0}, + {name: "one-table", dependency: 1}, + {name: "four-tables", dependency: 4}, + {name: "sixteen-tables", dependency: 16}, + } { + b.Run(tc.name, func(b *testing.B) { + versions := make(map[uint64]uint64, tc.dependency) + for tableID := 1; tableID <= tc.dependency; tableID++ { + versions[uint64(tableID)] = 0 + } + b.ReportAllocs() + for b.Loop() { + optimizerStatsVersionsCurrentSink = optimizerStatsVersionsCurrent( + service, accountID, versions) + } + }) + } +} + func TestSessionRemoveCachedPlanOnlyEvictsTarget(t *testing.T) { ses := &Session{planCache: newPlanCache(2)} first := &trackedStatement{} diff --git a/pkg/frontend/server.go b/pkg/frontend/server.go index 529537e5cfb22..f81aefa2d440c 100644 --- a/pkg/frontend/server.go +++ b/pkg/frontend/server.go @@ -551,6 +551,16 @@ func nextConnectionID() uint32 { var serverVarsMap sync.Map +const ( + optimizerStatsPublisherStripes = 64 + optimizerStatsVersionEntries = 64 * 1024 +) + +type optimizerStatsTableKey struct { + accountID uint32 + tableID uint64 +} + func init() { InitServerLevelVars("") } @@ -565,10 +575,25 @@ func getServerLevelVars(service string) *ServerLevelVariables { } func InitServerLevelVars(service string) { - serverVarsMap.LoadOrStore(service, &ServerLevelVariables{}) + vars := &ServerLevelVariables{ + optimizerStatsVersions: make(map[optimizerStatsTableKey]uint64), + } + for i := range vars.optimizerStatsPublish { + vars.optimizerStatsPublish[i] = make(chan struct{}, 1) + } + serverVarsMap.LoadOrStore(service, vars) getServerLevelVars(service) } +func getOptimizerStatsVars(service string) *ServerLevelVariables { + vars := getServerLevelVars(service) + if vars == nil { + InitServerLevelVars(service) + vars = getServerLevelVars(service) + } + return vars +} + func getSessionAlloc(service string) Allocator { return getServerLevelVars(service).sessionAlloc.Load().(Allocator) } @@ -631,6 +656,93 @@ func getPu(service string) *config.ParameterUnit { return pu } +func currentOptimizerStatsClock(service string) uint64 { + vars := getOptimizerStatsVars(service) + vars.optimizerStatsMu.RLock() + defer vars.optimizerStatsMu.RUnlock() + return vars.optimizerStatsClock +} + +func currentOptimizerStatsVersion(service string, key optimizerStatsTableKey) uint64 { + vars := getOptimizerStatsVars(service) + vars.optimizerStatsMu.RLock() + defer vars.optimizerStatsMu.RUnlock() + return currentOptimizerStatsVersionLocked(vars, key) +} + +func advanceOptimizerStatsVersion(service string, key optimizerStatsTableKey) uint64 { + vars := getOptimizerStatsVars(service) + vars.optimizerStatsMu.Lock() + defer vars.optimizerStatsMu.Unlock() + return advanceOptimizerStatsVersionLocked(vars, key, optimizerStatsVersionEntries) +} + +func currentOptimizerStatsVersionLocked(vars *ServerLevelVariables, key optimizerStatsTableKey) uint64 { + if version, ok := vars.optimizerStatsVersions[key]; ok { + return version + } + return vars.optimizerStatsReset +} + +func advanceOptimizerStatsVersionLocked( + vars *ServerLevelVariables, + key optimizerStatsTableKey, + maxEntries int, +) uint64 { + if _, exists := vars.optimizerStatsVersions[key]; !exists && len(vars.optimizerStatsVersions) >= maxEntries { + // Explicit ANALYZE of many short-lived tables must not grow process + // metadata forever. A rare compaction advances the missing-key token, + // making every older cache label conservatively stale before reuse. + vars.optimizerStatsClock++ + vars.optimizerStatsReset = vars.optimizerStatsClock + clear(vars.optimizerStatsVersions) + } + vars.optimizerStatsClock++ + vars.optimizerStatsVersions[key] = vars.optimizerStatsClock + return vars.optimizerStatsClock +} + +func optimizerStatsVersionsCurrent( + service string, + accountID uint32, + versions map[uint64]uint64, +) bool { + if len(versions) == 0 { + return true + } + vars := getOptimizerStatsVars(service) + vars.optimizerStatsMu.RLock() + defer vars.optimizerStatsMu.RUnlock() + for tableID, version := range versions { + if currentOptimizerStatsVersionLocked(vars, optimizerStatsTableKey{ + accountID: accountID, + tableID: tableID, + }) != version { + return false + } + } + return true +} + +func optimizerStatsPublisherStripe(key optimizerStatsTableKey) int { + mixed := key.tableID ^ uint64(key.accountID)*0x9e3779b97f4a7c15 + return int(mixed % optimizerStatsPublisherStripes) +} + +func acquireOptimizerStatsPublisher( + ctx context.Context, + service string, + key optimizerStatsTableKey, +) (func(), error) { + admission := getOptimizerStatsVars(service).optimizerStatsPublish[optimizerStatsPublisherStripe(key)] + select { + case admission <- struct{}{}: + return func() { <-admission }, nil + case <-ctx.Done(): + return nil, context.Cause(ctx) + } +} + func setAicm(service string, aicm *defines.AutoIncrCacheManager) { getServerLevelVars(service).Aicm.Store(aicm) } diff --git a/pkg/frontend/session.go b/pkg/frontend/session.go index c613958f5690a..be04d5b68dde2 100644 --- a/pkg/frontend/session.go +++ b/pkg/frontend/session.go @@ -44,6 +44,7 @@ import ( "github.com/matrixorigin/matrixone/pkg/logutil" "github.com/matrixorigin/matrixone/pkg/pb/plan" "github.com/matrixorigin/matrixone/pkg/pb/query" + pbstats "github.com/matrixorigin/matrixone/pkg/pb/statsinfo" "github.com/matrixorigin/matrixone/pkg/pb/status" "github.com/matrixorigin/matrixone/pkg/perfcounter" "github.com/matrixorigin/matrixone/pkg/sql/parsers/dialect/mysql" @@ -284,8 +285,10 @@ type Session struct { planCache *planCache - statsCache *plan2.StatsCache - seqCurValues map[uint64]string + statsCacheMu sync.Mutex + statsCache *plan2.StatsCache + statsCacheVersions map[uint64]uint64 + seqCurValues map[uint64]string /* CORNER CASE: @@ -797,9 +800,72 @@ func (ses *Session) GetProc() *process.Process { } func (ses *Session) GetStatsCache() *plan2.StatsCache { + ses.statsCacheMu.Lock() + defer ses.statsCacheMu.Unlock() return ses.statsCache } +func (ses *Session) optimizerStatsKey(tableID uint64) optimizerStatsTableKey { + return optimizerStatsTableKey{ + accountID: ses.GetAccountId(), + tableID: tableID, + } +} + +func (ses *Session) getStatsCacheWithVersion(tableID uint64) (*plan2.StatsCache, uint64) { + ses.statsCacheMu.Lock() + defer ses.statsCacheMu.Unlock() + ses.initStatsCacheLocked() + version := currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) + wrapper := ses.statsCache.Get(tableID) + cachedVersion, tagged := ses.statsCacheVersions[tableID] + if !wrapper.Exists() { + delete(ses.statsCacheVersions, tableID) + } else if !tagged && version == 0 { + // Accept caches created before version tracking only in the initial + // generation. Once any publication has happened, an untagged entry is + // conservatively stale. + ses.statsCacheVersions[tableID] = version + } else if !tagged || cachedVersion != version { + ses.statsCache.Delete(tableID) + delete(ses.statsCacheVersions, tableID) + } + return ses.statsCache, version +} + +func (ses *Session) cacheStatsIfCurrent(tableID, version uint64, stats *pbstats.StatsInfo) bool { + ses.statsCacheMu.Lock() + defer ses.statsCacheMu.Unlock() + if currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) != version { + return false + } + ses.initStatsCacheLocked() + if ses.statsCache.SetAndReportReset(tableID, stats) { + clear(ses.statsCacheVersions) + } + ses.statsCacheVersions[tableID] = version + return true +} + +func (ses *Session) cachePublishedStats(tableID, version uint64, stats *pbstats.StatsInfo) { + ses.statsCacheMu.Lock() + defer ses.statsCacheMu.Unlock() + ses.initStatsCacheLocked() + if ses.statsCache.SetAndReportReset(tableID, stats) { + clear(ses.statsCacheVersions) + } + ses.statsCacheVersions[tableID] = version +} + +func (ses *Session) initStatsCacheLocked() { + if ses.statsCache == nil { + ses.statsCache = plan2.NewStatsCache() + } + if ses.statsCacheVersions == nil { + ses.statsCacheVersions = make(map[uint64]uint64) + } +} + func (ses *Session) GetSessionStart() time.Time { ses.mu.Lock() defer ses.mu.Unlock() @@ -1161,7 +1227,6 @@ func NewSession( var txnOp TxnOperator var err error txnHandler := InitTxnHandler(service, getPu(service).StorageEngine, connCtx, txnOp) - ses := &Session{ feSessionImpl: feSessionImpl{ pool: mp, @@ -1184,8 +1249,9 @@ func NewSession( startedAt: time.Now(), connType: ConnTypeUnset, - timestampMap: map[TS]time.Time{}, - statsCache: plan2.NewStatsCache(), + timestampMap: map[TS]time.Time{}, + statsCache: plan2.NewStatsCache(), + statsCacheVersions: make(map[uint64]uint64), } atomic.StoreInt32(&ses.sqlModeNoAutoValueOnZero, -1) @@ -1428,9 +1494,25 @@ func (ses *Session) IsBackgroundSession() bool { } func (ses *Session) cachePlan(sql string, stmts []tree.Statement, plans []*plan.Plan, versions ...int64) { + ses.cachePlanWithStatsVersions(sql, stmts, plans, nil, versions...) +} + +func (ses *Session) cachePlanWithStatsVersions( + sql string, + stmts []tree.Statement, + plans []*plan.Plan, + statsVersions map[uint64]uint64, + versions ...int64, +) { if len(sql) == 0 { return } + if !optimizerStatsVersionsCurrent(ses.GetService(), ses.GetAccountId(), statsVersions) { + // The plan crossed a statistics publication boundary while compiling. + // It may execute, but must not enter the cache with stale dependencies. + freeStmts(stmts) + return + } ses.mu.Lock() defer ses.mu.Unlock() if ses.planCache == nil { @@ -1441,7 +1523,7 @@ func (ses *Session) cachePlan(sql string, stmts []tree.Statement, plans []*plan. if len(versions) > 0 { protocolVersion = versions[0] } - ses.planCache.cache(sql, stmts, plans, protocolVersion) + ses.planCache.cacheWithStatsVersions(sql, stmts, plans, statsVersions, protocolVersion) } func (ses *Session) getCachedPlan(sql string) *cachedPlan { @@ -1454,7 +1536,8 @@ func (ses *Session) getCachedPlan(sql string) *cachedPlan { return nil } cached := ses.planCache.get(sql) - if cached != nil && cached.protocolVersion != currentProtocolVersion(ses.proc) { + if cached != nil && (cached.protocolVersion != currentProtocolVersion(ses.proc) || + !optimizerStatsVersionsCurrent(ses.GetService(), ses.GetAccountId(), cached.statsVersions)) { ses.planCache.remove(sql) return nil } @@ -1470,7 +1553,16 @@ func (ses *Session) isCached(sql string) bool { if ses.planCache == nil { return false } - return ses.planCache.isCached(sql) + cached := ses.planCache.get(sql) + if cached == nil { + return false + } + if cached.protocolVersion != currentProtocolVersion(ses.proc) || + !optimizerStatsVersionsCurrent(ses.GetService(), ses.GetAccountId(), cached.statsVersions) { + ses.planCache.remove(sql) + return false + } + return true } func (ses *Session) removeCachedPlan(sql string) { diff --git a/pkg/frontend/types.go b/pkg/frontend/types.go index 98df735b4f7a2..127f6ef96aafa 100644 --- a/pkg/frontend/types.go +++ b/pkg/frontend/types.go @@ -2003,4 +2003,10 @@ type ServerLevelVariables struct { Aicm atomic.Value moServerStarted atomic.Bool sessionAlloc atomic.Value + + optimizerStatsMu sync.RWMutex + optimizerStatsClock uint64 + optimizerStatsReset uint64 + optimizerStatsVersions map[optimizerStatsTableKey]uint64 + optimizerStatsPublish [optimizerStatsPublisherStripes]chan struct{} } diff --git a/pkg/sql/plan/stats.go b/pkg/sql/plan/stats.go index 9f679bf01426e..08974c66e0c53 100644 --- a/pkg/sql/plan/stats.go +++ b/pkg/sql/plan/stats.go @@ -149,17 +149,38 @@ func (sc *StatsCache) Get(tableID uint64) StatsInfoWrapper { // Set caches the stats result for the table. func (sc *StatsCache) Set(tableID uint64, stats *pb.StatsInfo) { + sc.set(tableID, stats) +} + +// SetAndReportReset caches stats and reports whether the bounded cache evicted +// all prior entries. Callers that keep side metadata can reset it in lockstep. +func (sc *StatsCache) SetAndReportReset(tableID uint64, stats *pb.StatsInfo) bool { + return sc.set(tableID, stats) +} + +func (sc *StatsCache) set(tableID uint64, stats *pb.StatsInfo) bool { if sc == nil { - return + return false } + reset := false if len(sc.cache) > statsCacheMaxSize { sc.cache = make(map[uint64]StatsInfoWrapper, statsCacheInitSize) logutil.Infof("statscache entries more than %v in long session, release memory", statsCacheMaxSize) + reset = true } sc.cache[tableID] = StatsInfoWrapper{ stats: stats, lastVisit: time.Now().Unix(), } + return reset +} + +// Delete removes one table without disturbing unrelated session statistics. +func (sc *StatsCache) Delete(tableID uint64) { + if sc == nil { + return + } + delete(sc.cache, tableID) } func NewStatsInfo() *pb.StatsInfo { diff --git a/pkg/sql/plan/stats_test.go b/pkg/sql/plan/stats_test.go index 9a74cd62cf693..b3cce5ca3fa76 100644 --- a/pkg/sql/plan/stats_test.go +++ b/pkg/sql/plan/stats_test.go @@ -345,6 +345,19 @@ func newStatsTestBuilderWithNDV(colName string, ndv float64) *QueryBuilder { return builder } +func TestStatsCacheReportsWholeCacheReset(t *testing.T) { + statsCache := NewStatsCache() + stats := NewStatsInfo() + for tableID := uint64(0); tableID <= statsCacheMaxSize; tableID++ { + require.False(t, statsCache.SetAndReportReset(tableID, stats)) + } + require.True(t, statsCache.SetAndReportReset(statsCacheMaxSize+1, stats)) + removed := statsCache.Get(0) + retained := statsCache.Get(statsCacheMaxSize + 1) + require.False(t, removed.Exists()) + require.True(t, retained.Exists()) +} + type statsCacheCompilerContext struct { *MockCompilerContext statsCache *StatsCache diff --git a/pkg/vm/engine/disttae/engine.go b/pkg/vm/engine/disttae/engine.go index 07198d6d6f3bb..1f7630ae6dcc2 100644 --- a/pkg/vm/engine/disttae/engine.go +++ b/pkg/vm/engine/disttae/engine.go @@ -58,6 +58,7 @@ import ( ) var _ engine.Engine = new(Engine) +var _ engine.StatsRefresher = new(Engine) const ( workspaceRSSCacheFamilyEvictTimeout = 10 * time.Second @@ -1278,6 +1279,24 @@ func (e *Engine) Stats(ctx context.Context, key pb.StatsInfoKey, sync bool) *pb. return e.globalStats.Get(ctx, key, sync) } +// RefreshTableStats synchronously replaces the local optimizer statistics for +// key. The cache swap is the publication boundary observed by later plans. +func (e *Engine) RefreshTableStats(ctx context.Context, key pb.StatsInfoKey) (*pb.StatsInfo, error) { + return refreshTableStats(ctx, key, e.globalStats) +} + +type optimizerStatsStore interface { + RefreshWithMode(context.Context, pb.StatsInfoKey, string) error + Get(context.Context, pb.StatsInfoKey, bool) *pb.StatsInfo +} + +func refreshTableStats(ctx context.Context, key pb.StatsInfoKey, store optimizerStatsStore) (*pb.StatsInfo, error) { + if err := store.RefreshWithMode(ctx, key, "auto"); err != nil { + return nil, err + } + return store.Get(ctx, key, false), nil +} + // GetGlobalStats returns the GlobalStats instance func (e *Engine) GetGlobalStats() *GlobalStats { return e.globalStats diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go new file mode 100644 index 0000000000000..3bf85626be728 --- /dev/null +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -0,0 +1,94 @@ +// Copyright 2026 Matrix Origin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package disttae + +import ( + "context" + "errors" + "testing" + + pb "github.com/matrixorigin/matrixone/pkg/pb/statsinfo" + "github.com/stretchr/testify/require" +) + +type optimizerStatsStoreStub struct { + stats *pb.StatsInfo + refreshErr error + key pb.StatsInfoKey + mode string + getCalled bool + getSync bool +} + +func (s *optimizerStatsStoreStub) RefreshWithMode(_ context.Context, key pb.StatsInfoKey, mode string) error { + s.key = key + s.mode = mode + return s.refreshErr +} + +func (s *optimizerStatsStoreStub) Get(_ context.Context, key pb.StatsInfoKey, sync bool) *pb.StatsInfo { + s.getCalled = true + s.key = key + s.getSync = sync + return s.stats +} + +func TestRefreshTableStatsDefinesPublicationBoundary(t *testing.T) { + key := pb.StatsInfoKey{TableID: 42, DbName: "db", TableName: "events"} + t.Run("success", func(t *testing.T) { + fresh := &pb.StatsInfo{TableCnt: 1_000_000} + store := &optimizerStatsStoreStub{stats: fresh} + + got, err := refreshTableStats(context.Background(), key, store) + require.NoError(t, err) + require.Same(t, fresh, got) + require.Equal(t, key, store.key) + require.Equal(t, "auto", store.mode) + require.True(t, store.getCalled) + require.False(t, store.getSync) + }) + + t.Run("refresh failure is not published", func(t *testing.T) { + wantErr := errors.New("refresh failed") + store := &optimizerStatsStoreStub{refreshErr: wantErr} + + got, err := refreshTableStats(context.Background(), key, store) + require.ErrorIs(t, err, wantErr) + require.Nil(t, got) + require.False(t, store.getCalled) + }) +} + +func TestOptimizerStatsRefreshAdmissionIsTableScopedAndCancelable(t *testing.T) { + gs := &GlobalStats{} + gs.initStatsRefreshAdmission() + key := pb.StatsInfoKey{AccId: 1, TableID: 42} + + release, err := gs.acquireStatsRefresh(context.Background(), key) + require.NoError(t, err) + defer release() + + otherKey := key + otherKey.TableID++ + releaseOther, err := gs.acquireStatsRefresh(context.Background(), otherKey) + require.NoError(t, err, "unrelated tables must not queue behind this table") + releaseOther() + + canceled, cancel := context.WithCancel(context.Background()) + cancel() + releaseCanceled, err := gs.acquireStatsRefresh(canceled, key) + require.ErrorIs(t, err, context.Canceled) + require.Nil(t, releaseCanceled) +} diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index 11162cd984ee1..5ed1fff265d1f 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -181,6 +181,8 @@ type GlobalStatsConfig struct { LogtailUpdateStatsThreshold int } +const optimizerStatsRefreshStripes = 64 + type GlobalStatsOption func(s *GlobalStats) // WithUpdateWorkerFactor set the update worker factor. @@ -233,6 +235,11 @@ type GlobalStats struct { updating map[pb.StatsInfoKey]*updateRecord } + // Explicit ANALYZE refreshes and automatic logtail refreshes share this + // bounded admission layer. The same table is calculated and published in + // order; unrelated tables normally remain parallel. + refreshAdmission [optimizerStatsRefreshStripes]chan struct{} + // statsInfoMap is the global stats info in engine which // contains all subscribed tables stats info. mu struct { @@ -279,6 +286,7 @@ func NewGlobalStats( s.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) s.mu.statsInfoMap = make(map[pb.StatsInfoKey]*pb.StatsInfo) s.mu.cond = sync.NewCond(&s.mu) + s.initStatsRefreshAdmission() for _, opt := range opts { opt(s) } @@ -316,6 +324,30 @@ func NewGlobalStats( return s } +func (gs *GlobalStats) initStatsRefreshAdmission() { + for i := range gs.refreshAdmission { + gs.refreshAdmission[i] = make(chan struct{}, 1) + } +} + +func optimizerStatsRefreshStripe(key pb.StatsInfoKey) int { + mixed := key.TableID ^ uint64(key.AccId)*0x9e3779b97f4a7c15 + return int(mixed % optimizerStatsRefreshStripes) +} + +func (gs *GlobalStats) acquireStatsRefresh( + ctx context.Context, + key pb.StatsInfoKey, +) (func(), error) { + admission := gs.refreshAdmission[optimizerStatsRefreshStripe(key)] + select { + case admission <- struct{}{}: + return func() { <-admission }, nil + case <-ctx.Done(): + return nil, context.Cause(ctx) + } +} + // keyExists returns true only if key already exists in the map. func (gs *GlobalStats) keyExists(key pb.StatsInfoKey) bool { gs.mu.Lock() @@ -886,6 +918,11 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) defer func() { gs.markUpdateComplete(wrapKey.Key, updated, actualObjectCount, samplingRatio) }() + release, err := gs.acquireStatsRefresh(wrapKey.Ctx, wrapKey.Key) + if err != nil { + return + } + defer release() broadcastWithoutUpdate := func() { gs.mu.Lock() @@ -947,6 +984,12 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) // RefreshWithMode triggers a stats refresh with the specified sampling mode func (gs *GlobalStats) RefreshWithMode(ctx context.Context, key pb.StatsInfoKey, samplingMode string) error { + release, err := gs.acquireStatsRefresh(ctx, key) + if err != nil { + return err + } + defer release() + // Get partition state ps, err := gs.engine.pClient.toSubscribeTable( ctx, diff --git a/pkg/vm/engine/types.go b/pkg/vm/engine/types.go index 10ec9f4a75dcf..9586a069f8591 100644 --- a/pkg/vm/engine/types.go +++ b/pkg/vm/engine/types.go @@ -1327,6 +1327,14 @@ type Engine interface { LatestLogtailAppliedTime() timestamp.Timestamp } +// StatsRefresher is an optional engine capability for statements that define +// a synchronous statistics-publication boundary, such as ANALYZE TABLE. +// Implementations must not return until Stats() can observe the returned +// statistics on the local engine instance. +type StatsRefresher interface { + RefreshTableStats(ctx context.Context, key pb.StatsInfoKey) (*pb.StatsInfo, error) +} + type VectorPool interface { PutBatch(bat *batch.Batch) GetVector(typ types.Type) *vector.Vector From b4e87b1391762abcb56af1e371346b17c9a7c82c Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 27 Aug 2026 20:44:37 +0800 Subject: [PATCH 02/18] fix(frontend): preserve physical stats ownership --- pkg/frontend/compiler_context.go | 50 +++++-- pkg/frontend/compiler_context_test.go | 9 +- pkg/frontend/computation_wrapper.go | 10 +- pkg/frontend/mysql_cmd_executor.go | 33 +++-- pkg/frontend/mysql_cmd_executor_test.go | 127 +++++++++++++++--- pkg/frontend/plan_cache.go | 20 +-- pkg/frontend/plan_cache_test.go | 24 ++-- pkg/frontend/server.go | 10 +- pkg/frontend/session.go | 59 ++++---- .../cases/analyze/analyze_stmt.result | 8 ++ .../cases/analyze/analyze_stmt.sql | 7 + 11 files changed, 263 insertions(+), 94 deletions(-) diff --git a/pkg/frontend/compiler_context.go b/pkg/frontend/compiler_context.go index fce8578be65f0..f817c459974c1 100644 --- a/pkg/frontend/compiler_context.go +++ b/pkg/frontend/compiler_context.go @@ -162,15 +162,17 @@ func (tcc *TxnCompilerContext) GetStatsCache() *plan2.StatsCache { return tcc.execCtx.ses.GetStatsCache() } -func (tcc *TxnCompilerContext) getStatsCacheVersion(tableID uint64) (*Session, *plan2.StatsCache, uint64) { +func (tcc *TxnCompilerContext) getStatsCacheVersion( + key optimizerStatsTableKey, +) (*Session, *plan2.StatsCache, uint64) { tcc.mu.Lock() feSes := tcc.execCtx.ses txnWrapper, _ := tcc.tcw.(*TxnComputationWrapper) tcc.mu.Unlock() if ses, ok := feSes.(*Session); ok { - cache, version := ses.getStatsCacheWithVersion(tableID) + cache, version := ses.getStatsCacheWithVersion(key) if txnWrapper != nil { - txnWrapper.recordOptimizerStatsVersion(tableID, version) + txnWrapper.recordOptimizerStatsVersion(key, version) } return ses, cache, version } @@ -274,25 +276,50 @@ func (tcc *TxnCompilerContext) ResolveViewDependencyAccount( tableDef *plan2.TableDef, snapshot *plan2.Snapshot, ) (uint32, error) { + return tcc.resolvePhysicalObjectAccount(obj, tableDef, snapshot), nil +} + +// resolvePhysicalObjectAccount keeps statistics and view dependencies aligned +// with the account context used by getRelation. The identity must be resolved +// before consulting any cache so cached data and its generation share one key. +func (tcc *TxnCompilerContext) resolvePhysicalObjectAccount( + obj *plan2.ObjectRef, + tableDef *plan2.TableDef, + snapshot *plan2.Snapshot, +) uint32 { accountID := tcc.execCtx.ses.GetAccountId() if snapshot != nil && snapshot.Tenant != nil { accountID = snapshot.Tenant.TenantID } - if obj.PubInfo != nil { + if obj != nil && obj.PubInfo != nil { accountID = uint32(obj.PubInfo.TenantId) } - dbName, tableName := obj.SchemaName, obj.ObjName - if dbName == "" { + var dbName, tableName string + if obj != nil { + dbName, tableName = obj.SchemaName, obj.ObjName + } + if dbName == "" && tableDef != nil { dbName = tableDef.DbName } - if tableName == "" { + if tableName == "" && tableDef != nil { tableName = tableDef.Name } - if isClusterTable(dbName, tableName) || ShouldSwitchToSysAccount(dbName, tableName) { + if (tableDef != nil && tableDef.TableType == catalog.SystemClusterRel) || + isClusterTable(dbName, tableName) || ShouldSwitchToSysAccount(dbName, tableName) { accountID = sysAccountID } - return accountID, nil + return accountID +} + +func (tcc *TxnCompilerContext) optimizerStatsKey( + obj *plan2.ObjectRef, + snapshot *plan2.Snapshot, +) optimizerStatsTableKey { + return optimizerStatsTableKey{ + accountID: tcc.resolvePhysicalObjectAccount(obj, nil, snapshot), + tableID: uint64(obj.Obj), + } } func (tcc *TxnCompilerContext) GetAccountName() string { @@ -1140,7 +1167,8 @@ func (tcc *TxnCompilerContext) Stats(obj *plan2.ObjectRef, snapshot *plan2.Snaps }() tableID := uint64(obj.Obj) - ses, statsCache, statsVersion := tcc.getStatsCacheVersion(tableID) + statsKey := tcc.optimizerStatsKey(obj, snapshot) + ses, statsCache, statsVersion := tcc.getStatsCacheVersion(statsKey) // Fast path: return cached result if visited within 3 seconds AND stats is valid // Stats is valid if AccurateObjectNumber > 0 (meaning we have real data) @@ -1165,7 +1193,7 @@ func (tcc *TxnCompilerContext) Stats(obj *plan2.ObjectRef, snapshot *plan2.Snaps if ses == nil { statsCache.Set(tableID, result) } else { - ses.cacheStatsIfCurrent(tableID, statsVersion, result) + ses.cacheStatsIfCurrent(statsKey, statsVersion, result) } return result, nil diff --git a/pkg/frontend/compiler_context_test.go b/pkg/frontend/compiler_context_test.go index 101b70b622347..5a608d3887e10 100644 --- a/pkg/frontend/compiler_context_test.go +++ b/pkg/frontend/compiler_context_test.go @@ -103,6 +103,7 @@ func TestResolveViewDependencyAccount(t *testing.T) { for _, test := range []struct { name string obj *pbplan.ObjectRef + tableDef *pbplan.TableDef snapshot *pbplan.Snapshot want uint32 }{ @@ -115,6 +116,8 @@ func TestResolveViewDependencyAccount(t *testing.T) { PubInfo: &pbplan.PubInfo{TenantId: 9}}, snapshot: &pbplan.Snapshot{Tenant: &pbplan.SnapshotTenant{TenantID: 8}}, want: 9}, {name: "cluster table", obj: &pbplan.ObjectRef{SchemaName: catalog.MO_CATALOG, ObjName: "cluster_table"}, want: 0}, + {name: "cluster relation kind", obj: &pbplan.ObjectRef{SchemaName: "db", ObjName: "cluster_table"}, + tableDef: &pbplan.TableDef{TableType: catalog.SystemClusterRel}, want: 0}, {name: "statement info", obj: &pbplan.ObjectRef{SchemaName: catalog.MO_SYSTEM, ObjName: catalog.MO_STATEMENT}, want: 0}, {name: "system relation overrides publisher", obj: &pbplan.ObjectRef{SchemaName: catalog.MO_SYSTEM, ObjName: catalog.MO_STATEMENT, PubInfo: &pbplan.PubInfo{TenantId: 9}}, want: 0}, @@ -122,7 +125,11 @@ func TestResolveViewDependencyAccount(t *testing.T) { {name: "sql statement cu", obj: &pbplan.ObjectRef{SchemaName: catalog.MO_SYSTEM_METRICS, ObjName: catalog.MO_SQL_STMT_CU}, want: 0}, } { t.Run(test.name, func(t *testing.T) { - got, err := tcc.ResolveViewDependencyAccount(test.obj, &pbplan.TableDef{}, test.snapshot) + tableDef := test.tableDef + if tableDef == nil { + tableDef = &pbplan.TableDef{} + } + got, err := tcc.ResolveViewDependencyAccount(test.obj, tableDef, test.snapshot) require.NoError(t, err) require.Equal(t, test.want, got) }) diff --git a/pkg/frontend/computation_wrapper.go b/pkg/frontend/computation_wrapper.go index 03ad7053f9fcc..8480114fc4244 100644 --- a/pkg/frontend/computation_wrapper.go +++ b/pkg/frontend/computation_wrapper.go @@ -109,7 +109,7 @@ type TxnComputationWrapper struct { // protocolVersion is captured when plan is built. The session plan cache // uses it instead of the version observed later when execution completes. protocolVersion int64 - optimizerStatsVersions map[uint64]uint64 + optimizerStatsVersions map[optimizerStatsTableKey]uint64 } func InitTxnComputationWrapper( @@ -210,14 +210,14 @@ func (cwft *TxnComputationWrapper) Clear() { cwft.schedulingTrace.Reset() } -func (cwft *TxnComputationWrapper) recordOptimizerStatsVersion(tableID, version uint64) { +func (cwft *TxnComputationWrapper) recordOptimizerStatsVersion(key optimizerStatsTableKey, version uint64) { if cwft.optimizerStatsVersions == nil { - cwft.optimizerStatsVersions = make(map[uint64]uint64) + cwft.optimizerStatsVersions = make(map[optimizerStatsTableKey]uint64) } // Keep the first observed version. If publication happens between repeated // reads, admission against the newer current version will reject the plan. - if _, exists := cwft.optimizerStatsVersions[tableID]; !exists { - cwft.optimizerStatsVersions[tableID] = version + if _, exists := cwft.optimizerStatsVersions[key]; !exists { + cwft.optimizerStatsVersions[key] = version } } diff --git a/pkg/frontend/mysql_cmd_executor.go b/pkg/frontend/mysql_cmd_executor.go index abdac57cf3479..0690c52cef048 100644 --- a/pkg/frontend/mysql_cmd_executor.go +++ b/pkg/frontend/mysql_cmd_executor.go @@ -2107,16 +2107,14 @@ func refreshAnalyzeTableStats(ses *Session, ctx context.Context, entry *tree.Ana if obj == nil || tableDef == nil { return moerr.NewNoSuchTable(ctx, dbName, string(entry.Table.Name())) } - // Historical snapshots, temporary tables, and publication-backed tables do - // not own the current local engine statistics generation. - if tableDef.IsTemporary || obj.PubInfo != nil { + // Historical snapshots and publication-backed tables do not own the current + // local engine statistics generation. Non-physical relations keep the + // legacy derived-query result without asking disttae to subscribe to them. + if obj.PubInfo != nil || !analyzeTableOwnsPersistentStats(tableDef) { return nil } - accountID, err := defines.GetAccountId(ctx) - if err != nil { - return err - } + accountID := tcc.resolvePhysicalObjectAccount(obj, tableDef, nil) databaseID := tableDef.DbId if databaseID == 0 { databaseID, err = tcc.GetDatabaseId(obj.SchemaName, nil) @@ -2134,6 +2132,23 @@ func refreshAnalyzeTableStats(ses *Session, ctx context.Context, entry *tree.Ana return publishAnalyzeTableStats(ses, ctx, key, refresher) } +func analyzeTableOwnsPersistentStats(tableDef *plan.TableDef) bool { + if tableDef == nil || tableDef.IsTemporary || tableDef.ViewSql != nil { + return false + } + switch tableDef.TableType { + case "", + catalog.SystemOrdinaryRel, + catalog.SystemIndexRel, + catalog.SystemMaterializedRel, + catalog.SystemClusterRel, + catalog.SystemPartitionRel: + return true + default: + return false + } +} + func publishAnalyzeTableStats( ses *Session, ctx context.Context, @@ -2159,7 +2174,7 @@ func publishAnalyzeTableStats( // this table's version invalidates only dependent session entries; unrelated // table statistics and plans remain reusable. version := advanceOptimizerStatsVersion(ses.GetService(), tableKey) - ses.cachePublishedStats(key.TableID, version, stats) + ses.cachePublishedStats(tableKey, version, stats) return nil } @@ -5792,7 +5807,7 @@ func doComQuery(ses *Session, execCtx *ExecCtx, input *UserInput) (retErr error) } // end of for cacheProtocolVersion := currentProtocolVersion(proc) - cacheStatsVersions := make(map[uint64]uint64) + cacheStatsVersions := make(map[optimizerStatsTableKey]uint64) if canCache && !ses.isCached(input.getHash()) { for _, cw := range cws { tcw, ok := cw.(*TxnComputationWrapper) diff --git a/pkg/frontend/mysql_cmd_executor_test.go b/pkg/frontend/mysql_cmd_executor_test.go index e9b802fbd86e9..1a5e14a380051 100644 --- a/pkg/frontend/mysql_cmd_executor_test.go +++ b/pkg/frontend/mysql_cmd_executor_test.go @@ -4788,6 +4788,39 @@ func (f analyzeStatsRefresherFunc) RefreshTableStats( return f(ctx, key) } +func TestAnalyzeTableOwnsPersistentStats(t *testing.T) { + for _, test := range []struct { + name string + tableDef *plan0.TableDef + want bool + }{ + {name: "missing definition"}, + {name: "ordinary", tableDef: &plan0.TableDef{TableType: catalog.SystemOrdinaryRel}, want: true}, + {name: "legacy physical kind", tableDef: &plan0.TableDef{}, want: true}, + {name: "index", tableDef: &plan0.TableDef{TableType: catalog.SystemIndexRel}, want: true}, + {name: "materialized", tableDef: &plan0.TableDef{TableType: catalog.SystemMaterializedRel}, want: true}, + {name: "cluster", tableDef: &plan0.TableDef{TableType: catalog.SystemClusterRel}, want: true}, + {name: "partition", tableDef: &plan0.TableDef{TableType: catalog.SystemPartitionRel}, want: true}, + {name: "view kind", tableDef: &plan0.TableDef{TableType: catalog.SystemViewRel}}, + {name: "view definition", tableDef: &plan0.TableDef{ + TableType: catalog.SystemOrdinaryRel, + ViewSql: &plan0.ViewDef{View: "select 1"}, + }}, + {name: "external", tableDef: &plan0.TableDef{TableType: catalog.SystemExternalRel}}, + {name: "sequence", tableDef: &plan0.TableDef{TableType: catalog.SystemSequenceRel}}, + {name: "removed source", tableDef: &plan0.TableDef{TableType: catalog.SystemSourceRel}}, + {name: "temporary session table", tableDef: &plan0.TableDef{ + TableType: catalog.SystemOrdinaryRel, + IsTemporary: true, + }}, + {name: "legacy temporary kind", tableDef: &plan0.TableDef{TableType: catalog.SystemTemporaryTable}}, + } { + t.Run(test.name, func(t *testing.T) { + require.Equal(t, test.want, analyzeTableOwnsPersistentStats(test.tableDef)) + }) + } +} + func isolateOptimizerStatsTest(t *testing.T, sessions ...*Session) { t.Helper() service := "optimizer-stats-" + t.Name() @@ -4813,27 +4846,63 @@ func optimizerStatsTestPlan(tableIDs ...uint64) *plan0.Plan { return &plan0.Plan{Plan: &plan0.Plan_Query{Query: &plan0.Query{Nodes: nodes}}} } -func optimizerStatsVersionsForTest(ses *Session, tableIDs ...uint64) map[uint64]uint64 { - versions := make(map[uint64]uint64, len(tableIDs)) +func optimizerStatsVersionsForTest( + ses *Session, + tableIDs ...uint64, +) map[optimizerStatsTableKey]uint64 { + keys := make([]optimizerStatsTableKey, 0, len(tableIDs)) for _, tableID := range tableIDs { - versions[tableID] = currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) + keys = append(keys, ses.optimizerStatsKey(tableID)) + } + return optimizerStatsVersionsForKeysTest(ses, keys...) + +} + +func optimizerStatsVersionsForKeysTest( + ses *Session, + keys ...optimizerStatsTableKey, +) map[optimizerStatsTableKey]uint64 { + versions := make(map[optimizerStatsTableKey]uint64, len(keys)) + for _, key := range keys { + versions[key] = currentOptimizerStatsVersion(ses.GetService(), key) } return versions } func cacheOptimizerPlanForTest(ses *Session, sql string, tableIDs ...uint64) { + keys := make([]optimizerStatsTableKey, 0, len(tableIDs)) + for _, tableID := range tableIDs { + keys = append(keys, ses.optimizerStatsKey(tableID)) + } + cacheOptimizerPlanForKeysTest(ses, sql, keys...) +} + +func cacheOptimizerPlanForKeysTest(ses *Session, sql string, keys ...optimizerStatsTableKey) { + tableIDs := make([]uint64, 0, len(keys)) + for _, key := range keys { + tableIDs = append(tableIDs, key.tableID) + } ses.cachePlanWithStatsVersions( sql, []tree.Statement{&tree.Select{}}, []*plan0.Plan{optimizerStatsTestPlan(tableIDs...)}, - optimizerStatsVersionsForTest(ses, tableIDs...), + optimizerStatsVersionsForKeysTest(ses, keys...), ) } func cacheOptimizerStatsForTest(t *testing.T, ses *Session, tableID uint64, stats *pbstats.StatsInfo) uint64 { + return cacheOptimizerStatsForKeyTest(t, ses, ses.optimizerStatsKey(tableID), stats) +} + +func cacheOptimizerStatsForKeyTest( + t *testing.T, + ses *Session, + key optimizerStatsTableKey, + stats *pbstats.StatsInfo, +) uint64 { t.Helper() - version := currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) - require.True(t, ses.cacheStatsIfCurrent(tableID, version, stats)) + version := currentOptimizerStatsVersion(ses.GetService(), key) + require.True(t, ses.cacheStatsIfCurrent(key, version, stats)) return version } @@ -4842,6 +4911,7 @@ func TestCompilerContextRecordsTheStatsVersionActuallyRead(t *testing.T) { defer ctrl.Finish() ses, execCtx := newAnalyzeHandlerTestSession(t, ctrl) isolateOptimizerStatsTest(t, ses) + ses.SetAccountId(7) const tableID = uint64(42) wrapper := InitTxnComputationWrapper(ses, &tree.Select{}, execCtx.proc) @@ -4849,17 +4919,26 @@ func TestCompilerContextRecordsTheStatsVersionActuallyRead(t *testing.T) { tcc.SetExecCtx(execCtx) tcc.tcw = wrapper - _, firstVersion := ses.getStatsCacheWithVersion(tableID) + key := tcc.optimizerStatsKey(&plan0.ObjectRef{ + Obj: int64(tableID), + SchemaName: catalog.MO_SYSTEM, + ObjName: catalog.MO_STATEMENT, + }, nil) + require.Equal(t, optimizerStatsTableKey{ + accountID: catalog.System_Account, + tableID: tableID, + }, key) + _, firstVersion := ses.getStatsCacheWithVersion(key) require.NotContains(t, ses.statsCacheVersions, tableID, "a failed or uncached stats read must not grow session version metadata") - _, _, recordedVersion := tcc.getStatsCacheVersion(tableID) + _, _, recordedVersion := tcc.getStatsCacheVersion(key) require.Equal(t, firstVersion, recordedVersion) - require.Equal(t, firstVersion, wrapper.optimizerStatsVersions[tableID]) + require.Equal(t, firstVersion, wrapper.optimizerStatsVersions[key]) - advanceOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) - _, _, currentVersion := tcc.getStatsCacheVersion(tableID) + advanceOptimizerStatsVersion(ses.GetService(), key) + _, _, currentVersion := tcc.getStatsCacheVersion(key) require.NotEqual(t, firstVersion, currentVersion) - require.Equal(t, firstVersion, wrapper.optimizerStatsVersions[tableID], + require.Equal(t, firstVersion, wrapper.optimizerStatsVersions[key], "a plan that read both sides of publication must retain its stale dependency and be rejected") } @@ -4891,8 +4970,10 @@ func TestPublishAnalyzeTableStatsDefinesCacheBoundary(t *testing.T) { ses, execCtx := newAnalyzeHandlerTestSession(t, ctrl) otherSes, _ := newAnalyzeHandlerTestSession(t, ctrl) otherTenantSes, _ := newAnalyzeHandlerTestSession(t, ctrl) - isolateOptimizerStatsTest(t, ses, otherSes, otherTenantSes) + crossAccountSes, _ := newAnalyzeHandlerTestSession(t, ctrl) + isolateOptimizerStatsTest(t, ses, otherSes, otherTenantSes, crossAccountSes) otherTenantSes.SetAccountId(7) + crossAccountSes.SetAccountId(7) const ( tableID = uint64(42) @@ -4913,12 +4994,15 @@ func TestPublishAnalyzeTableStatsDefinesCacheBoundary(t *testing.T) { cacheOptimizerStatsForTest(t, otherSes, otherTableID, otherStats) cacheOptimizerStatsForTest(t, ses, tableID, oldStats) cacheOptimizerStatsForTest(t, otherTenantSes, tableID, oldStats) + physicalKey := optimizerStatsTableKey{accountID: catalog.System_Account, tableID: tableID} + crossAccountOldVersion := cacheOptimizerStatsForKeyTest(t, crossAccountSes, physicalKey, oldStats) dependentPlan := optimizerStatsTestPlan(tableID) compileVersions := optimizerStatsVersionsForTest(otherSes, tableID) cacheOptimizerPlanForTest(otherSes, "select url from events", tableID) cacheOptimizerPlanForTest(otherSes, "select id from other_table", otherTableID) cacheOptimizerPlanForTest(otherTenantSes, "select url from tenant_events", tableID) + cacheOptimizerPlanForKeysTest(crossAccountSes, "select url from system.statement_info", physicalKey) freshStats := plan.NewStatsInfo() freshStats.AccurateObjectNumber = 8 @@ -4931,7 +5015,7 @@ func TestPublishAnalyzeTableStatsDefinesCacheBoundary(t *testing.T) { require.NoError(t, publishAnalyzeTableStats(ses, execCtx.reqCtx, key, refresher)) require.Equal(t, key, gotKey) - cache, _ := ses.getStatsCacheWithVersion(tableID) + cache, _ := ses.getStatsCacheWithVersion(physicalKey) wrapper := cache.Get(tableID) require.Same(t, freshStats, wrapper.GetStats()) otherSes.cachePlanWithStatsVersions("compiled before analyze completed", @@ -4943,15 +5027,20 @@ func TestPublishAnalyzeTableStatsDefinesCacheBoundary(t *testing.T) { "an unrelated table publication must not flush the session plan cache") require.NotNil(t, otherTenantSes.getCachedPlan("select url from tenant_events"), "the same table ID in another account must keep its plan") - require.False(t, otherSes.cacheStatsIfCurrent(tableID, oldVersion, oldStats), + require.Nil(t, crossAccountSes.getCachedPlan("select url from system.statement_info"), + "a tenant plan must validate the system account generation that owns its statistics") + require.False(t, otherSes.cacheStatsIfCurrent(otherSes.optimizerStatsKey(tableID), oldVersion, oldStats), "a stats read started before publication must not repopulate the table entry") - otherCache, currentVersion := otherSes.getStatsCacheWithVersion(tableID) + require.False(t, crossAccountSes.cacheStatsIfCurrent(physicalKey, crossAccountOldVersion, oldStats), + "a cross-account stats read must not repopulate the old physical generation") + otherCache, currentVersion := otherSes.getStatsCacheWithVersion(otherSes.optimizerStatsKey(tableID)) otherWrapper := otherCache.Get(tableID) require.False(t, otherWrapper.Exists()) otherTableWrapper := otherCache.Get(otherTableID) require.Same(t, otherStats, otherTableWrapper.GetStats(), "invalidating one table must retain unrelated statistics") - require.True(t, otherSes.cacheStatsIfCurrent(tableID, currentVersion, freshStats)) + require.True(t, otherSes.cacheStatsIfCurrent( + otherSes.optimizerStatsKey(tableID), currentVersion, freshStats)) currentWrapper := otherCache.Get(tableID) require.Same(t, freshStats, currentWrapper.GetStats()) } @@ -4974,7 +5063,7 @@ func TestPublishAnalyzeTableStatsDoesNotExposeFailedRefresh(t *testing.T) { err := publishAnalyzeTableStats(ses, execCtx.reqCtx, key, refresher) require.ErrorIs(t, err, wantErr) - cache, _ := ses.getStatsCacheWithVersion(tableID) + cache, _ := ses.getStatsCacheWithVersion(ses.optimizerStatsKey(tableID)) wrapper := cache.Get(tableID) require.Same(t, oldStats, wrapper.GetStats()) require.NotNil(t, ses.getCachedPlan("select url from events")) @@ -5002,7 +5091,7 @@ func TestPublishAnalyzeTableStatsRejectsMissingRefreshResult(t *testing.T) { require.Equal(t, version, currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID))) require.Equal(t, clock, currentOptimizerStatsClock(ses.GetService())) - cache, _ := ses.getStatsCacheWithVersion(tableID) + cache, _ := ses.getStatsCacheWithVersion(ses.optimizerStatsKey(tableID)) wrapper := cache.Get(tableID) require.Same(t, oldStats, wrapper.GetStats()) require.NotNil(t, ses.getCachedPlan("select url from events")) diff --git a/pkg/frontend/plan_cache.go b/pkg/frontend/plan_cache.go index e90d16f17ef65..2a65014d1076b 100644 --- a/pkg/frontend/plan_cache.go +++ b/pkg/frontend/plan_cache.go @@ -26,7 +26,7 @@ type cachedPlan struct { stmts []tree.Statement plans []*plan.Plan protocolVersion int64 - statsVersions map[uint64]uint64 + statsVersions map[optimizerStatsTableKey]uint64 } // planCache uses LRU to cache plan for the same sql @@ -60,7 +60,7 @@ func (pc *planCache) cacheWithStatsVersions( sql string, stmts []tree.Statement, plans []*plan.Plan, - statsVersions map[uint64]uint64, + statsVersions map[optimizerStatsTableKey]uint64, versions ...int64, ) { protocolVersion := currentProtocolVersion(nil) @@ -106,23 +106,23 @@ func (pc *planCache) cacheWithStatsVersions( } } -func cloneStatsVersions(versions map[uint64]uint64) map[uint64]uint64 { +func cloneStatsVersions(versions map[optimizerStatsTableKey]uint64) map[optimizerStatsTableKey]uint64 { if len(versions) == 0 { return nil } - cloned := make(map[uint64]uint64, len(versions)) - for tableID, version := range versions { - cloned[tableID] = version + cloned := make(map[optimizerStatsTableKey]uint64, len(versions)) + for key, version := range versions { + cloned[key] = version } return cloned } -func mergeOptimizerStatsVersions(dst, src map[uint64]uint64) bool { - for tableID, version := range src { - if prior, exists := dst[tableID]; exists && prior != version { +func mergeOptimizerStatsVersions(dst, src map[optimizerStatsTableKey]uint64) bool { + for key, version := range src { + if prior, exists := dst[key]; exists && prior != version { return false } - dst[tableID] = version + dst[key] = version } return true } diff --git a/pkg/frontend/plan_cache_test.go b/pkg/frontend/plan_cache_test.go index ee3f80e896f26..83284bd498798 100644 --- a/pkg/frontend/plan_cache_test.go +++ b/pkg/frontend/plan_cache_test.go @@ -279,11 +279,15 @@ func Test_SessionAccessorsWithNilPlanCache(t *testing.T) { } func TestMergeOptimizerStatsVersionsRejectsMixedGenerations(t *testing.T) { - versions := map[uint64]uint64{1: 10} - require.True(t, mergeOptimizerStatsVersions(versions, map[uint64]uint64{1: 10, 2: 20})) - require.Equal(t, map[uint64]uint64{1: 10, 2: 20}, versions) - require.False(t, mergeOptimizerStatsVersions(versions, map[uint64]uint64{1: 11})) - require.Equal(t, uint64(10), versions[1]) + first := optimizerStatsTableKey{accountID: 7, tableID: 1} + second := optimizerStatsTableKey{accountID: 8, tableID: 1} + versions := map[optimizerStatsTableKey]uint64{first: 10} + require.True(t, mergeOptimizerStatsVersions(versions, + map[optimizerStatsTableKey]uint64{first: 10, second: 20})) + require.Equal(t, map[optimizerStatsTableKey]uint64{first: 10, second: 20}, versions) + require.False(t, mergeOptimizerStatsVersions(versions, + map[optimizerStatsTableKey]uint64{first: 11})) + require.Equal(t, uint64(10), versions[first]) } var optimizerStatsVersionsCurrentSink bool @@ -306,14 +310,16 @@ func BenchmarkOptimizerStatsVersionsCurrent(b *testing.B) { {name: "sixteen-tables", dependency: 16}, } { b.Run(tc.name, func(b *testing.B) { - versions := make(map[uint64]uint64, tc.dependency) + versions := make(map[optimizerStatsTableKey]uint64, tc.dependency) for tableID := 1; tableID <= tc.dependency; tableID++ { - versions[uint64(tableID)] = 0 + versions[optimizerStatsTableKey{ + accountID: accountID, + tableID: uint64(tableID), + }] = 0 } b.ReportAllocs() for b.Loop() { - optimizerStatsVersionsCurrentSink = optimizerStatsVersionsCurrent( - service, accountID, versions) + optimizerStatsVersionsCurrentSink = optimizerStatsVersionsCurrent(service, versions) } }) } diff --git a/pkg/frontend/server.go b/pkg/frontend/server.go index f81aefa2d440c..49815c78c8ddd 100644 --- a/pkg/frontend/server.go +++ b/pkg/frontend/server.go @@ -704,8 +704,7 @@ func advanceOptimizerStatsVersionLocked( func optimizerStatsVersionsCurrent( service string, - accountID uint32, - versions map[uint64]uint64, + versions map[optimizerStatsTableKey]uint64, ) bool { if len(versions) == 0 { return true @@ -713,11 +712,8 @@ func optimizerStatsVersionsCurrent( vars := getOptimizerStatsVars(service) vars.optimizerStatsMu.RLock() defer vars.optimizerStatsMu.RUnlock() - for tableID, version := range versions { - if currentOptimizerStatsVersionLocked(vars, optimizerStatsTableKey{ - accountID: accountID, - tableID: tableID, - }) != version { + for key, version := range versions { + if currentOptimizerStatsVersionLocked(vars, key) != version { return false } } diff --git a/pkg/frontend/session.go b/pkg/frontend/session.go index be04d5b68dde2..a55d825442c38 100644 --- a/pkg/frontend/session.go +++ b/pkg/frontend/session.go @@ -287,7 +287,7 @@ type Session struct { statsCacheMu sync.Mutex statsCache *plan2.StatsCache - statsCacheVersions map[uint64]uint64 + statsCacheVersions map[uint64]optimizerStatsCacheTag seqCurValues map[uint64]string /* @@ -812,49 +812,62 @@ func (ses *Session) optimizerStatsKey(tableID uint64) optimizerStatsTableKey { } } -func (ses *Session) getStatsCacheWithVersion(tableID uint64) (*plan2.StatsCache, uint64) { +type optimizerStatsCacheTag struct { + key optimizerStatsTableKey + version uint64 +} + +func (ses *Session) getStatsCacheWithVersion(key optimizerStatsTableKey) (*plan2.StatsCache, uint64) { ses.statsCacheMu.Lock() defer ses.statsCacheMu.Unlock() ses.initStatsCacheLocked() - version := currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) - wrapper := ses.statsCache.Get(tableID) - cachedVersion, tagged := ses.statsCacheVersions[tableID] + version := currentOptimizerStatsVersion(ses.GetService(), key) + wrapper := ses.statsCache.Get(key.tableID) + tag, tagged := ses.statsCacheVersions[key.tableID] if !wrapper.Exists() { - delete(ses.statsCacheVersions, tableID) + delete(ses.statsCacheVersions, key.tableID) } else if !tagged && version == 0 { // Accept caches created before version tracking only in the initial // generation. Once any publication has happened, an untagged entry is // conservatively stale. - ses.statsCacheVersions[tableID] = version - } else if !tagged || cachedVersion != version { - ses.statsCache.Delete(tableID) - delete(ses.statsCacheVersions, tableID) + ses.statsCacheVersions[key.tableID] = optimizerStatsCacheTag{key: key, version: version} + } else if tag.key != key || tag.version != version { + ses.statsCache.Delete(key.tableID) + delete(ses.statsCacheVersions, key.tableID) } return ses.statsCache, version } -func (ses *Session) cacheStatsIfCurrent(tableID, version uint64, stats *pbstats.StatsInfo) bool { +func (ses *Session) cacheStatsIfCurrent( + key optimizerStatsTableKey, + version uint64, + stats *pbstats.StatsInfo, +) bool { ses.statsCacheMu.Lock() defer ses.statsCacheMu.Unlock() - if currentOptimizerStatsVersion(ses.GetService(), ses.optimizerStatsKey(tableID)) != version { + if currentOptimizerStatsVersion(ses.GetService(), key) != version { return false } ses.initStatsCacheLocked() - if ses.statsCache.SetAndReportReset(tableID, stats) { + if ses.statsCache.SetAndReportReset(key.tableID, stats) { clear(ses.statsCacheVersions) } - ses.statsCacheVersions[tableID] = version + ses.statsCacheVersions[key.tableID] = optimizerStatsCacheTag{key: key, version: version} return true } -func (ses *Session) cachePublishedStats(tableID, version uint64, stats *pbstats.StatsInfo) { +func (ses *Session) cachePublishedStats( + key optimizerStatsTableKey, + version uint64, + stats *pbstats.StatsInfo, +) { ses.statsCacheMu.Lock() defer ses.statsCacheMu.Unlock() ses.initStatsCacheLocked() - if ses.statsCache.SetAndReportReset(tableID, stats) { + if ses.statsCache.SetAndReportReset(key.tableID, stats) { clear(ses.statsCacheVersions) } - ses.statsCacheVersions[tableID] = version + ses.statsCacheVersions[key.tableID] = optimizerStatsCacheTag{key: key, version: version} } func (ses *Session) initStatsCacheLocked() { @@ -862,7 +875,7 @@ func (ses *Session) initStatsCacheLocked() { ses.statsCache = plan2.NewStatsCache() } if ses.statsCacheVersions == nil { - ses.statsCacheVersions = make(map[uint64]uint64) + ses.statsCacheVersions = make(map[uint64]optimizerStatsCacheTag) } } @@ -1251,7 +1264,7 @@ func NewSession( timestampMap: map[TS]time.Time{}, statsCache: plan2.NewStatsCache(), - statsCacheVersions: make(map[uint64]uint64), + statsCacheVersions: make(map[uint64]optimizerStatsCacheTag), } atomic.StoreInt32(&ses.sqlModeNoAutoValueOnZero, -1) @@ -1501,13 +1514,13 @@ func (ses *Session) cachePlanWithStatsVersions( sql string, stmts []tree.Statement, plans []*plan.Plan, - statsVersions map[uint64]uint64, + statsVersions map[optimizerStatsTableKey]uint64, versions ...int64, ) { if len(sql) == 0 { return } - if !optimizerStatsVersionsCurrent(ses.GetService(), ses.GetAccountId(), statsVersions) { + if !optimizerStatsVersionsCurrent(ses.GetService(), statsVersions) { // The plan crossed a statistics publication boundary while compiling. // It may execute, but must not enter the cache with stale dependencies. freeStmts(stmts) @@ -1537,7 +1550,7 @@ func (ses *Session) getCachedPlan(sql string) *cachedPlan { } cached := ses.planCache.get(sql) if cached != nil && (cached.protocolVersion != currentProtocolVersion(ses.proc) || - !optimizerStatsVersionsCurrent(ses.GetService(), ses.GetAccountId(), cached.statsVersions)) { + !optimizerStatsVersionsCurrent(ses.GetService(), cached.statsVersions)) { ses.planCache.remove(sql) return nil } @@ -1558,7 +1571,7 @@ func (ses *Session) isCached(sql string) bool { return false } if cached.protocolVersion != currentProtocolVersion(ses.proc) || - !optimizerStatsVersionsCurrent(ses.GetService(), ses.GetAccountId(), cached.statsVersions) { + !optimizerStatsVersionsCurrent(ses.GetService(), cached.statsVersions) { ses.planCache.remove(sql) return false } diff --git a/test/distributed/cases/analyze/analyze_stmt.result b/test/distributed/cases/analyze/analyze_stmt.result index d1017a5365976..4cf1762440685 100644 --- a/test/distributed/cases/analyze/analyze_stmt.result +++ b/test/distributed/cases/analyze/analyze_stmt.result @@ -42,6 +42,13 @@ approx_count_distinct(select) approx_count_distinct(a-b) approx_count_distinct(t select 'AFTER_EXPANDED_QUOTED'; AFTER_EXPANDED_QUOTED AFTER_EXPANDED_QUOTED +create view v_analyze as select a, b from t_analyze_01; +analyze table v_analyze(a); +approx_count_distinct(a) +2 +select 'AFTER_VIEW_ANALYZE'; +AFTER_VIEW_ANALYZE +AFTER_VIEW_ANALYZE create database `select-db`; create table `select-db`.`tick``table`(`a-b` int); insert into `select-db`.`tick``table` values (1),(1),(2); @@ -146,6 +153,7 @@ begin; show profile; not supported: SHOW PROFILE is not supported in MatrixOne rollback; +drop view v_analyze; drop table t_analyze_01; drop table t_analyze_02; drop table quoted_cols; diff --git a/test/distributed/cases/analyze/analyze_stmt.sql b/test/distributed/cases/analyze/analyze_stmt.sql index 291777a59de94..6cf3fde67be56 100644 --- a/test/distributed/cases/analyze/analyze_stmt.sql +++ b/test/distributed/cases/analyze/analyze_stmt.sql @@ -41,6 +41,12 @@ select 'AFTER_EXPLICIT_QUOTED'; analyze table quoted_cols; select 'AFTER_EXPANDED_QUOTED'; +-- views retain the legacy derived-query result and must not be subscribed as +-- physical optimizer-statistics tables +create view v_analyze as select a, b from t_analyze_01; +analyze table v_analyze(a); +select 'AFTER_VIEW_ANALYZE'; + -- quoted database, table, and column identifiers create database `select-db`; create table `select-db`.`tick``table`(`a-b` int); @@ -120,6 +126,7 @@ show profile; rollback; -- cleanup +drop view v_analyze; drop table t_analyze_01; drop table t_analyze_02; drop table quoted_cols; From 7fdc02e7b138929eae7fe3314455d6d979b1b2d5 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 27 Aug 2026 21:17:43 +0800 Subject: [PATCH 03/18] test(stats): close owner and cancellation edges --- pkg/frontend/compiler_context.go | 18 ++++---- pkg/frontend/compiler_context_test.go | 7 +++- pkg/frontend/mysql_cmd_executor_test.go | 49 ++++++++++++++++++++++ pkg/vm/engine/disttae/engine_stats_test.go | 22 ++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/pkg/frontend/compiler_context.go b/pkg/frontend/compiler_context.go index f817c459974c1..07ef95e2ac76d 100644 --- a/pkg/frontend/compiler_context.go +++ b/pkg/frontend/compiler_context.go @@ -269,8 +269,8 @@ func (tcc *TxnCompilerContext) GetAccountId() (uint32, error) { // ResolveViewDependencyAccount returns the account whose catalog namespace was // used to resolve a View dependency. Keep the override order aligned with -// getRelation: snapshot tenant, subscription publisher, then relations that -// are always read from the system account. +// getRelation: snapshot tenant, cluster-table name override, subscription +// publisher, then relations that are always read from the system account. func (tcc *TxnCompilerContext) ResolveViewDependencyAccount( obj *plan2.ObjectRef, tableDef *plan2.TableDef, @@ -291,9 +291,6 @@ func (tcc *TxnCompilerContext) resolvePhysicalObjectAccount( if snapshot != nil && snapshot.Tenant != nil { accountID = snapshot.Tenant.TenantID } - if obj != nil && obj.PubInfo != nil { - accountID = uint32(obj.PubInfo.TenantId) - } var dbName, tableName string if obj != nil { @@ -305,8 +302,15 @@ func (tcc *TxnCompilerContext) resolvePhysicalObjectAccount( if tableName == "" && tableDef != nil { tableName = tableDef.Name } - if (tableDef != nil && tableDef.TableType == catalog.SystemClusterRel) || - isClusterTable(dbName, tableName) || ShouldSwitchToSysAccount(dbName, tableName) { + if isClusterTable(dbName, tableName) { + accountID = sysAccountID + } + // getRelation applies publication ownership after the generic cluster-table + // name rule, so the publisher remains the physical owner in that overlap. + if obj != nil && obj.PubInfo != nil { + accountID = uint32(obj.PubInfo.TenantId) + } + if ShouldSwitchToSysAccount(dbName, tableName) { accountID = sysAccountID } return accountID diff --git a/pkg/frontend/compiler_context_test.go b/pkg/frontend/compiler_context_test.go index 5a608d3887e10..14bc4f571dd5e 100644 --- a/pkg/frontend/compiler_context_test.go +++ b/pkg/frontend/compiler_context_test.go @@ -116,8 +116,11 @@ func TestResolveViewDependencyAccount(t *testing.T) { PubInfo: &pbplan.PubInfo{TenantId: 9}}, snapshot: &pbplan.Snapshot{Tenant: &pbplan.SnapshotTenant{TenantID: 8}}, want: 9}, {name: "cluster table", obj: &pbplan.ObjectRef{SchemaName: catalog.MO_CATALOG, ObjName: "cluster_table"}, want: 0}, - {name: "cluster relation kind", obj: &pbplan.ObjectRef{SchemaName: "db", ObjName: "cluster_table"}, - tableDef: &pbplan.TableDef{TableType: catalog.SystemClusterRel}, want: 0}, + {name: "relation kind alone keeps tenant context", obj: &pbplan.ObjectRef{SchemaName: "db", ObjName: "cluster_table"}, + tableDef: &pbplan.TableDef{TableType: catalog.SystemClusterRel}, want: 7}, + {name: "publication overrides generic cluster name", obj: &pbplan.ObjectRef{ + SchemaName: catalog.MO_CATALOG, ObjName: "cluster_table", + PubInfo: &pbplan.PubInfo{TenantId: 9}}, want: 9}, {name: "statement info", obj: &pbplan.ObjectRef{SchemaName: catalog.MO_SYSTEM, ObjName: catalog.MO_STATEMENT}, want: 0}, {name: "system relation overrides publisher", obj: &pbplan.ObjectRef{SchemaName: catalog.MO_SYSTEM, ObjName: catalog.MO_STATEMENT, PubInfo: &pbplan.PubInfo{TenantId: 9}}, want: 0}, diff --git a/pkg/frontend/mysql_cmd_executor_test.go b/pkg/frontend/mysql_cmd_executor_test.go index 7fdaff5ffd548..18a9f802d4a08 100644 --- a/pkg/frontend/mysql_cmd_executor_test.go +++ b/pkg/frontend/mysql_cmd_executor_test.go @@ -5006,6 +5006,40 @@ func TestCompilerContextRecordsTheStatsVersionActuallyRead(t *testing.T) { "a plan that read both sides of publication must retain its stale dependency and be rejected") } +func TestSessionStatsCacheDoesNotAliasSameTableIDAcrossAccounts(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ses, _ := newAnalyzeHandlerTestSession(t, ctrl) + isolateOptimizerStatsTest(t, ses) + + const tableID = uint64(42) + tenantKey := optimizerStatsTableKey{accountID: 7, tableID: tableID} + systemKey := optimizerStatsTableKey{accountID: catalog.System_Account, tableID: tableID} + tenantStats := plan.NewStatsInfo() + tenantStats.TableCnt = 7 + systemStats := plan.NewStatsInfo() + systemStats.TableCnt = 70 + + tenantVersion := currentOptimizerStatsVersion(ses.GetService(), tenantKey) + require.True(t, ses.cacheStatsIfCurrent(tenantKey, tenantVersion, tenantStats)) + cache, _ := ses.getStatsCacheWithVersion(tenantKey) + wrapper := cache.Get(tableID) + require.Same(t, tenantStats, wrapper.GetStats()) + + cache, systemVersion := ses.getStatsCacheWithVersion(systemKey) + wrapper = cache.Get(tableID) + require.False(t, wrapper.Exists(), + "the physical owner is part of the cache identity") + require.True(t, ses.cacheStatsIfCurrent(systemKey, systemVersion, systemStats)) + wrapper = cache.Get(tableID) + require.Same(t, systemStats, wrapper.GetStats()) + + cache, _ = ses.getStatsCacheWithVersion(tenantKey) + wrapper = cache.Get(tableID) + require.False(t, wrapper.Exists(), + "switching back must not expose statistics from the system account") +} + func TestOptimizerStatsVersionsCompactWithoutRevalidatingOldEntries(t *testing.T) { vars := &ServerLevelVariables{ optimizerStatsVersions: make(map[optimizerStatsTableKey]uint64), @@ -5131,6 +5165,21 @@ func TestPublishAnalyzeTableStatsDoesNotExposeFailedRefresh(t *testing.T) { wrapper := cache.Get(tableID) require.Same(t, oldStats, wrapper.GetStats()) require.NotNil(t, ses.getCachedPlan("select url from events")) + + tableKey := optimizerStatsTableKey{accountID: key.AccId, tableID: key.TableID} + admission := getOptimizerStatsVars(ses.GetService()). + optimizerStatsPublish[optimizerStatsPublisherStripe(tableKey)] + select { + case admission <- struct{}{}: + <-admission + default: + t.Fatal("a failed refresh leaked same-table publication admission") + } + freshStats := plan.NewStatsInfo() + require.NoError(t, publishAnalyzeTableStats(ses, execCtx.reqCtx, key, + analyzeStatsRefresherFunc(func(context.Context, pbstats.StatsInfoKey) (*pbstats.StatsInfo, error) { + return freshStats, nil + })), "a failed refresh must release same-table publication admission") } func TestPublishAnalyzeTableStatsRejectsMissingRefreshResult(t *testing.T) { diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go index 3bf85626be728..37b4136c577cf 100644 --- a/pkg/vm/engine/disttae/engine_stats_test.go +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -92,3 +92,25 @@ func TestOptimizerStatsRefreshAdmissionIsTableScopedAndCancelable(t *testing.T) require.ErrorIs(t, err, context.Canceled) require.Nil(t, releaseCanceled) } + +func TestCoordinateStatsUpdateCancellationReleasesUpdateGeneration(t *testing.T) { + gs := &GlobalStats{} + gs.initStatsRefreshAdmission() + gs.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) + key := pb.StatsInfoKey{AccId: 1, TableID: 42} + + release, err := gs.acquireStatsRefresh(context.Background(), key) + require.NoError(t, err) + defer release() + + canceled, cancel := context.WithCancel(context.Background()) + cancel() + gs.coordinateStatsUpdate(pb.StatsInfoKeyWithContext{Ctx: canceled, Key: key}) + + gs.updatingMu.Lock() + record := gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + require.NotNil(t, record) + require.False(t, record.inProgress, + "cancellation while waiting for refresh admission must close the update generation") +} From d3e3b282862187a0b72494ea876cb3bbc53d53a1 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Thu, 27 Aug 2026 22:18:27 +0800 Subject: [PATCH 04/18] chore(frontend): remove unused plan cache helpers --- pkg/frontend/plan_cache.go | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/pkg/frontend/plan_cache.go b/pkg/frontend/plan_cache.go index fbe14d8257adb..05e7c9d469ebf 100644 --- a/pkg/frontend/plan_cache.go +++ b/pkg/frontend/plan_cache.go @@ -64,30 +64,6 @@ func (pc *planCache) cache(sql string, stmts []tree.Statement, plans []*plan.Pla make([]map[optimizerStatsTableKey]uint64, len(plans)), versions...) } -func (pc *planCache) cacheWithStatsVersions( - sql string, - stmts []tree.Statement, - plans []*plan.Plan, - statsVersions map[optimizerStatsTableKey]uint64, - versions ...int64, -) { - pc.cacheWithPlanSnapshotsAndStatsVersions( - sql, stmts, plans, make([]timestamp.Timestamp, len(plans)), - planStatsVersionsFromAggregate(len(plans), statsVersions), versions...) -} - -func (pc *planCache) cacheWithPlanSnapshots( - sql string, - stmts []tree.Statement, - plans []*plan.Plan, - planSnapshotTS []timestamp.Timestamp, - versions ...int64, -) { - pc.cacheWithPlanSnapshotsAndStatsVersions( - sql, stmts, plans, planSnapshotTS, - make([]map[optimizerStatsTableKey]uint64, len(plans)), versions...) -} - func (pc *planCache) cacheWithPlanSnapshotsAndStatsVersions( sql string, stmts []tree.Statement, From 9df127ce2359a24f729905feea7e81383cc66a38 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 03:23:22 +0800 Subject: [PATCH 05/18] fix(stats): close analyze publication boundaries --- pkg/frontend/mysql_cmd_executor.go | 19 +++++- pkg/frontend/mysql_cmd_executor_test.go | 22 +++++++ pkg/vm/engine/disttae/engine.go | 8 +-- pkg/vm/engine/disttae/engine_stats_test.go | 59 ++++++++++++++----- pkg/vm/engine/disttae/stats.go | 48 ++++++++++++--- .../cases/analyze/analyze_stmt.result | 14 +++++ .../cases/analyze/analyze_stmt.sql | 16 +++++ 7 files changed, 155 insertions(+), 31 deletions(-) diff --git a/pkg/frontend/mysql_cmd_executor.go b/pkg/frontend/mysql_cmd_executor.go index 25faadb978551..38d974d6115ef 100644 --- a/pkg/frontend/mysql_cmd_executor.go +++ b/pkg/frontend/mysql_cmd_executor.go @@ -2075,7 +2075,7 @@ func handleAnalyzeStmt(ses *Session, execCtx *ExecCtx, stmt *tree.AnalyzeStmt) e if err != nil { return err } - if err := refreshAnalyzeTableStats(ses, execCtx.reqCtx, entry); err != nil { + if err := refreshAnalyzeTableStats(ses, execCtx, entry); err != nil { return err } results = append(results, result) @@ -2084,7 +2084,16 @@ func handleAnalyzeStmt(ses *Session, execCtx *ExecCtx, stmt *tree.AnalyzeStmt) e return nil } -func refreshAnalyzeTableStats(ses *Session, ctx context.Context, entry *tree.AnalyzeTableEntry) error { +func refreshAnalyzeTableStats(ses *Session, execCtx *ExecCtx, entry *tree.AnalyzeTableEntry) error { + // The derived ANALYZE query observes the transaction workspace. The engine + // statistics cache is process-global and observes only committed catalog and + // object state, so publishing while a user transaction was already active + // would mix two visibility domains. Preserve the legacy derived result and + // leave global publication to an ANALYZE statement outside that transaction. + if !analyzeStatsPublicationAllowed(execCtx) { + return nil + } + ctx := execCtx.reqCtx if entry == nil || entry.Table == nil || entry.Table.AtTsExpr != nil { return nil } @@ -2133,6 +2142,12 @@ func refreshAnalyzeTableStats(ses *Session, ctx context.Context, entry *tree.Ana return publishAnalyzeTableStats(ses, ctx, key, refresher) } +func analyzeStatsPublicationAllowed(execCtx *ExecCtx) bool { + return execCtx != nil && + execCtx.txnOpt.activeTxnAtStartKnown && + !execCtx.txnOpt.activeTxnAtStart +} + func analyzeTableOwnsPersistentStats(tableDef *plan.TableDef) bool { if tableDef == nil || tableDef.IsTemporary || tableDef.ViewSql != nil { return false diff --git a/pkg/frontend/mysql_cmd_executor_test.go b/pkg/frontend/mysql_cmd_executor_test.go index 18a9f802d4a08..e4dc4057cee51 100644 --- a/pkg/frontend/mysql_cmd_executor_test.go +++ b/pkg/frontend/mysql_cmd_executor_test.go @@ -4885,6 +4885,28 @@ func TestAnalyzeTableOwnsPersistentStats(t *testing.T) { } } +func TestAnalyzeStatsPublicationRequiresStatementOwnedTransaction(t *testing.T) { + for _, test := range []struct { + name string + execCtx *ExecCtx + want bool + }{ + {name: "missing execution context"}, + {name: "unknown transaction owner", execCtx: &ExecCtx{}}, + {name: "statement-owned transaction", execCtx: &ExecCtx{txnOpt: FeTxnOption{ + activeTxnAtStartKnown: true, + }}, want: true}, + {name: "pre-existing user transaction", execCtx: &ExecCtx{txnOpt: FeTxnOption{ + activeTxnAtStartKnown: true, + activeTxnAtStart: true, + }}}, + } { + t.Run(test.name, func(t *testing.T) { + require.Equal(t, test.want, analyzeStatsPublicationAllowed(test.execCtx)) + }) + } +} + func isolateOptimizerStatsTest(t *testing.T, sessions ...*Session) { t.Helper() service := "optimizer-stats-" + t.Name() diff --git a/pkg/vm/engine/disttae/engine.go b/pkg/vm/engine/disttae/engine.go index 1f7630ae6dcc2..d7dd008cb0ae8 100644 --- a/pkg/vm/engine/disttae/engine.go +++ b/pkg/vm/engine/disttae/engine.go @@ -1286,15 +1286,11 @@ func (e *Engine) RefreshTableStats(ctx context.Context, key pb.StatsInfoKey) (*p } type optimizerStatsStore interface { - RefreshWithMode(context.Context, pb.StatsInfoKey, string) error - Get(context.Context, pb.StatsInfoKey, bool) *pb.StatsInfo + refreshStatsWithMode(context.Context, pb.StatsInfoKey, string) (*pb.StatsInfo, error) } func refreshTableStats(ctx context.Context, key pb.StatsInfoKey, store optimizerStatsStore) (*pb.StatsInfo, error) { - if err := store.RefreshWithMode(ctx, key, "auto"); err != nil { - return nil, err - } - return store.Get(ctx, key, false), nil + return store.refreshStatsWithMode(ctx, key, "auto") } // GetGlobalStats returns the GlobalStats instance diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go index 37b4136c577cf..efb23ad599878 100644 --- a/pkg/vm/engine/disttae/engine_stats_test.go +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -28,21 +28,16 @@ type optimizerStatsStoreStub struct { refreshErr error key pb.StatsInfoKey mode string - getCalled bool - getSync bool } -func (s *optimizerStatsStoreStub) RefreshWithMode(_ context.Context, key pb.StatsInfoKey, mode string) error { +func (s *optimizerStatsStoreStub) refreshStatsWithMode( + _ context.Context, + key pb.StatsInfoKey, + mode string, +) (*pb.StatsInfo, error) { s.key = key s.mode = mode - return s.refreshErr -} - -func (s *optimizerStatsStoreStub) Get(_ context.Context, key pb.StatsInfoKey, sync bool) *pb.StatsInfo { - s.getCalled = true - s.key = key - s.getSync = sync - return s.stats + return s.stats, s.refreshErr } func TestRefreshTableStatsDefinesPublicationBoundary(t *testing.T) { @@ -56,8 +51,6 @@ func TestRefreshTableStatsDefinesPublicationBoundary(t *testing.T) { require.Same(t, fresh, got) require.Equal(t, key, store.key) require.Equal(t, "auto", store.mode) - require.True(t, store.getCalled) - require.False(t, store.getSync) }) t.Run("refresh failure is not published", func(t *testing.T) { @@ -67,7 +60,6 @@ func TestRefreshTableStatsDefinesPublicationBoundary(t *testing.T) { got, err := refreshTableStats(context.Background(), key, store) require.ErrorIs(t, err, wantErr) require.Nil(t, got) - require.False(t, store.getCalled) }) } @@ -114,3 +106,42 @@ func TestCoordinateStatsUpdateCancellationReleasesUpdateGeneration(t *testing.T) require.False(t, record.inProgress, "cancellation while waiting for refresh admission must close the update generation") } + +func TestCompleteStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { + gs := &GlobalStats{} + gs.initStatsRefreshAdmission() + gs.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) + key := pb.StatsInfoKey{AccId: 1, TableID: 42} + + gs.updatingMu.updating[key] = &updateRecord{inProgress: true} + oldRelease, err := gs.acquireStatsRefresh(context.Background(), key) + require.NoError(t, err) + + newerDone := make(chan error, 1) + go func() { + newRelease, acquireErr := gs.acquireStatsRefresh(context.Background(), key) + if acquireErr != nil { + newerDone <- acquireErr + return + } + gs.markUpdateComplete(key, true, 100, 0.5) + newRelease() + newerDone <- nil + }() + + // Waiting here after releasing forces the newer refresh to commit between + // release and any code that might incorrectly update the old baseline late. + var newerErr error + gs.completeStatsRefresh(key, true, 1, 1.0, func() { + oldRelease() + newerErr = <-newerDone + }) + require.NoError(t, newerErr) + + gs.updatingMu.Lock() + record := *gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + require.False(t, record.inProgress) + require.Equal(t, int64(100), record.baseObjectCount) + require.Equal(t, 0.5, record.samplingRatio) +} diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index 5ed1fff265d1f..d1f7b9f0a09df 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -915,14 +915,17 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) var updated bool var actualObjectCount int64 var samplingRatio float64 - defer func() { - gs.markUpdateComplete(wrapKey.Key, updated, actualObjectCount, samplingRatio) - }() release, err := gs.acquireStatsRefresh(wrapKey.Ctx, wrapKey.Key) if err != nil { + // shouldExecuteUpdate opened this generation before admission. Close it + // even when cancellation prevents this worker from acquiring the stripe. + gs.markUpdateComplete(wrapKey.Key, false, 0, 0) return } - defer release() + defer func() { + gs.completeStatsRefresh( + wrapKey.Key, updated, actualObjectCount, samplingRatio, release) + }() broadcastWithoutUpdate := func() { gs.mu.Lock() @@ -982,11 +985,38 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) gs.mu.cond.Broadcast() } +// completeStatsRefresh commits the automatic-refresh scheduling metadata +// before another same-table refresh can enter. The statistics cache and its +// object-count/sampling baseline therefore advance in one serialized order. +func (gs *GlobalStats) completeStatsRefresh( + key pb.StatsInfoKey, + updated bool, + actualObjectCount int64, + samplingRatio float64, + release func(), +) { + gs.markUpdateComplete(key, updated, actualObjectCount, samplingRatio) + release() +} + // RefreshWithMode triggers a stats refresh with the specified sampling mode func (gs *GlobalStats) RefreshWithMode(ctx context.Context, key pb.StatsInfoKey, samplingMode string) error { + _, err := gs.refreshStatsWithMode(ctx, key, samplingMode) + return err +} + +// refreshStatsWithMode returns the exact statistics object published while +// same-table refresh admission is still held. Callers that define a synchronous +// publication boundary must use this result instead of re-reading the map after +// admission has been released. +func (gs *GlobalStats) refreshStatsWithMode( + ctx context.Context, + key pb.StatsInfoKey, + samplingMode string, +) (*pb.StatsInfo, error) { release, err := gs.acquireStatsRefresh(ctx, key) if err != nil { - return err + return nil, err } defer release() @@ -999,13 +1029,13 @@ func (gs *GlobalStats) RefreshWithMode(ctx context.Context, key pb.StatsInfoKey, key.DatabaseID, key.DbName) if err != nil { - return moerr.NewInternalErrorNoCtxf("failed to subscribe table: %v", err) + return nil, moerr.NewInternalErrorNoCtxf("failed to subscribe table: %v", err) } // Get table definition table := gs.engine.GetLatestCatalogCache().GetTableById(key.AccId, key.DatabaseID, key.TableID) if table == nil || table.TableDef == nil { - return moerr.NewInternalErrorNoCtx("table not found") + return nil, moerr.NewInternalErrorNoCtx("table not found") } // Create stats info @@ -1032,7 +1062,7 @@ func (gs *GlobalStats) RefreshWithMode(ctx context.Context, key pb.StatsInfoKey, // Execute stats update samplingRatio, err := CollectAndCalculateStats(ctx, req, gs.concurrentExecutor) if err != nil { - return moerr.NewInternalErrorNoCtxf("failed to update stats: %v", err) + return nil, moerr.NewInternalErrorNoCtxf("failed to update stats: %v", err) } // Update cache @@ -1044,7 +1074,7 @@ func (gs *GlobalStats) RefreshWithMode(ctx context.Context, key pb.StatsInfoKey, // Record sampling ratio in updateRecord gs.markUpdateComplete(key, true, stats.AccurateObjectNumber, samplingRatio) - return nil + return stats, nil } func (gs *GlobalStats) executeStatsUpdate(ctx context.Context, ps *logtailreplay.PartitionState, key pb.StatsInfoKey, stats *pb.StatsInfo) (bool, float64) { diff --git a/test/distributed/cases/analyze/analyze_stmt.result b/test/distributed/cases/analyze/analyze_stmt.result index 4cf1762440685..d410c1feddcce 100644 --- a/test/distributed/cases/analyze/analyze_stmt.result +++ b/test/distributed/cases/analyze/analyze_stmt.result @@ -146,6 +146,20 @@ AFTER_TXN_MULTI AFTER_TXN_MULTI rollback; begin; +create table txn_created_analyze(a int); +insert into txn_created_analyze values (1), (2); +analyze table txn_created_analyze(a); +approx_count_distinct(a) +2 +rollback; +drop table if exists txn_created_analyze; +begin; +insert into t_analyze_01 values (3, 30); +analyze table t_analyze_01(a); +approx_count_distinct(a) +3 +rollback; +begin; check table t_analyze_01; not supported: CHECK TABLE is not supported in MatrixOne rollback; diff --git a/test/distributed/cases/analyze/analyze_stmt.sql b/test/distributed/cases/analyze/analyze_stmt.sql index 6cf3fde67be56..9ac29b2c4f055 100644 --- a/test/distributed/cases/analyze/analyze_stmt.sql +++ b/test/distributed/cases/analyze/analyze_stmt.sql @@ -117,6 +117,22 @@ analyze table t_analyze_01, t_analyze_02; select 'AFTER_TXN_MULTI'; rollback; +-- A transaction-local table is visible to the derived ANALYZE query but not +-- to the process-global optimizer statistics refresher. +begin; +create table txn_created_analyze(a int); +insert into txn_created_analyze values (1), (2); +analyze table txn_created_analyze(a); +rollback; +drop table if exists txn_created_analyze; + +-- The result includes uncommitted workspace rows, while global optimizer +-- statistics remain at the committed visibility boundary. +begin; +insert into t_analyze_01 values (3, 30); +analyze table t_analyze_01(a); +rollback; + begin; check table t_analyze_01; rollback; From 3a051c7dbd6f069123c6117af4009a85a2944a82 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 04:24:48 +0800 Subject: [PATCH 06/18] docs(design): define analyze stats publication --- docs/design/analyze_stats_publication.md | 342 +++++++++++++++++++++++ 1 file changed, 342 insertions(+) create mode 100644 docs/design/analyze_stats_publication.md diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md new file mode 100644 index 0000000000000..064e43c35a738 --- /dev/null +++ b/docs/design/analyze_stats_publication.md @@ -0,0 +1,342 @@ +# ANALYZE Statistics Publication and Plan-Cache Freshness + +- Status: draft; independent design approval required before implementation delivery +- Tracking issue: [matrixorigin/matrixone#27728](https://github.com/matrixorigin/matrixone/issues/27728) +- Implementation PR: [matrixorigin/matrixone#27758](https://github.com/matrixorigin/matrixone/pull/27758) +- Last updated: 2026-08-28 + +## 1. Problem and evidence + +`ANALYZE TABLE` currently computes its SQL-visible result through a derived +aggregate query. Optimizer statistics, however, are owned by disttae's +process-local `GlobalStats` cache and are refreshed independently. A successful +ANALYZE therefore does not establish a boundary after which later statements on +the same CN must plan with the newly collected optimizer statistics. + +The observable failure is a long-lived session that continues to reuse both its +three-second statistics cache and a cached logical plan after ANALYZE. In the +reported Q35-shaped workload, a plan built before ANALYZE kept the stale +non-shuffle topology. Reconnecting appeared to fix the query only because a new +session discarded both caches; reconnecting is not a valid publication +contract. + +The implementation must also reject partial refreshes. Object statistics are +collected concurrently from S3 metadata. A missing/corrupt object, cancellation, +or executor shutdown must not be logged and then treated as a successful scan, +because that would publish a partially accumulated `StatsInfo` and invalidate +plans in favor of worse data. + +## 2. Scope + +This design covers: + +- current, physical tables analyzed outside an already-active user transaction; +- synchronous publication into the local disttae optimizer-statistics cache; +- invalidation of dependent session statistics and plan-cache entries on the + same CN process; +- serialization with automatic logtail-driven refreshes for the same physical + table; +- bounded process/session metadata and executor shutdown/error behavior. + +It intentionally does not provide: + +- cluster-wide or cross-CN invalidation; +- publication for historical snapshots, views, temporary tables, subscriptions, + or other relations that do not own current persistent optimizer statistics; +- publication of a derived query's uncommitted workspace view into the + committed, process-global statistics cache; +- a persistent generation, wire-protocol change, catalog change, or on-disk + format change; +- a guarantee that prepared statements whose physical compile lifecycle is + independently retained will be rebuilt by this first phase. + +## 3. Required invariants + +### 3.1 Safety + +1. A successful ANALYZE publication is all-or-nothing: every admitted object + task completes successfully before the engine cache is replaced. +2. For one physical `(account_id, table_id)`, explicit and automatic refreshes + publish in one serialized order. +3. The frontend generation advances only after the engine cache replacement + succeeds, and the issuing session is tagged with that exact generation. +4. A plan is reusable only if every statistics generation captured while + building it still equals the current generation. +5. Work that observed generation N cannot repopulate a session cache after N+1 + has been published. +6. Physical ownership is resolved before cache lookup. Tenant, system/cluster, + and publisher identities must not alias solely because table IDs match. + +### 3.2 Liveness and ownership + +1. Every frontend and engine admission token has exactly one effective release + owner on success, error, and cancellation. +2. Every concurrent object task is completed by exactly one owner: a worker + executes it, or executor shutdown rejects it. A rejected queued task must + still release the caller's completion barrier. +3. Admission and executor submission observe caller cancellation. Executor + shutdown cannot leave a producer blocked on a full queue or a caller waiting + for abandoned queued work. +4. Unrelated tables normally remain parallel. A bounded hash-stripe collision + may serialize refresh control work but cannot affect query execution. + +### 3.3 Boundedness + +1. Frontend publication admission uses 64 fixed stripes. +2. Engine refresh admission uses 64 fixed stripes. +3. The process-local table-generation registry retains at most 64K explicit + keys. Compaction advances a reset generation before clearing the map, so all + older missing-key labels become conservatively stale. +4. Session statistics remain bounded by the existing `StatsCache` policy; its + generation tags reset atomically with that cache. +5. Plan dependencies are bounded by the plan-cache capacity and by the number + of physical tables referenced by each cached plan. +6. Concurrent object work remains bounded by the existing worker count and + 2,048-entry executor queue. The fix must not add a channel/future allocation + per object on the successful collection path. + +## 4. Identity and visibility + +The generation key is the physical owner: + +```text +(account_id, table_id) +``` + +The account is resolved using the same precedence as relation resolution: + +1. snapshot tenant, when applicable; +2. cluster/system ownership rules; +3. publication ownership where relation resolution selects the publisher; +4. explicit system-account overrides. + +Current publication is skipped for historical AS OF references, publication +consumers, temporary tables, views, and non-persistent relation kinds. These +paths preserve the legacy derived ANALYZE result and do not claim a current +engine-cache boundary. + +An ANALYZE whose statement starts with an active transaction also preserves the +legacy result without global publication. The derived SQL can see the +transaction workspace, whereas `GlobalStats` is committed-object state. Mixing +those visibility domains would make uncommitted data process-global. + +## 5. State and publication model + +### 5.1 End-to-end order + +For a publishable table, success follows this order: + +```text +derived ANALYZE query succeeds + -> acquire frontend table stripe + -> acquire engine table stripe + -> subscribe current partition and resolve current table definition + -> submit visible-object tasks + -> wait for every task result or rejection + -> atomically replace GlobalStats entry and wake engine waiters + -> commit engine refresh scheduling metadata + -> release engine stripe + -> advance frontend table generation + -> cache returned StatsInfo in issuing session under that generation + -> release frontend stripe + -> return ANALYZE success +``` + +There are two related linearization points: + +- the engine data publication point is replacement of `statsInfoMap[key]` after + all object tasks succeed; +- the frontend reuse boundary is advancement of the table generation while the + frontend publication stripe is still held. + +The frontend point intentionally follows the engine point. Before generation +advancement, an old plan may still run against the old generation. After it, +new cache admission and later cache hits must reject that old generation. + +### 5.2 Automatic refresh interaction + +Automatic logtail refresh and explicit ANALYZE share the engine stripe. An +automatic refresh commits its statistics entry and object-count/sampling +baseline before releasing that stripe. This prevents an older refresh from +overwriting the scheduling metadata of a newer explicit refresh. + +Frontend publication needs a separate stripe because it serializes the larger +engine-publication-plus-generation transaction. Without it, two concurrent +ANALYZE statements could publish engine results A then B but advance/cache their +frontend generations in the opposite order. + +Hash collisions deliberately trade rare refresh serialization for fixed memory. +They do not merge table identity: engine maps, generations, session tags, and +plan dependencies remain keyed by the full physical key. + +### 5.3 Plan and session-cache admission + +Planning records the first generation observed for each physical table. A +generation change during repeated reads makes the completed plan ineligible for +cache admission. Cache lookup compares all recorded dependencies against the +current process registry; only dependent plans are removed. + +The session statistics cache uses a `(physical key, generation)` tag in addition +to its historical table-ID lookup. A slow storage read can cache its result only +if the generation is unchanged on completion. Publication installs the exact +engine result into the issuing session under the newly advanced generation. + +## 6. Failure, cancellation, and shutdown + +The terminal behavior is: + +| Failure phase | Engine cache | Engine metadata | Frontend generation/session cache | SQL result | +| --- | --- | --- | --- | --- | +| derived query fails | unchanged | unchanged | unchanged | error | +| frontend admission canceled | unchanged | unchanged | unchanged | cancellation | +| subscribe/catalog resolution fails | unchanged | unchanged | unchanged | error | +| task submission canceled/rejected | unchanged | failed generation closed | unchanged | error | +| object task fails or is canceled | unchanged; local partial object discarded | failed generation closed | unchanged | error | +| engine cache publication succeeds | replaced | committed before engine release | generation must then advance while frontend token is held | success only after remaining steps | + +The shared object executor owns queued tasks only after successful admission. +On normal operation, a worker removes a task and executes its callback. During +executor shutdown, new submissions fail, workers stop after their current task, +and a shutdown owner rejects every task left in the queue. Execution and +rejection both invoke the caller-provided completion callback exactly once. + +The per-refresh error accumulator records the first non-nil task or rejection +error and waits for all admitted work before returning. Waiting is required +because callbacks mutate a refresh-local accumulator; returning early would let +old work race a discarded accumulator. Callback I/O receives the request +context, so cancellation terminates the expensive work without polling or +sleeps. + +Cancellation and deadline errors remain cancellation/deadline errors at the +public refresh boundary. Other object/metadata failures may be wrapped with +table-refresh context, but must remain a failed publication. + +Retry starts a fresh refresh and generation attempt. No generation is reserved +before success, so a failed retry creates no gap that consumers must interpret. + +## 7. Compatibility and operations + +`engine.StatsRefresher` is an optional in-process capability. Engines that do +not implement it preserve legacy ANALYZE behavior. There is no persisted or wire +state, so mixed binaries do not need protocol negotiation: each CN invalidates +only its own caches and a restart naturally starts a new local generation epoch +with empty sessions. + +Rollback removes the optional publication call and process-local generation +tracking. The disttae statistics cache remains compatible with its existing +automatic refresh path. No data/catalog migration, backup/restore action, or +downgrade procedure is required. + +Operational diagnosis should distinguish: + +- derived ANALYZE query duration; +- synchronous disttae refresh duration and S3 request counts; +- refresh failures by subscribe, catalog, object I/O, cancellation, or executor + shutdown; +- plan-cache invalidations caused by statistics generations; +- generation-registry compactions and refresh-stripe wait time. + +The initial implementation can reuse current refresh logging and metrics, but +the above dimensions are the required observability target before enabling a +cross-CN extension. + +## 8. Performance and capacity model + +The common TP path with no recorded statistics dependency performs no new +generation-map read. A dependent cache hit takes one process-local read lock and +O(number of referenced physical tables) comparisons, with no allocations. The +implementation evidence at the PR revision measured approximately 1.5 ns for +zero dependencies, 47 ns for one, 56 ns for four, and 131-136 ns for sixteen. +These values are directional microbenchmark evidence, not a production latency +SLO. + +ANALYZE adds a synchronous disttae object-metadata scan after its derived query. +This increases ANALYZE latency and S3 reads but moves the cost off the normal TP +execution path. The reported 10M-row Q35-shaped case completed that refresh in +approximately 181 ms and changed the next plan from stale non-shuffle to a +16-way hash plan. Production acceptance must continue to compare object count, +sampling mode, S3 requests, and end-to-end ANALYZE latency. + +The executor repair keeps the existing closure and wait-group shape. It adds a +success-path branch and error accumulator, plus executor lifecycle bookkeeping; +it must not create one result channel or goroutine per object. Focused +benchmarks or allocation tests are required if the final implementation changes +that property. + +## 9. Alternatives + +### A. Keep TTL-only session caching + +This is the status quo. It is simple and cross-CN-neutral, but cannot define a +synchronous ANALYZE boundary and leaves cached plans stale indefinitely. It is +rejected for correctness. + +### B. Flush every session and plan cache on ANALYZE + +A process-wide flush is easy to reason about but turns one table's maintenance +into unrelated TP plan rebuilds and requires enumerating/live-coordinating all +sessions. It has a larger latency and availability blast radius and is rejected. + +### C. Rely only on the engine statistics-map replacement + +This fixes fresh statistics reads but not a session's three-second cache or +logical plan cache. It also cannot fence a slow old read from repopulating a +session cache. It is insufficient. + +### D. Broadcast generations across CNs + +Cross-CN invalidation is the desired broader semantic but requires a distributed +ordering, retry, restart, compatibility, and bounded replay design. Adding it to +this repair would materially enlarge the failure surface. The selected first +phase is explicitly CN-local and leaves this as a separately designed feature. + +### E. Selected: CN-local per-table generation after synchronous engine refresh + +This gives a precise local boundary, invalidates only dependent state, requires +no persistent/wire change, and keeps ANALYZE cost out of the normal TP path. Its +tradeoffs are local-only scope, two bounded admission layers, and dependency +checks on affected plan-cache hits. + +## 10. Verification map + +| Contract | Deterministic evidence | +| --- | --- | +| successful publication and exact returned object | engine publication-boundary UT | +| failed engine refresh does not advance/cache | frontend publisher failure UT | +| one successful and one failed object task rejects partial stats | concurrent visible-object UT | +| pre-canceled and shutdown-rejected work terminates | executor/visible-object cancellation UT | +| same-table refresh order; unrelated-table concurrency | frontend and engine admission race UT | +| slow generation-N read cannot overwrite N+1 | session-cache race UT | +| plan build spanning publication is not cached | plan-cache generation UT | +| physical account/view/temporary/transaction rules | focused frontend table-driven UT and ANALYZE BVT | +| no-dependency and 1/4/16-dependency cache-hit cost | allocation/latency benchmarks | +| SQL-visible existing-session plan changes after ANALYZE | real-service BVT and explain-plan assertion | + +Every new concurrency test uses explicit phase barriers and an outer timeout +only as a hang guard. The changed disttae and frontend packages require focused +normal tests, focused adaptive race stress, and one owning-package race run when +the local CGo environment can execute their linked test binaries. + +## 11. Rollout and decision log + +The initial rollout is behavior-on for disttae engines implementing the optional +capability. It fails closed: inability to complete the refresh returns an error +and leaves previous generations/caches valid. Legacy engines and excluded table +classes retain their previous behavior. + +Decision log: + +- Use physical `(account_id, table_id)` identity; names are lookup inputs, not + generation identity. +- Publish only committed current-table state; do not globalize an active + transaction's derived result. +- Keep the first phase CN-local; cross-CN publication requires another design. +- Use fixed stripes and conservative 64K compaction rather than unbounded + per-table locks/generations. +- Wait for all admitted object work after the first failure so no callback can + outlive its refresh-local accumulator. +- Preserve no per-object future/channel allocation in the successful scan path. + +Open approval item: an independent reviewer must approve this exact design +revision before the implementation is considered deliverable. There are no +known unresolved correctness decisions in the proposal itself. From abfe90d7da319ad925a1a49d8d3a8444d25894f5 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 04:27:02 +0800 Subject: [PATCH 07/18] fix(stats): reject partial analyze refreshes --- pkg/vm/engine/disttae/stats.go | 18 ++- pkg/vm/engine/disttae/txn_table.go | 69 +++++++-- pkg/vm/engine/disttae/util.go | 142 ++++++++++++++--- pkg/vm/engine/disttae/util_test.go | 241 +++++++++++++++++++++++++++-- 4 files changed, 418 insertions(+), 52 deletions(-) diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index d1f7b9f0a09df..b393188de625a 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -17,6 +17,7 @@ package disttae import ( "context" "encoding/binary" + "errors" "math" "runtime" "sync" @@ -1062,8 +1063,17 @@ func (gs *GlobalStats) refreshStatsWithMode( // Execute stats update samplingRatio, err := CollectAndCalculateStats(ctx, req, gs.concurrentExecutor) if err != nil { + if cause := context.Cause(ctx); cause != nil { + return nil, cause + } + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil, err + } return nil, moerr.NewInternalErrorNoCtxf("failed to update stats: %v", err) } + if cause := context.Cause(ctx); cause != nil { + return nil, cause + } // Update cache gs.mu.Lock() @@ -1113,6 +1123,9 @@ func (gs *GlobalStats) executeStatsUpdate(ctx context.Context, ps *logtailreplay logutil.Errorf("failed to init stats info for table %v, err: %v", key, err) return false, 0 } + if context.Cause(ctx) != nil { + return false, 0 + } v2.StatsUpdateDurationHistogram.Observe(time.Since(start).Seconds()) v2.StatsUpdateBlockCounter.Add(float64(stats.BlockNumber)) return true, samplingRatio @@ -1247,7 +1260,7 @@ func collectTableStats( var sampledRowCount float64 var sampledObjectCount int64 - onObjFn := func(obj objectio.ObjectEntry) error { + onObjFn := func(objCtx context.Context, obj objectio.ObjectEntry) error { objName := obj.ObjectShortName() // ===== Phase 1: Get exact values from ObjectStats (no IO) ===== @@ -1278,7 +1291,7 @@ func collectTableStats( // Sampled object: read ObjectMeta (requires IO) location := obj.Location() - objMeta, err := objectio.FastLoadObjectMeta(ctx, &location, false, fs) + objMeta, err := objectio.FastLoadObjectMeta(objCtx, &location, false, fs) if err != nil { return err } @@ -1417,6 +1430,7 @@ func collectTableStats( } if err := ForeachVisibleObjects( + ctx, req.partitionState, req.ts, onObjFn, diff --git a/pkg/vm/engine/disttae/txn_table.go b/pkg/vm/engine/disttae/txn_table.go index b49f6fa9ccce1..aac71849085f8 100644 --- a/pkg/vm/engine/disttae/txn_table.go +++ b/pkg/vm/engine/disttae/txn_table.go @@ -363,36 +363,77 @@ func (tbl *txnTable) Size(ctx context.Context, columnName string) (uint64, error } func ForeachVisibleObjects( + ctx context.Context, state *logtailreplay.PartitionState, ts types.TS, - fn func(obj objectio.ObjectEntry) error, + fn func(context.Context, objectio.ObjectEntry) error, executor ConcurrentExecutor, visitTombstone bool, ) (err error) { + if cause := context.Cause(ctx); cause != nil { + return cause + } iter, err := state.NewObjectsIter(ts, true, visitTombstone) if err != nil { return err } defer iter.Close() - var wg sync.WaitGroup + + taskCtx, cancelTasks := context.WithCancelCause(ctx) + defer cancelTasks(nil) + var ( + wg sync.WaitGroup + firstErrOnce sync.Once + firstErr error + ) + completeTask := func(taskErr error) { + if taskErr != nil { + firstErrOnce.Do(func() { + firstErr = taskErr + // Stop sibling object I/O and prevent further admission. Already + // admitted work is still joined below before its accumulator dies. + cancelTasks(taskErr) + }) + } + wg.Done() + } + for iter.Next() { + if cause := context.Cause(taskCtx); cause != nil { + err = cause + break + } entry := iter.Entry() if executor != nil { wg.Add(1) - executor.AppendTask(func() error { - defer wg.Done() - return fn(entry) - }) + appendErr := executor.AppendTask( + taskCtx, + func() error { return fn(taskCtx, entry) }, + completeTask, + ) + if appendErr != nil { + // Ownership was not transferred to the executor. + completeTask(appendErr) + err = appendErr + break + } } else { - if err = fn(entry); err != nil { + if err = fn(taskCtx, entry); err != nil { + cancelTasks(err) break } } } if executor != nil { wg.Wait() + if firstErr != nil { + return firstErr + } } - return + if err != nil { + return err + } + return context.Cause(ctx) } // not accurate! only used by stats @@ -430,10 +471,10 @@ func (tbl *txnTable) MaxAndMinValues(ctx context.Context) ([][2]any, []uint8, er return nil, nil, err } var updateMu sync.Mutex - onObjFn := func(obj objectio.ObjectEntry) error { + onObjFn := func(objCtx context.Context, obj objectio.ObjectEntry) error { var err error location := obj.Location() - if objMeta, err = objectio.FastLoadObjectMeta(ctx, &location, false, fs); err != nil { + if objMeta, err = objectio.FastLoadObjectMeta(objCtx, &location, false, fs); err != nil { return err } updateMu.Lock() @@ -460,6 +501,7 @@ func (tbl *txnTable) MaxAndMinValues(ctx context.Context) ([][2]any, []uint8, er } if err = ForeachVisibleObjects( + ctx, part, types.TimestampToTS(tbl.db.op.SnapshotTS()), onObjFn, @@ -516,7 +558,7 @@ func (tbl *txnTable) GetColumMetadataScanInfo(ctx context.Context, name string, } infoList := make([]*plan.MetadataScanInfo, 0, state.ApproxDataObjectsNum()) var updateMu sync.Mutex - onObjFn := func(obj objectio.ObjectEntry) error { + onObjFn := func(objCtx context.Context, obj objectio.ObjectEntry) error { createTs, err := obj.CreateTime.Marshal() if err != nil { return err @@ -548,7 +590,7 @@ func (tbl *txnTable) GetColumMetadataScanInfo(ctx context.Context, name string, return nil } - objMeta, err := objectio.FastLoadObjectMeta(ctx, &location, false, fs) + objMeta, err := objectio.FastLoadObjectMeta(objCtx, &location, false, fs) if err != nil { return err } @@ -578,6 +620,7 @@ func (tbl *txnTable) GetColumMetadataScanInfo(ctx context.Context, name string, } if err = ForeachVisibleObjects( + ctx, state, types.TimestampToTS(tbl.db.op.SnapshotTS()), onObjFn, @@ -3343,7 +3386,7 @@ func (tbl *txnTable) GetNonAppendableObjectStats(ctx context.Context) ([]objecti sortKeyPos, _ := tbl.getSortKeyPosAndSortKeyIsPK() objStats := make([]objectio.ObjectStats, 0, tbl.ApproxObjectsNum(ctx)) - err = ForeachVisibleObjects(state, snapshot, func(obj objectio.ObjectEntry) error { + err = ForeachVisibleObjects(ctx, state, snapshot, func(_ context.Context, obj objectio.ObjectEntry) error { if obj.GetAppendable() { return nil } diff --git a/pkg/vm/engine/disttae/util.go b/pkg/vm/engine/disttae/util.go index f678d64647da4..e4a9a29b47013 100644 --- a/pkg/vm/engine/disttae/util.go +++ b/pkg/vm/engine/disttae/util.go @@ -18,9 +18,10 @@ import ( "bytes" "context" "encoding/hex" - "errors" "fmt" "reflect" + "sync" + "sync/atomic" "go.uber.org/zap" @@ -627,8 +628,10 @@ type concurrentTask func() error // ConcurrentExecutor is an interface that runs tasks concurrently. type ConcurrentExecutor interface { - // AppendTask append the concurrent task to the exuecutor. - AppendTask(concurrentTask) + // AppendTask admits a task or returns without taking ownership. Once + // admitted, the executor calls complete exactly once with either the task + // result or its own lifecycle cancellation. + AppendTask(context.Context, concurrentTask, func(error)) error // Run starts receive task to execute. Run(context.Context) // GetConcurrency returns the concurrency of this executor. @@ -639,40 +642,135 @@ type concurrentExecutor struct { // concurrency is the concurrency to run the tasks at the same time. concurrency int // task contains all the tasks needed to run. - tasks chan concurrentTask + tasks chan queuedConcurrentTask + + runOnce sync.Once + stopOnce sync.Once + stopCh chan struct{} + stopCause atomic.Pointer[concurrentExecutorStop] + workers sync.WaitGroup + + // submitMu closes the race between a producer admitting work and shutdown + // draining the queue. Shutdown signals stopCh before taking the write lock, + // so a producer blocked on a full queue can always leave promptly. + submitMu sync.RWMutex + stopped bool +} + +type queuedConcurrentTask struct { + run concurrentTask + complete func(error) +} + +type concurrentExecutorStop struct { + err error } func newConcurrentExecutor(concurrency int) ConcurrentExecutor { return &concurrentExecutor{ concurrency: concurrency, - tasks: make(chan concurrentTask, 2048), + tasks: make(chan queuedConcurrentTask, 2048), + stopCh: make(chan struct{}), } } // AppendTask implements the ConcurrentExecutor interface. -func (e *concurrentExecutor) AppendTask(t concurrentTask) { - e.tasks <- t +func (e *concurrentExecutor) AppendTask( + ctx context.Context, + t concurrentTask, + complete func(error), +) error { + e.submitMu.RLock() + defer e.submitMu.RUnlock() + if e.stopped { + return e.stoppedError() + } + + // Prefer rejection once shutdown is already observable. The second select + // still covers shutdown or caller cancellation racing queue admission. + select { + case <-e.stopCh: + return e.stoppedError() + default: + } + select { + case e.tasks <- queuedConcurrentTask{run: t, complete: complete}: + return nil + case <-ctx.Done(): + return context.Cause(ctx) + case <-e.stopCh: + return e.stoppedError() + } } // Run implements the ConcurrentExecutor interface. func (e *concurrentExecutor) Run(ctx context.Context) { - for i := 0; i < e.concurrency; i++ { - go func() { - for { - select { - case <-ctx.Done(): - return - - case t := <-e.tasks: - if err := t(); err != nil { - if !errors.Is(err, context.Canceled) { - logutil.Errorf("failed to execute task: %v", err) - } - } - } + e.runOnce.Do(func() { + e.workers.Add(e.concurrency) + for i := 0; i < e.concurrency; i++ { + go e.runWorker() + } + go e.stopWhenDone(ctx) + }) +} + +func (e *concurrentExecutor) runWorker() { + defer e.workers.Done() + for { + // Make shutdown deterministic after the current task. Without this + // priority check, a ready queue and a closed stop channel could keep + // selecting more data work while shutdown waits. + select { + case <-e.stopCh: + return + default: + } + select { + case <-e.stopCh: + return + case task := <-e.tasks: + err := task.run() + if task.complete != nil { + task.complete(err) } - }() + } + } +} + +func (e *concurrentExecutor) stopWhenDone(ctx context.Context) { + <-ctx.Done() + cause := context.Cause(ctx) + if cause == nil { + cause = context.Canceled + } + e.stopOnce.Do(func() { + e.stopCause.Store(&concurrentExecutorStop{err: cause}) + close(e.stopCh) + }) + + // Wait for every producer that could still admit a task, then prevent all + // future admission before workers stop and the remaining queue is rejected. + e.submitMu.Lock() + e.stopped = true + e.submitMu.Unlock() + e.workers.Wait() + for { + select { + case task := <-e.tasks: + if task.complete != nil { + task.complete(cause) + } + default: + return + } + } +} + +func (e *concurrentExecutor) stoppedError() error { + if stopped := e.stopCause.Load(); stopped != nil && stopped.err != nil { + return stopped.err } + return context.Canceled } // GetConcurrency implements the ConcurrentExecutor interface. diff --git a/pkg/vm/engine/disttae/util_test.go b/pkg/vm/engine/disttae/util_test.go index 35183131add4c..d6cf95c7f5c73 100644 --- a/pkg/vm/engine/disttae/util_test.go +++ b/pkg/vm/engine/disttae/util_test.go @@ -16,10 +16,12 @@ package disttae import ( "context" + "errors" "io" "math/rand" "slices" "sync" + "sync/atomic" "testing" "time" @@ -661,24 +663,233 @@ func TestConcurrentExecutor_Run(t *testing.T) { require.Equal(t, 3, ex.GetConcurrency()) var wg sync.WaitGroup - wg.Add(1) - ex.AppendTask(func() error { - defer wg.Done() + submit := func(task concurrentTask) { + wg.Add(1) + err := ex.AppendTask(ctx, task, func(error) { + wg.Done() + }) + if err != nil { + wg.Done() + } + require.NoError(t, err) + } + submit(func() error { return nil }) + submit(func() error { return context.Canceled }) + submit(func() error { return io.EOF }) + wg.Wait() +} + +func TestConcurrentExecutorRejectsQueuedTasksOnShutdown(t *testing.T) { + ex := newConcurrentExecutor(1) + executorCtx, stopExecutor := context.WithCancel(context.Background()) + defer stopExecutor() + ex.Run(executorCtx) + + firstStarted := make(chan struct{}) + releaseFirst := make(chan struct{}) + require.NoError(t, ex.AppendTask(context.Background(), func() error { + close(firstStarted) + <-releaseFirst return nil - }) + }, func(err error) { + if err != nil { + t.Errorf("running task was unexpectedly rejected: %v", err) + } + })) + select { + case <-firstStarted: + case <-time.After(time.Second): + close(releaseFirst) + t.Fatal("executor did not start its admitted task") + } - wg.Add(1) - ex.AppendTask(func() error { - defer wg.Done() - return context.Canceled - }) + secondRan := atomic.Bool{} + secondRejected := make(chan error, 1) + require.NoError(t, ex.AppendTask(context.Background(), func() error { + secondRan.Store(true) + return nil + }, func(err error) { + secondRejected <- err + })) + + stopExecutor() + <-ex.(*concurrentExecutor).stopCh + close(releaseFirst) + select { + case err := <-secondRejected: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(time.Second): + t.Fatal("executor shutdown abandoned an admitted task") + } + require.False(t, secondRan.Load()) - wg.Add(1) - ex.AppendTask(func() error { - defer wg.Done() - return io.EOF - }) - wg.Wait() + err := ex.AppendTask(context.Background(), func() error { return nil }, nil) + require.ErrorIs(t, err, context.Canceled) +} + +func visibleObjectStateForExecutorTest(t *testing.T, count int) *logtailreplay.PartitionState { + t.Helper() + ctx := context.Background() + state := logtailreplay.NewPartitionState("", true, 42, false) + state.UpdateDuration(types.TS{}, types.MaxTs()) + for i := 0; i < count; i++ { + oid := types.NewObjectid() + stats := objectio.NewObjectStatsWithObjectID(&oid, false, false, false) + require.NoError(t, objectio.SetObjectStatsSize(stats, 1)) + require.NoError(t, state.HandleObjectEntry(ctx, nil, objectio.ObjectEntry{ + ObjectStats: *stats, + CreateTime: types.BuildTS(int64(i+1), 0), + }, false)) + } + return state +} + +func TestForeachVisibleObjectsPropagatesConcurrentTaskError(t *testing.T) { + state := visibleObjectStateForExecutorTest(t, 2) + ex := newConcurrentExecutor(2) + executorCtx, stopExecutor := context.WithCancel(context.Background()) + defer stopExecutor() + ex.Run(executorCtx) + + wantErr := errors.New("object metadata unavailable") + allStarted := make(chan struct{}) + var releaseOnce sync.Once + t.Cleanup(func() { releaseOnce.Do(func() { close(allStarted) }) }) + var calls atomic.Int32 + result := make(chan error, 1) + go func() { + result <- ForeachVisibleObjects( + context.Background(), state, types.MaxTs(), + func(_ context.Context, _ objectio.ObjectEntry) error { + call := calls.Add(1) + if call == 2 { + releaseOnce.Do(func() { close(allStarted) }) + } + <-allStarted + if call == 1 { + return wantErr + } + return nil + }, + ex, + false, + ) + }() + var err error + select { + case err = <-result: + case <-time.After(time.Second): + t.Fatal("concurrent visible-object traversal did not join its tasks") + } + require.ErrorIs(t, err, wantErr) + require.Equal(t, int32(2), calls.Load(), "one successful and one failed task must both be joined") +} + +func TestForeachVisibleObjectsHonorsCancellationBeforeAdmission(t *testing.T) { + state := visibleObjectStateForExecutorTest(t, 1) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cancel() + + var called atomic.Bool + err := ForeachVisibleObjects( + ctx, state, types.MaxTs(), + func(context.Context, objectio.ObjectEntry) error { + called.Store(true) + return nil + }, + newConcurrentExecutor(1), + false, + ) + require.ErrorIs(t, err, context.Canceled) + require.False(t, called.Load()) +} + +func TestForeachVisibleObjectsCancelsInFlightTask(t *testing.T) { + state := visibleObjectStateForExecutorTest(t, 1) + ex := newConcurrentExecutor(1) + executorCtx, stopExecutor := context.WithCancel(context.Background()) + defer stopExecutor() + ex.Run(executorCtx) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + taskStarted := make(chan struct{}) + result := make(chan error, 1) + go func() { + result <- ForeachVisibleObjects( + ctx, state, types.MaxTs(), + func(taskCtx context.Context, _ objectio.ObjectEntry) error { + close(taskStarted) + <-taskCtx.Done() + return context.Cause(taskCtx) + }, + ex, + false, + ) + }() + + select { + case <-taskStarted: + case <-time.After(time.Second): + t.Fatal("executor did not start the admitted visible-object task") + } + cancel() + select { + case err := <-result: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(time.Second): + t.Fatal("in-flight object task ignored caller cancellation") + } +} + +func TestCollectAndCalculateStatsDoesNotApplyFailedObjectScan(t *testing.T) { + ctx := context.Background() + state := logtailreplay.NewPartitionState("", true, 42, false) + state.UpdateDuration(types.TS{}, types.MaxTs()) + location := objectio.NewRandomLocation(1, 128) + stats := objectio.NewObjectStats() + objectio.SetObjectStatsLocation(stats, location) + require.NoError(t, objectio.SetObjectStatsSize(stats, 1)) + require.NoError(t, state.HandleObjectEntry(ctx, nil, objectio.ObjectEntry{ + ObjectStats: *stats, + CreateTime: types.BuildTS(1, 0), + }, false)) + require.Equal(t, 1, state.ApproxDataObjectsNum()) + + fs, err := fileservice.NewMemoryFS( + defines.SharedFileServiceName, + fileservice.DisabledCacheConfig, + nil, + ) + require.NoError(t, err) + published := plan2.NewStatsInfo() + req := &updateStatsRequest{ + statsInfo: published, + tableDef: &plan.TableDef{ + Name: "events", + Cols: []*plan.ColDef{ + {Name: "event_id", Seqnum: 0, Typ: plan.Type{Id: int32(types.T_int64)}}, + {Name: "__mo_rowid", Seqnum: 1, Typ: plan.Type{Id: int32(types.T_Rowid)}}, + }, + }, + partitionState: state, + fs: fs, + ts: types.MaxTs(), + approxObjectNum: 1, + samplingMode: "full", + } + ex := newConcurrentExecutor(1) + executorCtx, stopExecutor := context.WithCancel(context.Background()) + defer stopExecutor() + ex.Run(executorCtx) + + _, err = CollectAndCalculateStats(ctx, req, ex) + require.Error(t, err) + require.Zero(t, published.TableCnt) + require.Zero(t, published.AccurateObjectNumber) + require.Empty(t, published.NdvMap) + require.Empty(t, published.ShuffleRangeMap) } func TestShrinkBatchWithRowids(t *testing.T) { From 5d9146e1a24a0b632f6e205bbc28eaa53ebae6f7 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 04:30:50 +0800 Subject: [PATCH 08/18] docs(design): close executor shutdown path --- docs/design/analyze_stats_publication.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index 064e43c35a738..c15525fb09669 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -74,9 +74,10 @@ It intentionally does not provide: 2. Every concurrent object task is completed by exactly one owner: a worker executes it, or executor shutdown rejects it. A rejected queued task must still release the caller's completion barrier. -3. Admission and executor submission observe caller cancellation. Executor - shutdown cannot leave a producer blocked on a full queue or a caller waiting - for abandoned queued work. +3. Admission and executor submission observe caller cancellation. The shared + traversal context also observes executor lifecycle cancellation, so shutdown + cannot leave a producer blocked on a full queue, a running S3 task using an + orphaned request, or a caller waiting for abandoned queued work. 4. Unrelated tables normally remain parallel. A bounded hash-stripe collision may serialize refresh control work but cannot affect query execution. @@ -196,7 +197,8 @@ The terminal behavior is: The shared object executor owns queued tasks only after successful admission. On normal operation, a worker removes a task and executes its callback. During -executor shutdown, new submissions fail, workers stop after their current task, +executor shutdown, new submissions fail, the executor lifecycle cancels the +shared context used by already-running callbacks, workers join those callbacks, and a shutdown owner rejects every task left in the queue. Execution and rejection both invoke the caller-provided completion callback exactly once. @@ -304,7 +306,7 @@ checks on affected plan-cache hits. | successful publication and exact returned object | engine publication-boundary UT | | failed engine refresh does not advance/cache | frontend publisher failure UT | | one successful and one failed object task rejects partial stats | concurrent visible-object UT | -| pre-canceled and shutdown-rejected work terminates | executor/visible-object cancellation UT | +| pre-canceled, in-flight canceled, and shutdown-rejected work terminates | executor/visible-object cancellation UT | | same-table refresh order; unrelated-table concurrency | frontend and engine admission race UT | | slow generation-N read cannot overwrite N+1 | session-cache race UT | | plan build spanning publication is not cached | plan-cache generation UT | From 28acbd4cc787b9b613f2b582394eafc327a777ae Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 04:30:51 +0800 Subject: [PATCH 09/18] fix(stats): cancel running scans on shutdown --- pkg/vm/engine/disttae/txn_table.go | 12 ++++++++++ pkg/vm/engine/disttae/util.go | 18 +++++++++++++-- pkg/vm/engine/disttae/util_test.go | 36 ++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/pkg/vm/engine/disttae/txn_table.go b/pkg/vm/engine/disttae/txn_table.go index aac71849085f8..82c9820f601eb 100644 --- a/pkg/vm/engine/disttae/txn_table.go +++ b/pkg/vm/engine/disttae/txn_table.go @@ -381,6 +381,18 @@ func ForeachVisibleObjects( taskCtx, cancelTasks := context.WithCancelCause(ctx) defer cancelTasks(nil) + if executor != nil { + if lifecycle := executor.LifecycleContext(); lifecycle != nil { + stopLifecycleWatch := context.AfterFunc(lifecycle, func() { + cause := context.Cause(lifecycle) + if cause == nil { + cause = context.Canceled + } + cancelTasks(cause) + }) + defer stopLifecycleWatch() + } + } var ( wg sync.WaitGroup firstErrOnce sync.Once diff --git a/pkg/vm/engine/disttae/util.go b/pkg/vm/engine/disttae/util.go index e4a9a29b47013..d014be34dec1f 100644 --- a/pkg/vm/engine/disttae/util.go +++ b/pkg/vm/engine/disttae/util.go @@ -634,6 +634,10 @@ type ConcurrentExecutor interface { AppendTask(context.Context, concurrentTask, func(error)) error // Run starts receive task to execute. Run(context.Context) + // LifecycleContext is canceled when the executor stops. A task group uses + // it to cancel work that a worker already owns, while queued work is rejected + // through the completion callback above. + LifecycleContext() context.Context // GetConcurrency returns the concurrency of this executor. GetConcurrency() int } @@ -653,8 +657,9 @@ type concurrentExecutor struct { // submitMu closes the race between a producer admitting work and shutdown // draining the queue. Shutdown signals stopCh before taking the write lock, // so a producer blocked on a full queue can always leave promptly. - submitMu sync.RWMutex - stopped bool + submitMu sync.RWMutex + stopped bool + lifecycle context.Context } type queuedConcurrentTask struct { @@ -706,6 +711,9 @@ func (e *concurrentExecutor) AppendTask( // Run implements the ConcurrentExecutor interface. func (e *concurrentExecutor) Run(ctx context.Context) { e.runOnce.Do(func() { + e.submitMu.Lock() + e.lifecycle = ctx + e.submitMu.Unlock() e.workers.Add(e.concurrency) for i := 0; i < e.concurrency; i++ { go e.runWorker() @@ -714,6 +722,12 @@ func (e *concurrentExecutor) Run(ctx context.Context) { }) } +func (e *concurrentExecutor) LifecycleContext() context.Context { + e.submitMu.RLock() + defer e.submitMu.RUnlock() + return e.lifecycle +} + func (e *concurrentExecutor) runWorker() { defer e.workers.Done() for { diff --git a/pkg/vm/engine/disttae/util_test.go b/pkg/vm/engine/disttae/util_test.go index d6cf95c7f5c73..212c9a2d2ffb7 100644 --- a/pkg/vm/engine/disttae/util_test.go +++ b/pkg/vm/engine/disttae/util_test.go @@ -843,6 +843,42 @@ func TestForeachVisibleObjectsCancelsInFlightTask(t *testing.T) { } } +func TestForeachVisibleObjectsCancelsInFlightTaskOnExecutorShutdown(t *testing.T) { + state := visibleObjectStateForExecutorTest(t, 1) + ex := newConcurrentExecutor(1) + executorCtx, stopExecutor := context.WithCancel(context.Background()) + defer stopExecutor() + ex.Run(executorCtx) + + taskStarted := make(chan struct{}) + result := make(chan error, 1) + go func() { + result <- ForeachVisibleObjects( + context.Background(), state, types.MaxTs(), + func(taskCtx context.Context, _ objectio.ObjectEntry) error { + close(taskStarted) + <-taskCtx.Done() + return context.Cause(taskCtx) + }, + ex, + false, + ) + }() + + select { + case <-taskStarted: + case <-time.After(time.Second): + t.Fatal("executor did not start the admitted visible-object task") + } + stopExecutor() + select { + case err := <-result: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(time.Second): + t.Fatal("in-flight object task outlived executor shutdown") + } +} + func TestCollectAndCalculateStatsDoesNotApplyFailedObjectScan(t *testing.T) { ctx := context.Background() state := logtailreplay.NewPartitionState("", true, 42, false) From 90929350b71930fff8cc4ee945edb9b5cfb3d1a4 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 04:32:33 +0800 Subject: [PATCH 10/18] docs(design): record cache-hit benchmark --- docs/design/analyze_stats_publication.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index c15525fb09669..44fe7d48d41ae 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -1,6 +1,6 @@ # ANALYZE Statistics Publication and Plan-Cache Freshness -- Status: draft; independent design approval required before implementation delivery +- Status: implementation record; ordinary bug-fix design gate exempt - Tracking issue: [matrixorigin/matrixone#27728](https://github.com/matrixorigin/matrixone/issues/27728) - Implementation PR: [matrixorigin/matrixone#27758](https://github.com/matrixorigin/matrixone/pull/27758) - Last updated: 2026-08-28 @@ -247,10 +247,10 @@ cross-CN extension. The common TP path with no recorded statistics dependency performs no new generation-map read. A dependent cache hit takes one process-local read lock and O(number of referenced physical tables) comparisons, with no allocations. The -implementation evidence at the PR revision measured approximately 1.5 ns for -zero dependencies, 47 ns for one, 56 ns for four, and 131-136 ns for sixteen. -These values are directional microbenchmark evidence, not a production latency -SLO. +local Apple M4 evidence at implementation revision `28acbd4cc7` measured +1.77-1.83 ns for zero dependencies, 35.21-35.47 ns for one, 46.23-47.95 ns for +four, and 115.6-128.7 ns for sixteen, all with zero allocations. These values +are directional microbenchmark evidence, not a production latency SLO. ANALYZE adds a synchronous disttae object-metadata scan after its derived query. This increases ANALYZE latency and S3 reads but moves the cost off the normal TP From d07f6d2220c68bed763b20874a8cfc5bcefbfa7a Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 05:38:12 +0800 Subject: [PATCH 11/18] fix: preserve optimizer stats refresh lifecycle --- docs/design/analyze_stats_publication.md | 29 ++- pkg/vm/engine/disttae/engine_stats_test.go | 61 ++++- pkg/vm/engine/disttae/stats.go | 247 ++++++++++++++++----- pkg/vm/engine/disttae/stats_test.go | 55 ++++- 4 files changed, 328 insertions(+), 64 deletions(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index 44fe7d48d41ae..5723a56a607eb 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -66,6 +66,10 @@ It intentionally does not provide: has been published. 6. Physical ownership is resolved before cache lookup. Tenant, system/cluster, and publisher identities must not alias solely because table IDs match. +7. A failed automatic refresh cannot replace a previously published statistics + value. It may install a nil completion sentinel only when the table has no + prior cache entry, so first-read waiters terminate without losing last-good + state. ### 3.2 Liveness and ownership @@ -95,6 +99,14 @@ It intentionally does not provide: 6. Concurrent object work remains bounded by the existing worker count and 2,048-entry executor queue. The fix must not add a channel/future allocation per object on the successful collection path. +7. Engine statistics and refresh-scheduling metadata share the table cleanup + boundary. Once an unsubscribed table reaches `RemoveTid`, neither + `statsInfoMap` nor `updatingMu.updating` retains any key for that table ID, + and a late automatic-refresh publication/completion cannot recreate either + entry or write into a replacement generation. The scheduling-record pointer + captured at enqueue is the automatic generation's lifetime token. Logtail's + cache-existence check and token capture use the same cleanup lock order, so + removal cannot fall between those two producer steps. ## 4. Identity and visibility @@ -161,6 +173,12 @@ automatic refresh commits its statistics entry and object-count/sampling baseline before releasing that stripe. This prevents an older refresh from overwriting the scheduling metadata of a newer explicit refresh. +The automatic-refresh cache transition is last-good preserving. Success +replaces the cached value; failure leaves an existing entry untouched. When the +first automatic attempt fails, it installs a nil completion sentinel and wakes +synchronous waiters, preserving the existing `GlobalStats.Get` termination +contract without representing the failure as a newer publication. + Frontend publication needs a separate stripe because it serializes the larger engine-publication-plus-generation transaction. Without it, two concurrent ANALYZE statements could publish engine results A then B but advance/cache their @@ -190,7 +208,8 @@ The terminal behavior is: | --- | --- | --- | --- | --- | | derived query fails | unchanged | unchanged | unchanged | error | | frontend admission canceled | unchanged | unchanged | unchanged | cancellation | -| subscribe/catalog resolution fails | unchanged | unchanged | unchanged | error | +| explicit subscribe/catalog resolution fails | unchanged | unchanged | unchanged | error | +| automatic subscribe/catalog resolution fails | last-good entry retained; nil completion sentinel only when absent | failed generation closed | unchanged | not an ANALYZE result | | task submission canceled/rejected | unchanged | failed generation closed | unchanged | error | | object task fails or is canceled | unchanged; local partial object discarded | failed generation closed | unchanged | error | | engine cache publication succeeds | replaced | committed before engine release | generation must then advance while frontend token is held | success only after remaining steps | @@ -308,6 +327,8 @@ checks on affected plan-cache hits. | one successful and one failed object task rejects partial stats | concurrent visible-object UT | | pre-canceled, in-flight canceled, and shutdown-rejected work terminates | executor/visible-object cancellation UT | | same-table refresh order; unrelated-table concurrency | frontend and engine admission race UT | +| failed automatic refresh preserves last-good stats and completes an absent first generation | injected subscribe-failure state-transition UT | +| table cleanup reclaims both statistics and refresh-scheduling entries; late automatic publication/completion cannot recreate them | `RemoveTid` ownership UT | | slow generation-N read cannot overwrite N+1 | session-cache race UT | | plan build spanning publication is not cached | plan-cache generation UT | | physical account/view/temporary/transaction rules | focused frontend table-driven UT and ANALYZE BVT | @@ -338,6 +359,12 @@ Decision log: - Wait for all admitted object work after the first failure so no callback can outlive its refresh-local accumulator. - Preserve no per-object future/channel allocation in the successful scan path. +- Preserve the last successful automatic-refresh value on failure; use a nil + sentinel only to complete an otherwise absent first generation. +- Make `RemoveTid` the common lifetime owner for published engine statistics + and per-table refresh-scheduling metadata; automatic publication and + completion require the update record as a lifetime token, so an old worker + cannot recreate cleanup-owned state. Open approval item: an independent reviewer must approve this exact design revision before the implementation is considered deliverable. There are no diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go index efb23ad599878..7d8b03060faf2 100644 --- a/pkg/vm/engine/disttae/engine_stats_test.go +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -17,9 +17,12 @@ package disttae import ( "context" "errors" + "sync" "testing" + "github.com/matrixorigin/matrixone/pkg/objectio" pb "github.com/matrixorigin/matrixone/pkg/pb/statsinfo" + "github.com/matrixorigin/matrixone/pkg/util/fault" "github.com/stretchr/testify/require" ) @@ -113,7 +116,8 @@ func TestCompleteStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { gs.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) key := pb.StatsInfoKey{AccId: 1, TableID: 42} - gs.updatingMu.updating[key] = &updateRecord{inProgress: true} + generation := &updateRecord{inProgress: true} + gs.updatingMu.updating[key] = generation oldRelease, err := gs.acquireStatsRefresh(context.Background(), key) require.NoError(t, err) @@ -132,7 +136,7 @@ func TestCompleteStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { // Waiting here after releasing forces the newer refresh to commit between // release and any code that might incorrectly update the old baseline late. var newerErr error - gs.completeStatsRefresh(key, true, 1, 1.0, func() { + gs.completeStatsRefresh(key, generation, true, 1, 1.0, func() { oldRelease() newerErr = <-newerDone }) @@ -145,3 +149,56 @@ func TestCompleteStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { require.Equal(t, int64(100), record.baseObjectCount) require.Equal(t, 0.5, record.samplingRatio) } + +func TestCoordinateStatsUpdateSubscribeFailurePreservesLastPublishedStats(t *testing.T) { + key := pb.StatsInfoKey{ + AccId: 1, DatabaseID: 10, TableID: 42, DbName: "db", TableName: "events", + } + newStats := func() *pb.StatsInfo { + return &pb.StatsInfo{TableCnt: 1_000_000} + } + newGlobalStats := func() *GlobalStats { + gs := &GlobalStats{engine: &Engine{}} + gs.initStatsRefreshAdmission() + gs.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) + gs.mu.statsInfoMap = make(map[pb.StatsInfoKey]*pb.StatsInfo) + gs.mu.cond = sync.NewCond(&gs.mu) + return gs + } + + fault.Enable() + t.Cleanup(func() { fault.Disable() }) + removeFault, err := objectio.InjectLogging( + objectio.FJ_CNSubscribeTableFail, key.DbName, key.TableName, 0, true, + ) + require.NoError(t, err) + t.Cleanup(removeFault) + + t.Run("retain last successful publication", func(t *testing.T) { + gs := newGlobalStats() + lastGood := newStats() + gs.mu.statsInfoMap[key] = lastGood + + gs.coordinateStatsUpdate(pb.StatsInfoKeyWithContext{Ctx: context.Background(), Key: key}) + + gs.mu.Lock() + got, exists := gs.mu.statsInfoMap[key] + gs.mu.Unlock() + require.True(t, exists) + require.Same(t, lastGood, got, + "a failed automatic refresh must not erase the last successful publication") + }) + + t.Run("complete first failed generation", func(t *testing.T) { + gs := newGlobalStats() + + gs.coordinateStatsUpdate(pb.StatsInfoKeyWithContext{Ctx: context.Background(), Key: key}) + + gs.mu.Lock() + got, exists := gs.mu.statsInfoMap[key] + gs.mu.Unlock() + require.True(t, exists, + "the first failed automatic generation must still wake synchronous waiters") + require.Nil(t, got) + }) +} diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index b393188de625a..b85ced22cba78 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -55,7 +55,7 @@ import ( // logtailConsumer (1个 goroutine) // │ // │ 判断入队条件（第一层）： -// │ - keyExists(): key 必须已存在 +// │ - cache/generation 原子检查：key 必须已存在 // │ - CkpLocation: checkpoint 时触发 // │ - MetaEntry: object 元数据变更时触发 // │ @@ -214,6 +214,14 @@ type updateRecord struct { samplingRatio float64 } +// statsUpdateJob carries the scheduling generation observed by the producer. +// Pointer identity prevents a queued worker from publishing into a newer table +// generation after RemoveTid deleted the old record. +type statsUpdateJob struct { + wrapKey pb.StatsInfoKeyWithContext + expectedRecord *updateRecord +} + type GlobalStats struct { ctx context.Context @@ -225,7 +233,7 @@ type GlobalStats struct { // TODO(volgariver6): add metrics of the chan length. tailC chan *logtail.TableLogtail - updateC chan pb.StatsInfoKeyWithContext + updateC chan statsUpdateJob // queueWatcher keeps the table id and its enqueue time. // and watch the queue item in the queue. @@ -280,7 +288,7 @@ func NewGlobalStats( ctx: ctx, engine: e, tailC: make(chan *logtail.TableLogtail, 10000), - updateC: make(chan pb.StatsInfoKeyWithContext, 3000), + updateC: make(chan statsUpdateJob, 3000), KeyRouter: keyRouter, queueWatcher: newQueueWatcher(), } @@ -349,26 +357,29 @@ func (gs *GlobalStats) acquireStatsRefresh( } } -// keyExists returns true only if key already exists in the map. -func (gs *GlobalStats) keyExists(key pb.StatsInfoKey) bool { - gs.mu.Lock() - defer gs.mu.Unlock() - _, ok := gs.mu.statsInfoMap[key] - return ok -} - -// RemoveTid removes all statsInfoMap entries for the given table ID. +// RemoveTid removes every GlobalStats entry owned by the given table ID. // Called from cleanMemoryTableWithTable (1+ hour after unsubscribe/drop) -// to prevent unbounded map growth. Safe because no queries target a -// table that has been unsubscribed for over an hour. +// to prevent both published statistics and refresh-scheduling metadata from +// growing for the process lifetime. Safe because no queries target a table +// that has been unsubscribed for over an hour. func (gs *GlobalStats) RemoveTid(tableID uint64) { + // Keep the established gs.mu -> updatingMu lock order used by + // broadcastStats. Table cleanup is the common lifetime boundary for both + // maps, so a removed table cannot retain one owner after losing the other. gs.mu.Lock() defer gs.mu.Unlock() + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() for key := range gs.mu.statsInfoMap { if key.TableID == tableID { delete(gs.mu.statsInfoMap, key) } } + for key := range gs.updatingMu.updating { + if key.TableID == tableID { + delete(gs.updatingMu.updating, key) + } + } if gs.mu.cond != nil { gs.mu.cond.Broadcast() } @@ -382,6 +393,12 @@ func (gs *GlobalStats) PrefetchTableMeta(ctx context.Context, key pb.StatsInfoKe return gs.enqueueStatsUpdate(wrapkey, false) } +func (gs *GlobalStats) currentUpdateRecord(key pb.StatsInfoKey) *updateRecord { + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() + return gs.updatingMu.updating[key] +} + func (gs *GlobalStats) subscribedEntry(key pb.StatsInfoKey) *subEntry { gs.engine.pClient.subscribed.rw.RLock() defer gs.engine.pClient.subscribed.rw.RUnlock() @@ -574,12 +591,12 @@ func (gs *GlobalStats) spawnUpdateWorkers(ctx context.Context, num int) { case <-ctx.Done(): return - case key := <-gs.updateC: + case job := <-gs.updateC: // after dequeue from the chan, remove the table ID from the queue watcher. - gs.queueWatcher.del(key.Key.TableID) + gs.queueWatcher.del(job.wrapKey.Key.TableID) v2.StatsTriggerConsumeCounter.Add(1) - gs.coordinateStatsUpdate(key) + gs.coordinateStatsUpdateJob(job) } } }() @@ -587,12 +604,21 @@ func (gs *GlobalStats) spawnUpdateWorkers(ctx context.Context, num int) { } func (gs *GlobalStats) enqueueStatsUpdate(key pb.StatsInfoKeyWithContext, force bool) bool { + return gs.enqueueStatsUpdateForRecord(key, force, gs.currentUpdateRecord(key.Key)) +} + +func (gs *GlobalStats) enqueueStatsUpdateForRecord( + key pb.StatsInfoKeyWithContext, + force bool, + expectedRecord *updateRecord, +) bool { defer func() { v2.StatsTriggerQueueSizeGauge.Set(float64(len(gs.updateC))) }() + job := statsUpdateJob{wrapKey: key, expectedRecord: expectedRecord} if force { select { - case gs.updateC <- key: + case gs.updateC <- job: gs.queueWatcher.add(key.Key.TableID) v2.StatsTriggerForcedCounter.Add(1) return true @@ -602,7 +628,7 @@ func (gs *GlobalStats) enqueueStatsUpdate(key pb.StatsInfoKeyWithContext, force } select { - case gs.updateC <- key: + case gs.updateC <- job: gs.queueWatcher.add(key.Key.TableID) v2.StatsTriggerUnforcedCounter.Add(1) return true @@ -631,11 +657,14 @@ func (gs *GlobalStats) processLogtail(ctx context.Context, tail *logtail.TableLo } if len(tail.CkpLocation) > 0 || metaChanges > 0 { - if gs.keyExists(key) && gs.shouldEnqueueUpdate(key, metaChanges, len(tail.CkpLocation) > 0) { - gs.enqueueStatsUpdate(pb.StatsInfoKeyWithContext{ + record, ok := gs.shouldEnqueueExistingStatsUpdateGeneration( + key, metaChanges, len(tail.CkpLocation) > 0, + ) + if ok { + gs.enqueueStatsUpdateForRecord(pb.StatsInfoKeyWithContext{ Ctx: ctx, Key: key, - }, false) + }, false, record) } } } @@ -650,17 +679,52 @@ func (gs *GlobalStats) processLogtail(ctx context.Context, tail *logtail.TableLo // - Accumulated change rate >= 5%, OR // - Time since last update > 30min func (gs *GlobalStats) shouldEnqueueUpdate(key pb.StatsInfoKey, metaChanges int, hasCheckpoint bool) bool { + _, ok := gs.shouldEnqueueUpdateGeneration(key, metaChanges, hasCheckpoint) + return ok +} +func (gs *GlobalStats) shouldEnqueueUpdateGeneration( + key pb.StatsInfoKey, + metaChanges int, + hasCheckpoint bool, +) (*updateRecord, bool) { gs.updatingMu.Lock() defer gs.updatingMu.Unlock() + return gs.shouldEnqueueUpdateGenerationLocked(key, metaChanges, hasCheckpoint) +} +// shouldEnqueueExistingStatsUpdateGeneration links the logtail producer to the +// same table-lifetime boundary as RemoveTid. The cache-existence check and +// scheduling-record capture are atomic in the established gs.mu -> updatingMu +// lock order, so cleanup cannot fall between them. +func (gs *GlobalStats) shouldEnqueueExistingStatsUpdateGeneration( + key pb.StatsInfoKey, + metaChanges int, + hasCheckpoint bool, +) (*updateRecord, bool) { + gs.mu.Lock() + defer gs.mu.Unlock() + if _, ok := gs.mu.statsInfoMap[key]; !ok { + return nil, false + } + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() + return gs.shouldEnqueueUpdateGenerationLocked(key, metaChanges, hasCheckpoint) +} + +func (gs *GlobalStats) shouldEnqueueUpdateGenerationLocked( + key pb.StatsInfoKey, + metaChanges int, + hasCheckpoint bool, +) (*updateRecord, bool) { rec, ok := gs.updatingMu.updating[key] if !ok { // First time: create record and enqueue - gs.updatingMu.updating[key] = &updateRecord{ + rec = &updateRecord{ pendingChanges: metaChanges, } - return true + gs.updatingMu.updating[key] = rec + return rec, true } // Accumulate pending changes @@ -668,7 +732,7 @@ func (gs *GlobalStats) shouldEnqueueUpdate(key pb.StatsInfoKey, metaChanges int, // Small table: enqueue on any change if rec.baseObjectCount < LargeTableThreshold { - return metaChanges > 0 || hasCheckpoint + return rec, metaChanges > 0 || hasCheckpoint } // Large table: check two conditions (enqueue if either is true) @@ -676,16 +740,16 @@ func (gs *GlobalStats) shouldEnqueueUpdate(key pb.StatsInfoKey, metaChanges int, if rec.baseObjectCount > 0 { changeRate := float64(rec.pendingChanges) / float64(rec.baseObjectCount) if changeRate >= LargeTableChangeRateThreshold { - return true + return rec, true } } // Condition 2: Time since last update > 30min if time.Since(rec.lastUpdate) > LargeTableMaxUpdateInterval { - return true + return rec, true } - return false + return rec, false } // shouldUpdate implements a debounce mechanism to prevent excessive stats updates. @@ -694,23 +758,42 @@ func (gs *GlobalStats) shouldEnqueueUpdate(key pb.StatsInfoKey, metaChanges int, // Only checks inProgress and MinUpdateInterval. // Change rate is NOT checked here (already checked in shouldEnqueueUpdate). func (gs *GlobalStats) shouldExecuteUpdate(key pb.StatsInfoKey) bool { + _, ok := gs.startAutomaticUpdate(key, nil) + return ok +} + +func (gs *GlobalStats) startAutomaticUpdate( + key pb.StatsInfoKey, + expectedRecord *updateRecord, +) (*updateRecord, bool) { gs.updatingMu.Lock() defer gs.updatingMu.Unlock() rec, ok := gs.updatingMu.updating[key] + if expectedRecord != nil && (!ok || rec != expectedRecord) { + return nil, false + } if !ok { - gs.updatingMu.updating[key] = &updateRecord{ + rec = &updateRecord{ inProgress: true, } - return true + gs.updatingMu.updating[key] = rec + return rec, true } if rec.inProgress { - return false + return nil, false } if time.Since(rec.lastUpdate) > MinUpdateInterval { rec.inProgress = true - return true + return rec, true } - return false + return nil, false +} + +func (gs *GlobalStats) automaticUpdateActive(key pb.StatsInfoKey, generation *updateRecord) bool { + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() + rec, ok := gs.updatingMu.updating[key] + return ok && rec == generation } func (gs *GlobalStats) markUpdateComplete(key pb.StatsInfoKey, updated bool, actualObjectCount int64, samplingRatio float64) { @@ -722,6 +805,30 @@ func (gs *GlobalStats) markUpdateComplete(key pb.StatsInfoKey, updated bool, act rec = &updateRecord{} gs.updatingMu.updating[key] = rec } + completeUpdateRecord(rec, updated, actualObjectCount, samplingRatio) +} + +// markAutomaticUpdateComplete closes a generation opened by +// shouldExecuteUpdate. Unlike the explicit RefreshWithMode completion path, it +// must not recreate scheduling metadata that table-lifetime cleanup removed +// while an old worker was still unwinding. +func (gs *GlobalStats) markAutomaticUpdateComplete( + key pb.StatsInfoKey, + generation *updateRecord, + updated bool, + actualObjectCount int64, + samplingRatio float64, +) { + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() + rec, ok := gs.updatingMu.updating[key] + if !ok || rec != generation { + return + } + completeUpdateRecord(rec, updated, actualObjectCount, samplingRatio) +} + +func completeUpdateRecord(rec *updateRecord, updated bool, actualObjectCount int64, samplingRatio float64) { rec.inProgress = false // only if the stats is updated, set the update time and reset baseline. if updated { @@ -905,10 +1012,48 @@ func (gs *GlobalStats) broadcastStats(key pb.StatsInfoKey) { }) } +// completeAutomaticStatsCacheUpdate is the only automatic-refresh transition +// for statsInfoMap. A successful generation replaces the published value. A +// failed generation never destroys the last successful value; it installs a +// nil sentinel only when no generation has completed before, so synchronous +// first-read waiters can terminate without treating failure as publication. +func (gs *GlobalStats) completeAutomaticStatsCacheUpdate( + key pb.StatsInfoKey, + generation *updateRecord, + stats *pb.StatsInfo, + updated bool, +) { + gs.mu.Lock() + defer gs.mu.Unlock() + // The update record is also the automatic generation's table-lifetime + // token. RemoveTid deletes it under the same gs.mu -> updatingMu order; an + // old worker that completes afterward must not resurrect either cache. + if !gs.automaticUpdateActive(key, generation) { + gs.mu.cond.Broadcast() + return + } + if updated { + gs.mu.statsInfoMap[key] = stats + gs.broadcastStats(key) + } else if _, ok := gs.mu.statsInfoMap[key]; !ok { + gs.mu.statsInfoMap[key] = nil + } + gs.mu.cond.Broadcast() +} + func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) { + gs.coordinateStatsUpdateJob(statsUpdateJob{ + wrapKey: wrapKey, + expectedRecord: gs.currentUpdateRecord(wrapKey.Key), + }) +} + +func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { + wrapKey := job.wrapKey statser := statistic.StatsInfoFromContext(wrapKey.Ctx) crs := new(perfcounter.CounterSet) - if !gs.shouldExecuteUpdate(wrapKey.Key) { + generation, ok := gs.startAutomaticUpdate(wrapKey.Key, job.expectedRecord) + if !ok { return } @@ -920,21 +1065,20 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) if err != nil { // shouldExecuteUpdate opened this generation before admission. Close it // even when cancellation prevents this worker from acquiring the stripe. - gs.markUpdateComplete(wrapKey.Key, false, 0, 0) + gs.markAutomaticUpdateComplete(wrapKey.Key, generation, false, 0, 0) + return + } + if !gs.automaticUpdateActive(wrapKey.Key, generation) { + // Table cleanup removed this queued generation while it waited for + // admission. Avoid re-subscribing and doing object work for stale state. + release() return } defer func() { gs.completeStatsRefresh( - wrapKey.Key, updated, actualObjectCount, samplingRatio, release) + wrapKey.Key, generation, updated, actualObjectCount, samplingRatio, release) }() - broadcastWithoutUpdate := func() { - gs.mu.Lock() - defer gs.mu.Unlock() - gs.mu.statsInfoMap[wrapKey.Key] = nil - gs.mu.cond.Broadcast() - } - // Get the latest partition state of the table. //Notice that for snapshot read, subscribing the table maybe failed since the invalid table id, //We should handle this case in next PR if needed. @@ -951,7 +1095,7 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) wrapKey.Key.TableID, wrapKey.Key.TableName, err) - broadcastWithoutUpdate() + gs.completeAutomaticStatsCacheUpdate(wrapKey.Key, generation, nil, false) return } stats := plan2.NewStatsInfo() @@ -973,17 +1117,7 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) DeleteMul: crs.FileService.S3.DeleteMulti.Load(), }) - gs.mu.Lock() - defer gs.mu.Unlock() - if updated { - gs.mu.statsInfoMap[wrapKey.Key] = stats - gs.broadcastStats(wrapKey.Key) - } else if _, ok := gs.mu.statsInfoMap[wrapKey.Key]; !ok { - gs.mu.statsInfoMap[wrapKey.Key] = nil - } - - // Notify all the waiters to read the new stats info. - gs.mu.cond.Broadcast() + gs.completeAutomaticStatsCacheUpdate(wrapKey.Key, generation, stats, updated) } // completeStatsRefresh commits the automatic-refresh scheduling metadata @@ -991,12 +1125,13 @@ func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) // object-count/sampling baseline therefore advance in one serialized order. func (gs *GlobalStats) completeStatsRefresh( key pb.StatsInfoKey, + generation *updateRecord, updated bool, actualObjectCount int64, samplingRatio float64, release func(), ) { - gs.markUpdateComplete(key, updated, actualObjectCount, samplingRatio) + gs.markAutomaticUpdateComplete(key, generation, updated, actualObjectCount, samplingRatio) release() } diff --git a/pkg/vm/engine/disttae/stats_test.go b/pkg/vm/engine/disttae/stats_test.go index 75b118d14fdf4..76565f4561ab1 100644 --- a/pkg/vm/engine/disttae/stats_test.go +++ b/pkg/vm/engine/disttae/stats_test.go @@ -1732,18 +1732,61 @@ func TestRemoveTid(t *testing.T) { gs.mu.statsInfoMap[k2] = nil // simulate failed update gs.mu.statsInfoMap[k3] = plan2.NewStatsInfo() gs.mu.Unlock() + gs.markUpdateComplete(k1, true, 1, 1) + gs.markUpdateComplete(k2, false, 0, 0) + gs.markUpdateComplete(k3, true, 2, 1) + generation := gs.currentUpdateRecord(k1) // Remove table 1001 entries gs.RemoveTid(1001) + // A worker admitted before cleanup may finish afterward. Its stale + // publication or completion must not recreate table-owned state. + queuedAfterCleanup, enqueueAfterCleanup := + gs.shouldEnqueueExistingStatsUpdateGeneration(k1, 1, false) + assert.False(t, enqueueAfterCleanup) + assert.Nil(t, queuedAfterCleanup) + gs.completeAutomaticStatsCacheUpdate(k1, generation, plan2.NewStatsInfo(), true) + gs.completeStatsRefresh(k1, generation, true, 3, 1, func() {}) gs.mu.Lock() - defer gs.mu.Unlock() _, ok1 := gs.mu.statsInfoMap[k1] _, ok2 := gs.mu.statsInfoMap[k2] _, ok3 := gs.mu.statsInfoMap[k3] + gs.mu.Unlock() assert.False(t, ok1, "k1 should be removed") assert.False(t, ok2, "k2 should be removed") assert.True(t, ok3, "k3 should not be removed") + + gs.updatingMu.Lock() + _, updating1 := gs.updatingMu.updating[k1] + _, updating2 := gs.updatingMu.updating[k2] + _, updating3 := gs.updatingMu.updating[k3] + gs.updatingMu.Unlock() + assert.False(t, updating1, "k1 scheduling metadata should be removed") + assert.False(t, updating2, "k2 scheduling metadata should be removed") + assert.True(t, updating3, "unrelated scheduling metadata should remain") + + // Reuse of the same table key creates a distinct generation. Neither + // an old queued job nor its late callbacks may publish into it. + replacement := &updateRecord{inProgress: true, pendingChanges: 7} + gs.updatingMu.Lock() + gs.updatingMu.updating[k1] = replacement + gs.updatingMu.Unlock() + gs.completeAutomaticStatsCacheUpdate(k1, generation, plan2.NewStatsInfo(), true) + gs.completeStatsRefresh(k1, generation, true, 4, 0.5, func() {}) + _, oldGenerationStarted := gs.startAutomaticUpdate(k1, generation) + assert.False(t, oldGenerationStarted, "an old queued generation should be rejected") + + gs.mu.Lock() + _, oldStatsPublished := gs.mu.statsInfoMap[k1] + gs.mu.Unlock() + assert.False(t, oldStatsPublished, "an old generation should not publish into its replacement") + gs.updatingMu.Lock() + current := gs.updatingMu.updating[k1] + gs.updatingMu.Unlock() + require.Same(t, replacement, current) + assert.True(t, current.inProgress) + assert.Equal(t, 7, current.pendingChanges) }) }) @@ -1943,9 +1986,10 @@ func TestGlobalStatsGetReturnsWhenContextCanceledWhileWaiting(t *testing.T) { gs := &GlobalStats{ ctx: ctx, engine: e, - updateC: make(chan statsinfo.StatsInfoKeyWithContext, 1), + updateC: make(chan statsUpdateJob, 1), queueWatcher: newQueueWatcher(), } + gs.updatingMu.updating = make(map[statsinfo.StatsInfoKey]*updateRecord) gs.mu.statsInfoMap = make(map[statsinfo.StatsInfoKey]*statsinfo.StatsInfo) gs.mu.cond = sync.NewCond(&gs.mu) @@ -1975,14 +2019,15 @@ func TestGlobalStatsGetReturnsWhenContextCanceledWhileWaiting(t *testing.T) { func TestEnqueueStatsUpdateForceReturnsWhenContextCanceled(t *testing.T) { gs := &GlobalStats{ - updateC: make(chan statsinfo.StatsInfoKeyWithContext, 1), + updateC: make(chan statsUpdateJob, 1), queueWatcher: newQueueWatcher(), } + gs.updatingMu.updating = make(map[statsinfo.StatsInfoKey]*updateRecord) queued := statsinfo.StatsInfoKeyWithContext{ Ctx: context.Background(), Key: statsinfo.StatsInfoKey{TableID: 1}, } - gs.updateC <- queued + gs.updateC <- statsUpdateJob{wrapKey: queued} ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -1991,7 +2036,7 @@ func TestEnqueueStatsUpdateForceReturnsWhenContextCanceled(t *testing.T) { Key: statsinfo.StatsInfoKey{TableID: 2}, }, true) require.False(t, accepted) - require.Equal(t, queued, <-gs.updateC) + require.Equal(t, queued, (<-gs.updateC).wrapKey) } func TestCacheRemoteInfoIfSubscribedBroadcastsWaiters(t *testing.T) { From b6c2359d555c674137a4f8444814f9a9a7f0df8e Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 05:53:42 +0800 Subject: [PATCH 12/18] fix: fence stats refreshes across table cleanup --- docs/design/analyze_stats_publication.md | 26 +++--- pkg/vm/engine/disttae/engine_stats_test.go | 2 +- pkg/vm/engine/disttae/stats.go | 102 +++++++++++++++------ pkg/vm/engine/disttae/stats_test.go | 85 +++++++++++++++-- 4 files changed, 167 insertions(+), 48 deletions(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index 5723a56a607eb..9d6d7303441be 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -102,11 +102,11 @@ It intentionally does not provide: 7. Engine statistics and refresh-scheduling metadata share the table cleanup boundary. Once an unsubscribed table reaches `RemoveTid`, neither `statsInfoMap` nor `updatingMu.updating` retains any key for that table ID, - and a late automatic-refresh publication/completion cannot recreate either - entry or write into a replacement generation. The scheduling-record pointer - captured at enqueue is the automatic generation's lifetime token. Logtail's - cache-existence check and token capture use the same cleanup lock order, so - removal cannot fall between those two producer steps. + and a late automatic or explicit refresh cannot recreate either entry or + write into a replacement generation. Every refresh, including the first + queued request, carries a non-nil scheduling-record pointer as its lifetime + token. Logtail's cache-existence check and token capture use the same cleanup + lock order, so removal cannot fall between those two producer steps. ## 4. Identity and visibility @@ -232,8 +232,10 @@ Cancellation and deadline errors remain cancellation/deadline errors at the public refresh boundary. Other object/metadata failures may be wrapped with table-refresh context, but must remain a failed publication. -Retry starts a fresh refresh and generation attempt. No generation is reserved -before success, so a failed retry creates no gap that consumers must interpret. +Retry starts a fresh refresh attempt. The engine reserves only an internal +table-lifetime token before work starts; the frontend reuse generation is not +advanced until engine publication succeeds, so a failed retry creates no +consumer-visible generation gap. ## 7. Compatibility and operations @@ -328,12 +330,12 @@ checks on affected plan-cache hits. | pre-canceled, in-flight canceled, and shutdown-rejected work terminates | executor/visible-object cancellation UT | | same-table refresh order; unrelated-table concurrency | frontend and engine admission race UT | | failed automatic refresh preserves last-good stats and completes an absent first generation | injected subscribe-failure state-transition UT | -| table cleanup reclaims both statistics and refresh-scheduling entries; late automatic publication/completion cannot recreate them | `RemoveTid` ownership UT | +| table cleanup reclaims both statistics and refresh-scheduling entries; first-queued, late automatic, and late explicit work cannot recreate them or target a replacement lifetime | `RemoveTid` ownership/generation UT | | slow generation-N read cannot overwrite N+1 | session-cache race UT | | plan build spanning publication is not cached | plan-cache generation UT | | physical account/view/temporary/transaction rules | focused frontend table-driven UT and ANALYZE BVT | | no-dependency and 1/4/16-dependency cache-hit cost | allocation/latency benchmarks | -| SQL-visible existing-session plan changes after ANALYZE | real-service BVT and explain-plan assertion | +| SQL-visible existing-session plan changes after ANALYZE | recorded real-service validation and explain-plan assertion | Every new concurrency test uses explicit phase barriers and an outer timeout only as a hang guard. The changed disttae and frontend packages require focused @@ -362,9 +364,9 @@ Decision log: - Preserve the last successful automatic-refresh value on failure; use a nil sentinel only to complete an otherwise absent first generation. - Make `RemoveTid` the common lifetime owner for published engine statistics - and per-table refresh-scheduling metadata; automatic publication and - completion require the update record as a lifetime token, so an old worker - cannot recreate cleanup-owned state. + and per-table refresh-scheduling metadata; every automatic and explicit + refresh requires a non-nil update record as a lifetime token, so old work + cannot recreate cleanup-owned state or target a replacement lifetime. Open approval item: an independent reviewer must approve this exact design revision before the implementation is considered deliverable. There are no diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go index 7d8b03060faf2..fd0c61c26e95d 100644 --- a/pkg/vm/engine/disttae/engine_stats_test.go +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -128,7 +128,7 @@ func TestCompleteStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { newerDone <- acquireErr return } - gs.markUpdateComplete(key, true, 100, 0.5) + gs.markAutomaticUpdateComplete(key, generation, true, 100, 0.5) newRelease() newerDone <- nil }() diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index b85ced22cba78..0515096593542 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -393,10 +393,20 @@ func (gs *GlobalStats) PrefetchTableMeta(ctx context.Context, key pb.StatsInfoKe return gs.enqueueStatsUpdate(wrapkey, false) } -func (gs *GlobalStats) currentUpdateRecord(key pb.StatsInfoKey) *updateRecord { +// currentOrCreateUpdateRecord returns the table-lifetime token that every +// queued or explicit refresh must carry. In particular, the first refresh must +// not use nil as an "expected absence" token: RemoveTid can make absence true +// again, allowing old queued work to cross the cleanup boundary and recreate +// table-owned state. +func (gs *GlobalStats) currentOrCreateUpdateRecord(key pb.StatsInfoKey) *updateRecord { gs.updatingMu.Lock() defer gs.updatingMu.Unlock() - return gs.updatingMu.updating[key] + rec := gs.updatingMu.updating[key] + if rec == nil { + rec = &updateRecord{} + gs.updatingMu.updating[key] = rec + } + return rec } func (gs *GlobalStats) subscribedEntry(key pb.StatsInfoKey) *subEntry { @@ -604,7 +614,8 @@ func (gs *GlobalStats) spawnUpdateWorkers(ctx context.Context, num int) { } func (gs *GlobalStats) enqueueStatsUpdate(key pb.StatsInfoKeyWithContext, force bool) bool { - return gs.enqueueStatsUpdateForRecord(key, force, gs.currentUpdateRecord(key.Key)) + return gs.enqueueStatsUpdateForRecord( + key, force, gs.currentOrCreateUpdateRecord(key.Key)) } func (gs *GlobalStats) enqueueStatsUpdateForRecord( @@ -758,7 +769,7 @@ func (gs *GlobalStats) shouldEnqueueUpdateGenerationLocked( // Only checks inProgress and MinUpdateInterval. // Change rate is NOT checked here (already checked in shouldEnqueueUpdate). func (gs *GlobalStats) shouldExecuteUpdate(key pb.StatsInfoKey) bool { - _, ok := gs.startAutomaticUpdate(key, nil) + _, ok := gs.startAutomaticUpdate(key, gs.currentOrCreateUpdateRecord(key)) return ok } @@ -766,19 +777,15 @@ func (gs *GlobalStats) startAutomaticUpdate( key pb.StatsInfoKey, expectedRecord *updateRecord, ) (*updateRecord, bool) { + if expectedRecord == nil { + return nil, false + } gs.updatingMu.Lock() defer gs.updatingMu.Unlock() rec, ok := gs.updatingMu.updating[key] - if expectedRecord != nil && (!ok || rec != expectedRecord) { + if !ok || rec != expectedRecord { return nil, false } - if !ok { - rec = &updateRecord{ - inProgress: true, - } - gs.updatingMu.updating[key] = rec - return rec, true - } if rec.inProgress { return nil, false } @@ -789,23 +796,32 @@ func (gs *GlobalStats) startAutomaticUpdate( return nil, false } -func (gs *GlobalStats) automaticUpdateActive(key pb.StatsInfoKey, generation *updateRecord) bool { +func (gs *GlobalStats) statsUpdateGenerationActive(key pb.StatsInfoKey, generation *updateRecord) bool { gs.updatingMu.Lock() defer gs.updatingMu.Unlock() rec, ok := gs.updatingMu.updating[key] return ok && rec == generation } -func (gs *GlobalStats) markUpdateComplete(key pb.StatsInfoKey, updated bool, actualObjectCount int64, samplingRatio float64) { +// markExplicitUpdateComplete advances the refresh baseline without stealing +// the in-progress bit from an automatic refresh that was admitted before the +// explicit refresh acquired the shared table stripe. +func (gs *GlobalStats) markExplicitUpdateComplete( + key pb.StatsInfoKey, + generation *updateRecord, + actualObjectCount int64, + samplingRatio float64, +) { gs.updatingMu.Lock() defer gs.updatingMu.Unlock() rec, ok := gs.updatingMu.updating[key] - if !ok { - // set new record for RefreshWithMode - rec = &updateRecord{} - gs.updatingMu.updating[key] = rec + if !ok || rec != generation { + return } - completeUpdateRecord(rec, updated, actualObjectCount, samplingRatio) + rec.lastUpdate = time.Now() + rec.baseObjectCount = actualObjectCount + rec.pendingChanges = 0 + rec.samplingRatio = samplingRatio } // markAutomaticUpdateComplete closes a generation opened by @@ -1028,7 +1044,7 @@ func (gs *GlobalStats) completeAutomaticStatsCacheUpdate( // The update record is also the automatic generation's table-lifetime // token. RemoveTid deletes it under the same gs.mu -> updatingMu order; an // old worker that completes afterward must not resurrect either cache. - if !gs.automaticUpdateActive(key, generation) { + if !gs.statsUpdateGenerationActive(key, generation) { gs.mu.cond.Broadcast() return } @@ -1044,7 +1060,7 @@ func (gs *GlobalStats) completeAutomaticStatsCacheUpdate( func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) { gs.coordinateStatsUpdateJob(statsUpdateJob{ wrapKey: wrapKey, - expectedRecord: gs.currentUpdateRecord(wrapKey.Key), + expectedRecord: gs.currentOrCreateUpdateRecord(wrapKey.Key), }) } @@ -1068,7 +1084,7 @@ func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { gs.markAutomaticUpdateComplete(wrapKey.Key, generation, false, 0, 0) return } - if !gs.automaticUpdateActive(wrapKey.Key, generation) { + if !gs.statsUpdateGenerationActive(wrapKey.Key, generation) { // Table cleanup removed this queued generation while it waited for // admission. Avoid re-subscribing and doing object work for stale state. release() @@ -1155,6 +1171,7 @@ func (gs *GlobalStats) refreshStatsWithMode( return nil, err } defer release() + generation := gs.currentOrCreateUpdateRecord(key) // Get partition state ps, err := gs.engine.pClient.toSubscribeTable( @@ -1210,16 +1227,45 @@ func (gs *GlobalStats) refreshStatsWithMode( return nil, cause } - // Update cache + if !gs.publishStatsForGeneration(key, generation, stats) { + return nil, moerr.NewInternalErrorNoCtxf( + "table statistics refresh crossed cleanup boundary for table %d", key.TableID) + } + // Record the baseline only if this exact table lifetime is still current. + // Preserve a concurrently admitted automatic refresh's in-progress bit. + gs.markExplicitUpdateComplete( + key, generation, stats.AccurateObjectNumber, samplingRatio) + + return stats, nil +} + +// publishStatsForGeneration replaces the cache only while the exact table +// lifetime captured by the refresh remains current. The gs.mu -> updatingMu +// order matches RemoveTid, making validation and publication atomic with +// respect to cleanup. A late explicit refresh therefore cannot resurrect an +// unsubscribed table or publish into a replacement generation. +func (gs *GlobalStats) publishStatsForGeneration( + key pb.StatsInfoKey, + generation *updateRecord, + stats *pb.StatsInfo, +) bool { + if generation == nil || stats == nil { + return false + } gs.mu.Lock() defer gs.mu.Unlock() + if !gs.statsUpdateGenerationActive(key, generation) { + if gs.mu.cond != nil { + gs.mu.cond.Broadcast() + } + return false + } gs.mu.statsInfoMap[key] = stats gs.broadcastStats(key) - gs.mu.cond.Broadcast() - // Record sampling ratio in updateRecord - gs.markUpdateComplete(key, true, stats.AccurateObjectNumber, samplingRatio) - - return stats, nil + if gs.mu.cond != nil { + gs.mu.cond.Broadcast() + } + return true } func (gs *GlobalStats) executeStatsUpdate(ctx context.Context, ps *logtailreplay.PartitionState, key pb.StatsInfoKey, stats *pb.StatsInfo) (bool, float64) { diff --git a/pkg/vm/engine/disttae/stats_test.go b/pkg/vm/engine/disttae/stats_test.go index 76565f4561ab1..311b3f44b99f6 100644 --- a/pkg/vm/engine/disttae/stats_test.go +++ b/pkg/vm/engine/disttae/stats_test.go @@ -233,7 +233,8 @@ func TestGlobalStats_ShouldUpdate(t *testing.T) { } assert.True(t, gs.shouldExecuteUpdate(k1)) assert.False(t, gs.shouldExecuteUpdate(k1)) - gs.markUpdateComplete(k1, true, 1, 1.0) + gs.markAutomaticUpdateComplete( + k1, gs.currentOrCreateUpdateRecord(k1), true, 1, 1.0) time.Sleep(MinUpdateInterval) assert.True(t, gs.shouldExecuteUpdate(k1)) }) @@ -260,7 +261,8 @@ func TestGlobalStats_ShouldUpdate(t *testing.T) { return } count.Add(1) - gs.markUpdateComplete(k1, true, 2, 1.0) + gs.markAutomaticUpdateComplete( + k1, gs.currentOrCreateUpdateRecord(k1), true, 2, 1.0) } for i := 0; i < 20; i++ { wg.Add(1) @@ -1267,7 +1269,8 @@ func TestSamplingForceAtLeastOneObject(t *testing.T) { // by calling shouldEnqueueUpdate once and then markUpdateComplete to set the baseObjectCount func initTableForTest(gs *GlobalStats, key statsinfo.StatsInfoKey, baseObjectCount int64) { gs.shouldEnqueueUpdate(key, 0, false) - gs.markUpdateComplete(key, true, baseObjectCount, 1.0) + gs.markExplicitUpdateComplete( + key, gs.currentOrCreateUpdateRecord(key), baseObjectCount, 1.0) } // TestGlobalStats_ShouldEnqueue tests the shouldEnqueue logic for large table throttling @@ -1732,10 +1735,14 @@ func TestRemoveTid(t *testing.T) { gs.mu.statsInfoMap[k2] = nil // simulate failed update gs.mu.statsInfoMap[k3] = plan2.NewStatsInfo() gs.mu.Unlock() - gs.markUpdateComplete(k1, true, 1, 1) - gs.markUpdateComplete(k2, false, 0, 0) - gs.markUpdateComplete(k3, true, 2, 1) - generation := gs.currentUpdateRecord(k1) + generation := gs.currentOrCreateUpdateRecord(k1) + gs.currentOrCreateUpdateRecord(k2) + gs.currentOrCreateUpdateRecord(k3) + gs.markExplicitUpdateComplete(k1, generation, 1, 1) + gs.markAutomaticUpdateComplete( + k2, gs.currentOrCreateUpdateRecord(k2), false, 0, 0) + gs.markExplicitUpdateComplete( + k3, gs.currentOrCreateUpdateRecord(k3), 2, 1) // Remove table 1001 entries gs.RemoveTid(1001) @@ -1790,6 +1797,70 @@ func TestRemoveTid(t *testing.T) { }) }) + t.Run("first_queued_and_explicit_refreshes_cannot_cross_cleanup_generation", func(t *testing.T) { + gs := &GlobalStats{ + updateC: make(chan statsUpdateJob, 1), + queueWatcher: newQueueWatcher(), + } + gs.updatingMu.updating = make(map[statsinfo.StatsInfoKey]*updateRecord) + gs.mu.statsInfoMap = make(map[statsinfo.StatsInfoKey]*statsinfo.StatsInfo) + gs.mu.cond = sync.NewCond(&gs.mu) + + key := statsinfo.StatsInfoKey{DatabaseID: 100, TableID: 1001, TableName: "t1"} + require.True(t, gs.enqueueStatsUpdate(statsinfo.StatsInfoKeyWithContext{ + Ctx: context.Background(), + Key: key, + }, false)) + job := <-gs.updateC + require.NotNil(t, job.expectedRecord, + "the first queued refresh must own a concrete lifetime token") + + gs.RemoveTid(key.TableID) + gs.coordinateStatsUpdateJob(job) + gs.markAutomaticUpdateComplete(key, job.expectedRecord, true, 1, 1) + assert.False(t, gs.publishStatsForGeneration( + key, job.expectedRecord, plan2.NewStatsInfo())) + + gs.mu.Lock() + _, cached := gs.mu.statsInfoMap[key] + gs.mu.Unlock() + gs.updatingMu.Lock() + _, scheduled := gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + assert.False(t, cached, "old work must not recreate the statistics cache") + assert.False(t, scheduled, "old work must not recreate scheduling metadata") + + replacement := gs.currentOrCreateUpdateRecord(key) + assert.False(t, gs.publishStatsForGeneration( + key, job.expectedRecord, plan2.NewStatsInfo()), + "old explicit work must not publish into a replacement lifetime") + fresh := plan2.NewStatsInfo() + fresh.TableCnt = 42 + require.True(t, gs.publishStatsForGeneration(key, replacement, fresh)) + gs.mu.Lock() + assert.Same(t, fresh, gs.mu.statsInfoMap[key]) + gs.mu.Unlock() + }) + + t.Run("explicit_completion_preserves_admitted_automatic_refresh", func(t *testing.T) { + gs := &GlobalStats{} + gs.updatingMu.updating = make(map[statsinfo.StatsInfoKey]*updateRecord) + key := statsinfo.StatsInfoKey{DatabaseID: 100, TableID: 1001, TableName: "t1"} + generation := &updateRecord{inProgress: true, pendingChanges: 7} + gs.updatingMu.updating[key] = generation + + gs.markExplicitUpdateComplete(key, generation, 42, 0.5) + + gs.updatingMu.Lock() + got := *gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + assert.True(t, got.inProgress, + "explicit completion must not reopen admission for another automatic refresh") + assert.Equal(t, int64(42), got.baseObjectCount) + assert.Zero(t, got.pendingChanges) + assert.Equal(t, 0.5, got.samplingRatio) + }) + t.Run("remove_nonexistent_table", func(t *testing.T) { runTest(t, func(ctx context.Context, e *Engine) { gs := e.globalStats From 26a2ff078afcdbe416c2d31306e5f0c23d3e68a4 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 09:05:05 +0800 Subject: [PATCH 13/18] fix: close stats refresh shutdown gaps --- docs/design/analyze_stats_publication.md | 17 ++++- pkg/vm/engine/disttae/engine_stats_test.go | 73 ++++++++++++++++++++++ pkg/vm/engine/disttae/stats.go | 37 ++++++++++- pkg/vm/engine/disttae/txn_table.go | 7 +++ pkg/vm/engine/disttae/util_test.go | 39 ++++++++++++ 5 files changed, 169 insertions(+), 4 deletions(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index 9d6d7303441be..9148e9807b599 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -107,6 +107,13 @@ It intentionally does not provide: queued request, carries a non-nil scheduling-record pointer as its lifetime token. Logtail's cache-existence check and token capture use the same cleanup lock order, so removal cannot fall between those two producer steps. +8. An explicit refresh creates its table-lifetime token only after subscription + and catalog resolution succeed, while holding the subscription lifecycle + read lock. A failed subscription therefore retains no scheduling entry, and + unsubscribe cleanup cannot fall between validation and token capture. +9. A first statistics read whose subscription fails returns without enqueueing + automatic work. Retrying through the worker queue would create cache and + scheduling state before any subscription lifetime can own its cleanup. ## 4. Identity and visibility @@ -208,7 +215,8 @@ The terminal behavior is: | --- | --- | --- | --- | --- | | derived query fails | unchanged | unchanged | unchanged | error | | frontend admission canceled | unchanged | unchanged | unchanged | cancellation | -| explicit subscribe/catalog resolution fails | unchanged | unchanged | unchanged | error | +| explicit subscribe/catalog resolution fails | unchanged | no generation retained before a cleanup owner exists | unchanged | error | +| initial statistics-read subscription fails | unchanged | no automatic work or generation admitted | unchanged | no statistics | | automatic subscribe/catalog resolution fails | last-good entry retained; nil completion sentinel only when absent | failed generation closed | unchanged | not an ANALYZE result | | task submission canceled/rejected | unchanged | failed generation closed | unchanged | error | | object task fails or is canceled | unchanged; local partial object discarded | failed generation closed | unchanged | error | @@ -226,7 +234,9 @@ error and waits for all admitted work before returning. Waiting is required because callbacks mutate a refresh-local accumulator; returning early would let old work race a discarded accumulator. Callback I/O receives the request context, so cancellation terminates the expensive work without polling or -sleeps. +sleeps. After joining all admitted work, traversal also checks the shared task +context itself: executor shutdown remains a failed traversal even if a running +callback ignored cancellation and returned `nil`. Cancellation and deadline errors remain cancellation/deadline errors at the public refresh boundary. Other object/metadata failures may be wrapped with @@ -328,9 +338,12 @@ checks on affected plan-cache hits. | failed engine refresh does not advance/cache | frontend publisher failure UT | | one successful and one failed object task rejects partial stats | concurrent visible-object UT | | pre-canceled, in-flight canceled, and shutdown-rejected work terminates | executor/visible-object cancellation UT | +| shutdown cannot become success when an in-flight callback returns nil | executor-lifecycle traversal UT | | same-table refresh order; unrelated-table concurrency | frontend and engine admission race UT | | failed automatic refresh preserves last-good stats and completes an absent first generation | injected subscribe-failure state-transition UT | | table cleanup reclaims both statistics and refresh-scheduling entries; first-queued, late automatic, and late explicit work cannot recreate them or target a replacement lifetime | `RemoveTid` ownership/generation UT | +| failed explicit subscription creates no ownerless scheduling generation | injected subscribe-failure UT | +| failed initial-read subscription queues no ownerless automatic generation | injected subscribe-failure UT | | slow generation-N read cannot overwrite N+1 | session-cache race UT | | plan build spanning publication is not cached | plan-cache generation UT | | physical account/view/temporary/transaction rules | focused frontend table-driven UT and ANALYZE BVT | diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go index fd0c61c26e95d..0849b002fc60a 100644 --- a/pkg/vm/engine/disttae/engine_stats_test.go +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -19,6 +19,7 @@ import ( "errors" "sync" "testing" + "time" "github.com/matrixorigin/matrixone/pkg/objectio" pb "github.com/matrixorigin/matrixone/pkg/pb/statsinfo" @@ -202,3 +203,75 @@ func TestCoordinateStatsUpdateSubscribeFailurePreservesLastPublishedStats(t *tes require.Nil(t, got) }) } + +func TestExplicitStatsRefreshSubscribeFailureDoesNotRetainGeneration(t *testing.T) { + key := pb.StatsInfoKey{ + AccId: 1, DatabaseID: 10, TableID: 42, DbName: "db", TableName: "events", + } + gs := &GlobalStats{engine: &Engine{}} + gs.initStatsRefreshAdmission() + gs.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) + + fault.Enable() + t.Cleanup(func() { fault.Disable() }) + removeFault, err := objectio.InjectLogging( + objectio.FJ_CNSubscribeTableFail, key.DbName, key.TableName, 0, true, + ) + require.NoError(t, err) + t.Cleanup(removeFault) + + stats, err := gs.refreshStatsWithMode(context.Background(), key, "auto") + require.Error(t, err) + require.Nil(t, stats) + + gs.updatingMu.Lock() + _, retained := gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + require.False(t, retained, + "a failed subscription has no cleanup owner and must not create a generation") +} + +func TestInitialStatsGetSubscribeFailureDoesNotQueueOwnerlessGeneration(t *testing.T) { + key := pb.StatsInfoKey{ + AccId: 1, DatabaseID: 10, TableID: 42, DbName: "db", TableName: "events", + } + gs := &GlobalStats{ + engine: &Engine{}, + updateC: make(chan statsUpdateJob, 1), + queueWatcher: newQueueWatcher(), + } + gs.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) + gs.mu.statsInfoMap = make(map[pb.StatsInfoKey]*pb.StatsInfo) + gs.mu.cond = sync.NewCond(&gs.mu) + + fault.Enable() + t.Cleanup(func() { fault.Disable() }) + removeFault, err := objectio.InjectLogging( + objectio.FJ_CNSubscribeTableFail, key.DbName, key.TableName, 0, true, + ) + require.NoError(t, err) + t.Cleanup(removeFault) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + result := make(chan *pb.StatsInfo, 1) + go func() { result <- gs.Get(ctx, key, true) }() + + select { + case got := <-result: + require.Nil(t, got) + case <-gs.updateC: + cancel() + <-result + t.Fatal("a failed initial subscription queued work without a cleanup owner") + case <-time.After(time.Second): + cancel() + t.Fatal("stats get did not terminate after subscription failure") + } + + gs.updatingMu.Lock() + _, retained := gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + require.False(t, retained) + require.Empty(t, gs.updateC) +} diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index 0515096593542..b46ca1e690d5b 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -409,6 +409,25 @@ func (gs *GlobalStats) currentOrCreateUpdateRecord(key pb.StatsInfoKey) *updateR return rec } +// currentOrCreateSubscribedUpdateRecord captures the scheduling generation +// only while the subscription that owns its cleanup is still current. Explicit +// refreshes call this after toSubscribeTable succeeds, so a failed subscription +// cannot leave metadata that no unsubscribe path can reclaim. Holding the +// subscription read lock across record creation also prevents cleanup from +// falling between subscription validation and token capture. +func (gs *GlobalStats) currentOrCreateSubscribedUpdateRecord( + key pb.StatsInfoKey, +) (*updateRecord, bool) { + gs.engine.pClient.subscribed.rw.RLock() + defer gs.engine.pClient.subscribed.rw.RUnlock() + + ent, ok := gs.engine.pClient.subscribed.m[key.TableID] + if !ok || ent == nil || ent.dbID != key.DatabaseID || ent.state != Subscribed { + return nil, false + } + return gs.currentOrCreateUpdateRecord(key), true +} + func (gs *GlobalStats) subscribedEntry(key pb.StatsInfoKey) *subEntry { gs.engine.pClient.subscribed.rw.RLock() defer gs.engine.pClient.subscribed.rw.RUnlock() @@ -482,7 +501,13 @@ func (gs *GlobalStats) Get(ctx context.Context, key pb.StatsInfoKey, sync bool) key.DatabaseID, key.DbName) - if err == nil && ps.ApproxDataObjectsNum() == 0 { + if err != nil { + // A failed initial subscription has no table-lifetime cleanup owner. + // Retrying through updateC would create a scheduling generation (and + // potentially a nil cache sentinel) that RemoveTid can never reclaim. + return nil + } + if ps.ApproxDataObjectsNum() == 0 { return nil } @@ -1171,7 +1196,6 @@ func (gs *GlobalStats) refreshStatsWithMode( return nil, err } defer release() - generation := gs.currentOrCreateUpdateRecord(key) // Get partition state ps, err := gs.engine.pClient.toSubscribeTable( @@ -1191,6 +1215,15 @@ func (gs *GlobalStats) refreshStatsWithMode( return nil, moerr.NewInternalErrorNoCtx("table not found") } + // The subscription owns eventual RemoveTid cleanup. Capture the refresh + // generation only after subscription and catalog resolution succeed, and + // only while that exact subscription lifetime is still current. + generation, ok := gs.currentOrCreateSubscribedUpdateRecord(key) + if !ok { + return nil, moerr.NewInternalErrorNoCtxf( + "table statistics refresh crossed subscription boundary for table %d", key.TableID) + } + // Create stats info stats := plan2.NewStatsInfo() approxObjectNum := int64(ps.ApproxDataObjectsNum()) diff --git a/pkg/vm/engine/disttae/txn_table.go b/pkg/vm/engine/disttae/txn_table.go index dfa193fe23463..524a4416dce14 100644 --- a/pkg/vm/engine/disttae/txn_table.go +++ b/pkg/vm/engine/disttae/txn_table.go @@ -441,6 +441,13 @@ func ForeachVisibleObjects( if firstErr != nil { return firstErr } + // Executor shutdown is a failed traversal even when a running callback + // ignores taskCtx and happens to return nil. Without this check, the + // caller can publish a partial accumulator after the executor lifecycle + // has already canceled the work group. + if cause := context.Cause(taskCtx); cause != nil { + return cause + } } if err != nil { return err diff --git a/pkg/vm/engine/disttae/util_test.go b/pkg/vm/engine/disttae/util_test.go index 212c9a2d2ffb7..c970f0a064157 100644 --- a/pkg/vm/engine/disttae/util_test.go +++ b/pkg/vm/engine/disttae/util_test.go @@ -879,6 +879,45 @@ func TestForeachVisibleObjectsCancelsInFlightTaskOnExecutorShutdown(t *testing.T } } +func TestForeachVisibleObjectsRejectsExecutorShutdownWhenTaskReturnsNil(t *testing.T) { + state := visibleObjectStateForExecutorTest(t, 1) + ex := newConcurrentExecutor(1) + executorCtx, stopExecutor := context.WithCancel(context.Background()) + t.Cleanup(stopExecutor) + ex.Run(executorCtx) + + taskStarted := make(chan struct{}) + result := make(chan error, 1) + go func() { + result <- ForeachVisibleObjects( + context.Background(), state, types.MaxTs(), + func(taskCtx context.Context, _ objectio.ObjectEntry) error { + close(taskStarted) + <-taskCtx.Done() + // Model a callback that observes shutdown only as a release + // signal and fails to propagate the context error itself. + return nil + }, + ex, + false, + ) + }() + + select { + case <-taskStarted: + case <-time.After(time.Second): + t.Fatal("executor did not start the admitted visible-object task") + } + stopExecutor() + select { + case err := <-result: + require.ErrorIs(t, err, context.Canceled, + "executor shutdown must remain visible even when a callback returns nil") + case <-time.After(time.Second): + t.Fatal("visible-object traversal did not join the shutdown task") + } +} + func TestCollectAndCalculateStatsDoesNotApplyFailedObjectScan(t *testing.T) { ctx := context.Background() state := logtailreplay.NewPartitionState("", true, 42, false) From d9938413a5948bdcc63ca747528ab7cff47f23a1 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 09:11:14 +0800 Subject: [PATCH 14/18] docs: record stats publication design gate --- docs/design/analyze_stats_publication.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index 9148e9807b599..413bacc822086 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -1,6 +1,6 @@ # ANALYZE Statistics Publication and Plan-Cache Freshness -- Status: implementation record; ordinary bug-fix design gate exempt +- Status: mandatory design review pending (stateful cache/concurrency lifecycle) - Tracking issue: [matrixorigin/matrixone#27728](https://github.com/matrixorigin/matrixone/issues/27728) - Implementation PR: [matrixorigin/matrixone#27758](https://github.com/matrixorigin/matrixone/pull/27758) - Last updated: 2026-08-28 From ec522c305ce6cd0714c36b36dcfcd8eff0e79322 Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 10:37:51 +0800 Subject: [PATCH 15/18] fix(disttae): close stats refresh waiter lifecycle --- docs/design/analyze_stats_publication.md | 90 +++- pkg/vm/engine/disttae/engine_stats_test.go | 18 +- pkg/vm/engine/disttae/stats.go | 285 +++++++++--- pkg/vm/engine/disttae/stats_test.go | 501 +++++++++++++++++---- 4 files changed, 726 insertions(+), 168 deletions(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index 413bacc822086..fa2db25334c73 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -84,6 +84,17 @@ It intentionally does not provide: orphaned request, or a caller waiting for abandoned queued work. 4. Unrelated tables normally remain parallel. A bounded hash-stripe collision may serialize refresh control work but cannot affect query execution. +5. A synchronous first-read waiter may sleep only while the exact refresh + generation it enqueued is still owned by the current table subscription and + has at least one queued or running producer. Cache completion, context + cancellation, worker-lifecycle shutdown, producer exhaustion, and generation + removal are durable predicates checked while holding `GlobalStats.mu`; + `cond.Wait` atomically releases that same mutex. A notification is never + treated as the predicate. +6. A producer may create or capture a refresh generation only while a live + subscription owns its eventual cleanup. Failed queue admission may leave an + idle record for that live subscription, but it cannot create process-lifetime + metadata for an unsubscribed table and no waiter may depend on a rejected job. ### 3.3 Boundedness @@ -114,6 +125,10 @@ It intentionally does not provide: 9. A first statistics read whose subscription fails returns without enqueueing automatic work. Retrying through the worker queue would create cache and scheduling state before any subscription lifetime can own its cleanup. +10. Prefetch and synchronous first-read producers capture their scheduling + token while holding the subscription lifecycle read lock. A first-read also + requires the exact `subEntry` observed after subscription, so cleanup and a + replacement subscription cannot silently retarget old work. ## 4. Identity and visibility @@ -173,7 +188,50 @@ The frontend point intentionally follows the engine point. Before generation advancement, an old plan may still run against the old generation. After it, new cache admission and later cache hits must reject that old generation. -### 5.2 Automatic refresh interaction +### 5.2 Synchronous first-read wait protocol + +`GlobalStats.Get(sync=true)` uses a level-triggered predicate rather than an +edge-triggered wakeup contract: + +```text +subscribe and capture exact subEntry + -> under subscription RLock, capture/create exact updateRecord + -> register and enqueue a job carrying that updateRecord + -> lock GlobalStats.mu + -> if cache key exists (value or nil sentinel), return it + -> if caller context is done, return nil + -> under GlobalStats.mu -> updatingMu, if updateRecord is no longer current + or has no queued/running producer, return nil + -> cond.Wait (atomically publish waiter and release GlobalStats.mu) + -> re-evaluate all predicates +``` + +`RemoveTid` takes `GlobalStats.mu -> updatingMu`, removes both the cache entry +and generation, then broadcasts before releasing `GlobalStats.mu`. Therefore +cleanup either happens before the waiter checks the generation, in which case +the durable generation predicate terminates it, or after `cond.Wait` has +atomically registered the waiter, in which case the broadcast wakes it. There +is no broadcast-before-wait gap. A stale worker may reject its generation +silently; progress never depends on that stale worker issuing another wakeup. +Each enqueue attempt increments the generation's queued-producer count before +it can become visible to a waiter; a forced sender blocked on a full queue is +therefore also a live producer with caller and lifecycle cancellation. Worker +admission atomically transfers that count to `inProgress` or consumes a +coalesced/rejected job. Queue rollback and +every path that removes the final running producer broadcast after persisting +the zero-producer predicate. Thus a waiter cannot depend on a job that was +accepted by the channel but later abandoned by debounce, coalescing, or a +different caller's cancellation. + +Context cancellation uses the same rule: its callback acquires +`GlobalStats.mu` before broadcasting, so cancellation cannot fall between the +predicate check and waiter registration. A forced queue submission that is +rejected or canceled is never followed by a wait. +The `GlobalStats` lifecycle context is an independent terminal predicate for +both a forced queue submission and an already parked waiter. Stopping update +workers therefore cannot strand a request whose caller context remains live. + +### 5.3 Automatic refresh interaction Automatic logtail refresh and explicit ANALYZE share the engine stripe. An automatic refresh commits its statistics entry and object-count/sampling @@ -195,7 +253,7 @@ Hash collisions deliberately trade rare refresh serialization for fixed memory. They do not merge table identity: engine maps, generations, session tags, and plan dependencies remain keyed by the full physical key. -### 5.3 Plan and session-cache admission +### 5.4 Plan and session-cache admission Planning records the first generation observed for each physical table. A generation change during repeated reads makes the completed plan ineligible for @@ -217,6 +275,12 @@ The terminal behavior is: | frontend admission canceled | unchanged | unchanged | unchanged | cancellation | | explicit subscribe/catalog resolution fails | unchanged | no generation retained before a cleanup owner exists | unchanged | error | | initial statistics-read subscription fails | unchanged | no automatic work or generation admitted | unchanged | no statistics | +| prefetch outside a live subscription | unchanged | no generation admitted | unchanged | rejected | +| forced first-read queue admission canceled/rejected | unchanged | live-subscription record may remain for reuse and eventual cleanup; no waiter admitted | unchanged | no statistics | +| shared automatic producer canceled before publication | unchanged | queued/running producer count reaches zero and wakes all coalesced waiters | unchanged | no statistics | +| statistics worker lifecycle stops with queued work | unchanged | waiter terminates on lifecycle predicate; process-owned record dies with `GlobalStats` | unchanged | no statistics | +| subscription cleanup before first-read waiter parks | removed | exact generation removed; durable predicate terminates waiter | unchanged | no statistics | +| subscription cleanup after first-read waiter parks | removed | exact generation removed; cleanup broadcast terminates waiter | unchanged | no statistics | | automatic subscribe/catalog resolution fails | last-good entry retained; nil completion sentinel only when absent | failed generation closed | unchanged | not an ANALYZE result | | task submission canceled/rejected | unchanged | failed generation closed | unchanged | error | | object task fails or is canceled | unchanged; local partial object discarded | failed generation closed | unchanged | error | @@ -278,10 +342,13 @@ cross-CN extension. The common TP path with no recorded statistics dependency performs no new generation-map read. A dependent cache hit takes one process-local read lock and O(number of referenced physical tables) comparisons, with no allocations. The -local Apple M4 evidence at implementation revision `28acbd4cc7` measured -1.77-1.83 ns for zero dependencies, 35.21-35.47 ns for one, 46.23-47.95 ns for -four, and 115.6-128.7 ns for sixteen, all with zero allocations. These values -are directional microbenchmark evidence, not a production latency SLO. +local Apple M4 evidence at implementation revision `cb2327fd90` measured +1.773-1.785 ns for zero dependencies, 34.95-36.45 ns for one, 47.20-48.10 ns +for four, and 121.5-123.1 ns for sixteen, all with zero allocations. The waiter +repair does not touch that path; its producer accounting adds one integer under +the existing scheduling mutex only when a statistics refresh is enqueued. +These values are directional microbenchmark evidence, not a production latency +SLO. ANALYZE adds a synchronous disttae object-metadata scan after its derived query. This increases ANALYZE latency and S3 reads but moves the cost off the normal TP @@ -344,6 +411,14 @@ checks on affected plan-cache hits. | table cleanup reclaims both statistics and refresh-scheduling entries; first-queued, late automatic, and late explicit work cannot recreate them or target a replacement lifetime | `RemoveTid` ownership/generation UT | | failed explicit subscription creates no ownerless scheduling generation | injected subscribe-failure UT | | failed initial-read subscription queues no ownerless automatic generation | injected subscribe-failure UT | +| synchronous first read returns the exact value published by its accepted producer | producer-publication UT | +| prefetch outside a live subscription creates no ownerless generation | focused ownership UT | +| cleanup before synchronous waiter registration cannot lose a wake | queue/admission phase-barrier UT | +| cleanup after synchronous waiter registration terminates the wait | condition-registration phase-barrier UT | +| caller cancellation after synchronous waiter registration terminates the wait | condition-registration phase-barrier UT | +| coalesced waiter terminates when another caller's producer is canceled at refresh admission | producer-transfer phase-barrier UT | +| synchronous waiter terminates when the statistics worker lifecycle stops | worker-lifecycle phase-barrier UT | +| replacement subscription rejects work captured from the old `subEntry` | subscription-generation UT | | slow generation-N read cannot overwrite N+1 | session-cache race UT | | plan build spanning publication is not cached | plan-cache generation UT | | physical account/view/temporary/transaction rules | focused frontend table-driven UT and ANALYZE BVT | @@ -380,6 +455,9 @@ Decision log: and per-table refresh-scheduling metadata; every automatic and explicit refresh requires a non-nil update record as a lifetime token, so old work cannot recreate cleanup-owned state or target a replacement lifetime. +- Treat condition-variable broadcasts only as hints. The cache/context/exact + generation predicates are checked under the condition mutex, and every + producer generation is captured under a live subscription cleanup owner. Open approval item: an independent reviewer must approve this exact design revision before the implementation is considered deliverable. There are no diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go index 0849b002fc60a..9d2f4cff5bd39 100644 --- a/pkg/vm/engine/disttae/engine_stats_test.go +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -101,7 +101,11 @@ func TestCoordinateStatsUpdateCancellationReleasesUpdateGeneration(t *testing.T) canceled, cancel := context.WithCancel(context.Background()) cancel() - gs.coordinateStatsUpdate(pb.StatsInfoKeyWithContext{Ctx: canceled, Key: key}) + generation := gs.currentOrCreateUpdateRecord(key) + gs.coordinateStatsUpdateJob(statsUpdateJob{ + wrapKey: pb.StatsInfoKeyWithContext{Ctx: canceled, Key: key}, + expectedRecord: generation, + }) gs.updatingMu.Lock() record := gs.updatingMu.updating[key] @@ -180,7 +184,11 @@ func TestCoordinateStatsUpdateSubscribeFailurePreservesLastPublishedStats(t *tes lastGood := newStats() gs.mu.statsInfoMap[key] = lastGood - gs.coordinateStatsUpdate(pb.StatsInfoKeyWithContext{Ctx: context.Background(), Key: key}) + generation := gs.currentOrCreateUpdateRecord(key) + gs.coordinateStatsUpdateJob(statsUpdateJob{ + wrapKey: pb.StatsInfoKeyWithContext{Ctx: context.Background(), Key: key}, + expectedRecord: generation, + }) gs.mu.Lock() got, exists := gs.mu.statsInfoMap[key] @@ -193,7 +201,11 @@ func TestCoordinateStatsUpdateSubscribeFailurePreservesLastPublishedStats(t *tes t.Run("complete first failed generation", func(t *testing.T) { gs := newGlobalStats() - gs.coordinateStatsUpdate(pb.StatsInfoKeyWithContext{Ctx: context.Background(), Key: key}) + generation := gs.currentOrCreateUpdateRecord(key) + gs.coordinateStatsUpdateJob(statsUpdateJob{ + wrapKey: pb.StatsInfoKeyWithContext{Ctx: context.Background(), Key: key}, + expectedRecord: generation, + }) gs.mu.Lock() got, exists := gs.mu.statsInfoMap[key] diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index b46ca1e690d5b..a167141fa974f 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -66,10 +66,10 @@ import ( // spawnUpdateWorkers (16-27个 goroutine) // │ // │ 判断执行条件（第二层）： 便于统一 debounce force/normal update request -// │ - shouldExecuteUpdate(): 检查 inProgress 和 MinUpdateInterval (15s) +// │ - startAutomaticUpdate(): 检查 generation、inProgress 和 MinUpdateInterval (15s) // │ // ▼ -// coordinateStatsUpdate() +// coordinateStatsUpdateJob() // │ // ├─→ 订阅表获取 PartitionState // ├─→ 从 CatalogCache 获取 TableDef @@ -202,6 +202,11 @@ func WithApproxObjectNumUpdater(f func() int64) GlobalStatsOption { // updateRecord records the update status of a key. type updateRecord struct { + // queued is the number of registered enqueue attempts that have not reached + // worker admission or rolled back. It includes a forced sender blocked on a + // full queue. Together with inProgress it is the durable predicate that + // proves a synchronous waiter still has a producer. + queued int // inProgress indicates if the stats of a table is being updated. inProgress bool // lastUpdate is the time of the stats last updated. @@ -220,6 +225,9 @@ type updateRecord struct { type statsUpdateJob struct { wrapKey pb.StatsInfoKeyWithContext expectedRecord *updateRecord + // registered means enqueueStatsUpdateForRecord accounted this job in + // expectedRecord. Direct test helpers leave it false. + registered bool } type GlobalStats struct { @@ -279,6 +287,14 @@ type GlobalStats struct { // beforeSubscribeTable is for test only. beforeSubscribeTable func(pb.StatsInfoKey) + + // beforeStatsWait is for deterministic wait-protocol tests only. It runs + // with gs.mu held immediately before cond.Wait atomically releases it. + beforeStatsWait func(pb.StatsInfoKey, *updateRecord) + + // afterAutomaticUpdateStarted is for deterministic producer-cancellation + // tests only. It runs after worker admission and before refresh admission. + afterAutomaticUpdateStarted func(pb.StatsInfoKey, *updateRecord) } func NewGlobalStats( @@ -295,6 +311,9 @@ func NewGlobalStats( s.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) s.mu.statsInfoMap = make(map[pb.StatsInfoKey]*pb.StatsInfo) s.mu.cond = sync.NewCond(&s.mu) + // One lifecycle callback wakes every current waiter when update workers + // stop. Register it once per GlobalStats rather than once per cache miss. + context.AfterFunc(ctx, s.notifyStatsWaiters) s.initStatsRefreshAdmission() for _, opt := range opts { opt(s) @@ -390,14 +409,20 @@ func (gs *GlobalStats) PrefetchTableMeta(ctx context.Context, key pb.StatsInfoKe Ctx: ctx, Key: key, } - return gs.enqueueStatsUpdate(wrapkey, false) + generation, ok := gs.currentOrCreateSubscribedUpdateRecord(key) + if !ok { + return false + } + return gs.enqueueStatsUpdateForRecord(wrapkey, false, generation) } // currentOrCreateUpdateRecord returns the table-lifetime token that every // queued or explicit refresh must carry. In particular, the first refresh must // not use nil as an "expected absence" token: RemoveTid can make absence true // again, allowing old queued work to cross the cleanup boundary and recreate -// table-owned state. +// table-owned state. Production callers must already hold or have proved a +// cleanup owner; use currentOrCreateSubscribedUpdateRecord for request-driven +// work and shouldEnqueueExistingStatsUpdateGeneration for logtail work. func (gs *GlobalStats) currentOrCreateUpdateRecord(key pb.StatsInfoKey) *updateRecord { gs.updatingMu.Lock() defer gs.updatingMu.Unlock() @@ -428,6 +453,27 @@ func (gs *GlobalStats) currentOrCreateSubscribedUpdateRecord( return gs.currentOrCreateUpdateRecord(key), true } +// currentOrCreateExactSubscribedUpdateRecord is the first-read variant. It +// validates the exact subscription generation before mutating scheduling +// state, so an old read cannot create even idle metadata for a replacement +// lifetime. +func (gs *GlobalStats) currentOrCreateExactSubscribedUpdateRecord( + key pb.StatsInfoKey, + expectedEnt *subEntry, +) (*updateRecord, bool) { + if expectedEnt == nil { + return nil, false + } + gs.engine.pClient.subscribed.rw.RLock() + defer gs.engine.pClient.subscribed.rw.RUnlock() + + ent, ok := gs.engine.pClient.subscribed.m[key.TableID] + if !ok || ent != expectedEnt || ent.dbID != key.DatabaseID || ent.state != Subscribed { + return nil, false + } + return gs.currentOrCreateUpdateRecord(key), true +} + func (gs *GlobalStats) subscribedEntry(key pb.StatsInfoKey) *subEntry { gs.engine.pClient.subscribed.rw.RLock() defer gs.engine.pClient.subscribed.rw.RUnlock() @@ -512,6 +558,12 @@ func (gs *GlobalStats) Get(ctx context.Context, key pb.StatsInfoKey, sync bool) } subscribedEnt := gs.subscribedEntry(key) + if subscribedEnt == nil { + // Cleanup crossed the successful subscribe return before this read could + // capture the exact lifetime. Do not let nil mean "accept any later + // subscription" when the synchronous producer is created below. + return nil + } var remoteInfo *pb.StatsInfo if _, ok = ctx.Value(perfcounter.CalcTableStatsKey{}).(bool); ok { stats := statistic.StatsInfoFromContext(ctx) @@ -545,57 +597,71 @@ func (gs *GlobalStats) Get(ctx context.Context, key pb.StatsInfoKey, sync bool) } } - if sync { - stopWake := context.AfterFunc(ctx, func() { - gs.mu.Lock() - gs.mu.cond.Broadcast() - gs.mu.Unlock() - }) - defer stopWake() - } - + // Another producer may have published while subscription or remote lookup + // was in progress. Preserve this recheck for both synchronous and non-blocking + // callers. For a synchronous caller, an existing nil sentinel still admits a + // background retry, as before. gs.mu.Lock() - defer gs.mu.Unlock() - - // Recheck local cache after lock reacquired, another goroutine may have updated it. - info, ok = gs.mu.statsInfoMap[key] - if ok && info != nil { + info = gs.mu.statsInfoMap[key] + gs.mu.Unlock() + if info != nil { return info } + if !sync { + return nil + } - ok = false - if sync { - for !ok { - if ctx.Err() != nil { - return nil - } + // Capture the producer generation only while the exact subscription + // observed above still owns cleanup. A replacement subscription must not + // silently retarget this read to a new table lifetime. + generation, generationOwned := + gs.currentOrCreateExactSubscribedUpdateRecord(key, subscribedEnt) + if !generationOwned { + return nil + } - func() { - // A forced update can block while the channel is full. A worker - // draining it may need gs.mu, so unlock before enqueueing. - gs.mu.Unlock() - defer gs.mu.Lock() - // If the trigger condition is not satisfied, the stats will not be updated - // for long time. So we trigger the update here to get the stats info as soon - // as possible. - gs.enqueueStatsUpdate(wrapkey, true) - }() - - info, ok = gs.mu.statsInfoMap[key] - if ok { - break - } - if ctx.Err() != nil { - return nil - } + // A forced enqueue is the ownership transfer to a producer. If admission + // is canceled, no waiter may depend on work that was never accepted. + if !gs.enqueueStatsUpdateForRecord(wrapkey, true, generation) { + return nil + } + return gs.waitForStatsUpdate(ctx, key, generation) +} - // Wait until stats info of the key is updated. - gs.mu.cond.Wait() +// waitForStatsUpdate waits on durable state, not on a Broadcast edge. The +// cache predicate and exact queued/running producer predicate are both checked +// while gs.mu is held; RemoveTid uses the same gs.mu -> updatingMu order. +// Cleanup or producer exhaustion therefore either changes the predicate before +// this check, or broadcasts after cond.Wait has atomically registered the +// waiter and released gs.mu. +func (gs *GlobalStats) waitForStatsUpdate( + ctx context.Context, + key pb.StatsInfoKey, + generation *updateRecord, +) *pb.StatsInfo { + stopWake := context.AfterFunc(ctx, gs.notifyStatsWaiters) + defer stopWake() - info, ok = gs.mu.statsInfoMap[key] + gs.mu.Lock() + defer gs.mu.Unlock() + for { + if info, complete := gs.mu.statsInfoMap[key]; complete { + return info + } + if ctx.Err() != nil { + return nil } + if gs.ctx != nil && gs.ctx.Err() != nil { + return nil + } + if !gs.statsUpdateProducerActive(key, generation) { + return nil + } + if gs.beforeStatsWait != nil { + gs.beforeStatsWait(key, generation) + } + gs.mu.cond.Wait() } - return info } func (gs *GlobalStats) enqueue(tail *logtail.TableLogtail) { @@ -638,20 +704,23 @@ func (gs *GlobalStats) spawnUpdateWorkers(ctx context.Context, num int) { } } -func (gs *GlobalStats) enqueueStatsUpdate(key pb.StatsInfoKeyWithContext, force bool) bool { - return gs.enqueueStatsUpdateForRecord( - key, force, gs.currentOrCreateUpdateRecord(key.Key)) -} - func (gs *GlobalStats) enqueueStatsUpdateForRecord( key pb.StatsInfoKeyWithContext, force bool, expectedRecord *updateRecord, ) bool { + if expectedRecord == nil { + return false + } defer func() { v2.StatsTriggerQueueSizeGauge.Set(float64(len(gs.updateC))) }() - job := statsUpdateJob{wrapKey: key, expectedRecord: expectedRecord} + gs.registerStatsUpdateJob(expectedRecord) + job := statsUpdateJob{ + wrapKey: key, + expectedRecord: expectedRecord, + registered: true, + } if force { select { case gs.updateC <- job: @@ -659,6 +728,14 @@ func (gs *GlobalStats) enqueueStatsUpdateForRecord( v2.StatsTriggerForcedCounter.Add(1) return true case <-key.Ctx.Done(): + if gs.unregisterStatsUpdateJob(key.Key, expectedRecord) { + gs.notifyStatsWaiters() + } + return false + case <-gs.lifecycleDone(): + if gs.unregisterStatsUpdateJob(key.Key, expectedRecord) { + gs.notifyStatsWaiters() + } return false } } @@ -669,10 +746,50 @@ func (gs *GlobalStats) enqueueStatsUpdateForRecord( v2.StatsTriggerUnforcedCounter.Add(1) return true default: + if gs.unregisterStatsUpdateJob(key.Key, expectedRecord) { + gs.notifyStatsWaiters() + } return false } } +func (gs *GlobalStats) registerStatsUpdateJob(generation *updateRecord) { + gs.updatingMu.Lock() + generation.queued++ + gs.updatingMu.Unlock() +} + +// unregisterStatsUpdateJob rolls back a job that never reached worker +// admission. It returns true only when the current generation has no remaining +// queued or running producer and waiters must re-evaluate their predicate. +func (gs *GlobalStats) unregisterStatsUpdateJob( + key pb.StatsInfoKey, + generation *updateRecord, +) bool { + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() + if generation.queued > 0 { + generation.queued-- + } + current, ok := gs.updatingMu.updating[key] + return ok && current == generation && generation.queued == 0 && !generation.inProgress +} + +func (gs *GlobalStats) notifyStatsWaiters() { + gs.mu.Lock() + if gs.mu.cond != nil { + gs.mu.cond.Broadcast() + } + gs.mu.Unlock() +} + +func (gs *GlobalStats) lifecycleDone() <-chan struct{} { + if gs.ctx == nil { + return nil + } + return gs.ctx.Done() +} + func (gs *GlobalStats) processLogtail(ctx context.Context, tail *logtail.TableLogtail) { key := pb.StatsInfoKey{ AccId: tail.Table.AccId, @@ -788,16 +905,6 @@ func (gs *GlobalStats) shouldEnqueueUpdateGenerationLocked( return rec, false } -// shouldUpdate implements a debounce mechanism to prevent excessive stats updates. - -// shouldExecuteUpdate implements a debounce mechanism to prevent excessive stats updates. -// Only checks inProgress and MinUpdateInterval. -// Change rate is NOT checked here (already checked in shouldEnqueueUpdate). -func (gs *GlobalStats) shouldExecuteUpdate(key pb.StatsInfoKey) bool { - _, ok := gs.startAutomaticUpdate(key, gs.currentOrCreateUpdateRecord(key)) - return ok -} - func (gs *GlobalStats) startAutomaticUpdate( key pb.StatsInfoKey, expectedRecord *updateRecord, @@ -807,6 +914,35 @@ func (gs *GlobalStats) startAutomaticUpdate( } gs.updatingMu.Lock() defer gs.updatingMu.Unlock() + return gs.startAutomaticUpdateLocked(key, expectedRecord) +} + +func (gs *GlobalStats) startAutomaticUpdateJob( + job statsUpdateJob, +) (*updateRecord, bool, bool) { + if job.expectedRecord == nil { + return nil, false, false + } + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() + if job.registered && job.expectedRecord.queued > 0 { + job.expectedRecord.queued-- + } + generation, started := + gs.startAutomaticUpdateLocked(job.wrapKey.Key, job.expectedRecord) + if started { + return generation, true, false + } + current, ok := gs.updatingMu.updating[job.wrapKey.Key] + noProducer := ok && current == job.expectedRecord && + job.expectedRecord.queued == 0 && !job.expectedRecord.inProgress + return nil, false, noProducer +} + +func (gs *GlobalStats) startAutomaticUpdateLocked( + key pb.StatsInfoKey, + expectedRecord *updateRecord, +) (*updateRecord, bool) { rec, ok := gs.updatingMu.updating[key] if !ok || rec != expectedRecord { return nil, false @@ -828,6 +964,13 @@ func (gs *GlobalStats) statsUpdateGenerationActive(key pb.StatsInfoKey, generati return ok && rec == generation } +func (gs *GlobalStats) statsUpdateProducerActive(key pb.StatsInfoKey, generation *updateRecord) bool { + gs.updatingMu.Lock() + defer gs.updatingMu.Unlock() + rec, ok := gs.updatingMu.updating[key] + return ok && rec == generation && (rec.queued > 0 || rec.inProgress) +} + // markExplicitUpdateComplete advances the refresh baseline without stealing // the in-progress bit from an automatic refresh that was admitted before the // explicit refresh acquired the shared table stripe. @@ -1082,21 +1225,20 @@ func (gs *GlobalStats) completeAutomaticStatsCacheUpdate( gs.mu.cond.Broadcast() } -func (gs *GlobalStats) coordinateStatsUpdate(wrapKey pb.StatsInfoKeyWithContext) { - gs.coordinateStatsUpdateJob(statsUpdateJob{ - wrapKey: wrapKey, - expectedRecord: gs.currentOrCreateUpdateRecord(wrapKey.Key), - }) -} - func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { wrapKey := job.wrapKey statser := statistic.StatsInfoFromContext(wrapKey.Ctx) crs := new(perfcounter.CounterSet) - generation, ok := gs.startAutomaticUpdate(wrapKey.Key, job.expectedRecord) + generation, ok, noProducer := gs.startAutomaticUpdateJob(job) if !ok { + if noProducer { + gs.notifyStatsWaiters() + } return } + if gs.afterAutomaticUpdateStarted != nil { + gs.afterAutomaticUpdateStarted(wrapKey.Key, generation) + } // updated is used to mark that the stats info is updated. var updated bool @@ -1104,9 +1246,10 @@ func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { var samplingRatio float64 release, err := gs.acquireStatsRefresh(wrapKey.Ctx, wrapKey.Key) if err != nil { - // shouldExecuteUpdate opened this generation before admission. Close it + // Worker admission opened this generation before refresh admission. Close it // even when cancellation prevents this worker from acquiring the stripe. gs.markAutomaticUpdateComplete(wrapKey.Key, generation, false, 0, 0) + gs.notifyStatsWaiters() return } if !gs.statsUpdateGenerationActive(wrapKey.Key, generation) { diff --git a/pkg/vm/engine/disttae/stats_test.go b/pkg/vm/engine/disttae/stats_test.go index 311b3f44b99f6..24fce09aafddd 100644 --- a/pkg/vm/engine/disttae/stats_test.go +++ b/pkg/vm/engine/disttae/stats_test.go @@ -231,12 +231,16 @@ func TestGlobalStats_ShouldUpdate(t *testing.T) { DatabaseID: 100, TableID: 101, } - assert.True(t, gs.shouldExecuteUpdate(k1)) - assert.False(t, gs.shouldExecuteUpdate(k1)) + generation := gs.currentOrCreateUpdateRecord(k1) + _, started := gs.startAutomaticUpdate(k1, generation) + assert.True(t, started) + _, started = gs.startAutomaticUpdate(k1, generation) + assert.False(t, started) gs.markAutomaticUpdateComplete( - k1, gs.currentOrCreateUpdateRecord(k1), true, 1, 1.0) + k1, generation, true, 1, 1.0) time.Sleep(MinUpdateInterval) - assert.True(t, gs.shouldExecuteUpdate(k1)) + _, started = gs.startAutomaticUpdate(k1, generation) + assert.True(t, started) }) t.Run("parallel", func(t *testing.T) { @@ -257,12 +261,13 @@ func TestGlobalStats_ShouldUpdate(t *testing.T) { var wg sync.WaitGroup updateFn := func() { defer wg.Done() - if !gs.shouldExecuteUpdate(k1) { + generation := gs.currentOrCreateUpdateRecord(k1) + if _, started := gs.startAutomaticUpdate(k1, generation); !started { return } count.Add(1) gs.markAutomaticUpdateComplete( - k1, gs.currentOrCreateUpdateRecord(k1), true, 2, 1.0) + k1, generation, true, 2, 1.0) } for i := 0; i < 20; i++ { wg.Add(1) @@ -1807,10 +1812,11 @@ func TestRemoveTid(t *testing.T) { gs.mu.cond = sync.NewCond(&gs.mu) key := statsinfo.StatsInfoKey{DatabaseID: 100, TableID: 1001, TableName: "t1"} - require.True(t, gs.enqueueStatsUpdate(statsinfo.StatsInfoKeyWithContext{ + generation := gs.currentOrCreateUpdateRecord(key) + require.True(t, gs.enqueueStatsUpdateForRecord(statsinfo.StatsInfoKeyWithContext{ Ctx: context.Background(), Key: key, - }, false)) + }, false, generation)) job := <-gs.updateC require.NotNil(t, job.expectedRecord, "the first queued refresh must own a concrete lifetime token") @@ -1880,46 +1886,6 @@ func TestRemoveTid(t *testing.T) { }) }) - t.Run("remove_wakes_waiting_goroutines", func(t *testing.T) { - runTest(t, func(ctx context.Context, e *Engine) { - gs := e.globalStats - - // Set up: goroutine will cond.Wait() on a key, RemoveTid should broadcast - targetKey := statsinfo.StatsInfoKey{TableID: 42, DatabaseID: 1} - - woken := make(chan bool, 1) - gs.mu.Lock() - go func() { - gs.mu.Lock() - defer gs.mu.Unlock() - // Block on cond.Wait() like production GlobalStats.Get does - for { - if _, ok := gs.mu.statsInfoMap[targetKey]; ok { - break - } - // cond.Wait releases the lock and waits for Broadcast - gs.mu.cond.Wait() - // After Broadcast, check if our condition changed - break - } - woken <- true - }() - gs.mu.Unlock() - - // Small sleep to let goroutine enter cond.Wait() - time.Sleep(50 * time.Millisecond) - - // RemoveTid broadcasts to cond, which should wake the waiting goroutine - gs.RemoveTid(999) - - select { - case <-woken: - // ok — goroutine was woken by Broadcast - case <-time.After(2 * time.Second): - t.Fatal("RemoveTid did not wake goroutine blocked on cond.Wait()") - } - }) - }) } func TestGlobalStatsGetDoesNotHoldMuWhileSubscribing(t *testing.T) { @@ -1930,6 +1896,17 @@ func TestGlobalStatsGetDoesNotHoldMuWhileSubscribing(t *testing.T) { e.pClient.eng = e e.pClient.subscribed.eng = e + partition := e.GetOrCreateLatestPart(ctx, 0, dbID, tblID) + state, commit := partition.MutateState() + objectID := objectio.NewObjectid() + objectStats := objectio.NewObjectStatsWithObjectID( + &objectID, false, false, false) + require.NoError(t, objectio.SetObjectStatsSize(objectStats, 1)) + require.NoError(t, state.HandleObjectEntry(ctx, nil, objectio.ObjectEntry{ + ObjectStats: *objectStats, + CreateTime: types.BuildTS(1, 0), + }, false)) + commit() ent := &subEntry{dbID: dbID, state: Subscribed} ent.lastTs.Store(time.Now().UnixNano()) @@ -1968,10 +1945,11 @@ func TestGlobalStatsGetDoesNotHoldMuWhileSubscribing(t *testing.T) { getCtx, cancel := context.WithTimeout(ctx, 2*time.Second) defer cancel() - getDone := make(chan struct{}) + getDone := make(chan *statsinfo.StatsInfo, 1) + getCompleted := make(chan struct{}) go func() { - defer close(getDone) - _ = gs.Get(getCtx, key, false) + getDone <- gs.Get(getCtx, key, false) + close(getCompleted) }() require.Eventually(t, func() bool { @@ -1983,16 +1961,19 @@ func TestGlobalStatsGetDoesNotHoldMuWhileSubscribing(t *testing.T) { } }, time.Second, 10*time.Millisecond, "GlobalStats.Get did not reach subscribe path") + published := plan2.NewStatsInfo() + published.TableCnt = 42 muAcquired := make(chan struct{}) go func() { gs.mu.Lock() + gs.mu.statsInfoMap[key] = published gs.mu.Unlock() close(muAcquired) }() require.Eventually(t, func() bool { select { - case <-getDone: + case <-getCompleted: return false default: } @@ -2008,13 +1989,58 @@ func TestGlobalStatsGetDoesNotHoldMuWhileSubscribing(t *testing.T) { e.pClient.subscribed.rw.Unlock() select { - case <-getDone: + case result := <-getDone: + require.Same(t, published, result, + "a non-blocking Get must recheck publication after subscription") case <-time.After(time.Second): t.Fatal("GlobalStats.Get did not return after subscribe lock released") } }) } +func newSynchronousStatsGetHarness( + t *testing.T, + ctx context.Context, + e *Engine, + key statsinfo.StatsInfoKey, +) (*GlobalStats, *subEntry) { + t.Helper() + partition := e.GetOrCreateLatestPart(ctx, uint64(key.AccId), key.DatabaseID, key.TableID) + state, commit := partition.MutateState() + objectID := objectio.NewObjectid() + objectStats := objectio.NewObjectStatsWithObjectID( + &objectID, false, false, false) + require.NoError(t, objectio.SetObjectStatsSize(objectStats, 1)) + require.NoError(t, state.HandleObjectEntry(ctx, nil, objectio.ObjectEntry{ + ObjectStats: *objectStats, + CreateTime: types.BuildTS(1, 0), + }, false)) + commit() + + e.pClient.eng = e + e.pClient.subscribed.eng = e + ent := &subEntry{dbID: key.DatabaseID, state: Subscribed} + ent.lastTs.Store(time.Now().UnixNano()) + e.pClient.subscribed.rw.Lock() + if e.pClient.subscribed.m == nil { + e.pClient.subscribed.m = make(map[uint64]*subEntry) + } + e.pClient.subscribed.m[key.TableID] = ent + e.pClient.subscribed.rw.Unlock() + + gs := &GlobalStats{ + ctx: ctx, + engine: e, + updateC: make(chan statsUpdateJob, 1), + queueWatcher: newQueueWatcher(), + } + gs.updatingMu.updating = make(map[statsinfo.StatsInfoKey]*updateRecord) + gs.mu.statsInfoMap = make(map[statsinfo.StatsInfoKey]*statsinfo.StatsInfo) + gs.mu.cond = sync.NewCond(&gs.mu) + gs.initStatsRefreshAdmission() + return gs, ent +} + func TestGlobalStatsGetReturnsWhenContextCanceledWhileWaiting(t *testing.T) { runTest(t, func(ctx context.Context, e *Engine) { const dbID uint64 = 100 @@ -2027,42 +2053,15 @@ func TestGlobalStatsGetReturnsWhenContextCanceledWhileWaiting(t *testing.T) { DbName: "d", } - partition := e.GetOrCreateLatestPart(ctx, 0, dbID, tblID) - state, commit := partition.MutateState() - objectID := objectio.NewObjectid() - objectStats := objectio.NewObjectStatsWithObjectID( - &objectID, false, false, false) - require.NoError(t, objectio.SetObjectStatsSize(objectStats, 1)) - require.NoError(t, state.HandleObjectEntry(ctx, nil, objectio.ObjectEntry{ - ObjectStats: *objectStats, - CreateTime: types.BuildTS(1, 0), - }, false)) - commit() - - e.pClient.eng = e - e.pClient.subscribed.eng = e - e.pClient.subscribed.rw.Lock() - if e.pClient.subscribed.m == nil { - e.pClient.subscribed.m = make(map[uint64]*subEntry) - } - e.pClient.subscribed.m[tblID] = &subEntry{ - dbID: dbID, - state: Subscribed, - } - e.pClient.subscribed.rw.Unlock() - // This isolated GlobalStats has no update worker. Receiving its forced // request below proves Get reached the synchronous update path, after // which no data-path goroutine can broadcast the condition variable. - gs := &GlobalStats{ - ctx: ctx, - engine: e, - updateC: make(chan statsUpdateJob, 1), - queueWatcher: newQueueWatcher(), + gs, _ := newSynchronousStatsGetHarness(t, ctx, e, key) + waitEntered := make(chan struct{}) + var waitOnce sync.Once + gs.beforeStatsWait = func(statsinfo.StatsInfoKey, *updateRecord) { + waitOnce.Do(func() { close(waitEntered) }) } - gs.updatingMu.updating = make(map[statsinfo.StatsInfoKey]*updateRecord) - gs.mu.statsInfoMap = make(map[statsinfo.StatsInfoKey]*statsinfo.StatsInfo) - gs.mu.cond = sync.NewCond(&gs.mu) getCtx, cancel := context.WithCancel(ctx) defer cancel() @@ -2071,11 +2070,17 @@ func TestGlobalStatsGetReturnsWhenContextCanceledWhileWaiting(t *testing.T) { result <- gs.Get(getCtx, key, true) }() + var job statsUpdateJob select { - case <-gs.updateC: + case job = <-gs.updateC: case <-time.After(time.Second): t.Fatal("GlobalStats.Get did not enqueue the synchronous update") } + select { + case <-waitEntered: + case <-time.After(time.Second): + t.Fatal("GlobalStats.Get did not register its condition wait") + } cancel() select { @@ -2084,10 +2089,329 @@ func TestGlobalStatsGetReturnsWhenContextCanceledWhileWaiting(t *testing.T) { case <-time.After(time.Second): t.Fatal("GlobalStats.Get did not return after context cancellation") } + gs.unregisterStatsUpdateJob(key, job.expectedRecord) gs.queueWatcher.del(tblID) }) } +func TestGlobalStatsGetReturnsPublishedStatsFromAcceptedProducer(t *testing.T) { + runTest(t, func(ctx context.Context, e *Engine) { + key := statsinfo.StatsInfoKey{ + DatabaseID: 100, + TableID: 10006, + TableName: "t", + DbName: "d", + } + gs, _ := newSynchronousStatsGetHarness(t, ctx, e, key) + result := make(chan *statsinfo.StatsInfo, 1) + go func() { result <- gs.Get(ctx, key, true) }() + + var job statsUpdateJob + select { + case job = <-gs.updateC: + case <-time.After(time.Second): + t.Fatal("synchronous read did not enqueue its producer") + } + generation, started, noProducer := gs.startAutomaticUpdateJob(job) + require.True(t, started) + require.False(t, noProducer) + published := plan2.NewStatsInfo() + published.TableCnt = 42 + gs.completeAutomaticStatsCacheUpdate(key, generation, published, true) + gs.markAutomaticUpdateComplete(key, generation, true, 1, 1) + + select { + case info := <-result: + require.Same(t, published, info) + case <-time.After(time.Second): + t.Fatal("synchronous read did not observe accepted producer publication") + } + gs.queueWatcher.del(key.TableID) + }) +} + +func TestGlobalStatsGetReturnsWhenCleanupPrecedesWait(t *testing.T) { + runTest(t, func(ctx context.Context, e *Engine) { + key := statsinfo.StatsInfoKey{ + DatabaseID: 100, + TableID: 10002, + TableName: "t", + DbName: "d", + } + gs, _ := newSynchronousStatsGetHarness(t, ctx, e, key) + + // Hold the watcher after updateC accepts the job. This is an observable + // barrier between producer ownership transfer and wait registration. + gs.queueWatcher.Lock() + watcherLocked := true + defer func() { + if watcherLocked { + gs.queueWatcher.Unlock() + } + }() + + waitEntered := make(chan struct{}) + var waitOnce sync.Once + gs.beforeStatsWait = func(statsinfo.StatsInfoKey, *updateRecord) { + waitOnce.Do(func() { close(waitEntered) }) + } + result := make(chan *statsinfo.StatsInfo, 1) + go func() { result <- gs.Get(ctx, key, true) }() + + var staleJob statsUpdateJob + select { + case staleJob = <-gs.updateC: + case <-time.After(time.Second): + t.Fatal("GlobalStats.Get did not transfer the refresh job") + } + + // Cleanup broadcasts before Get is able to enter cond.Wait. Processing + // the stale job produces no second notification. + gs.RemoveTid(key.TableID) + gs.coordinateStatsUpdateJob(staleJob) + watcherLocked = false + gs.queueWatcher.Unlock() + + select { + case info := <-result: + require.Nil(t, info) + case <-time.After(time.Second): + t.Fatal("GlobalStats.Get lost the cleanup wake before cond.Wait") + } + select { + case <-waitEntered: + t.Fatal("GlobalStats.Get waited after its producer generation was removed") + default: + } + gs.queueWatcher.del(key.TableID) + }) +} + +func TestGlobalStatsGetReturnsWhenCleanupFollowsWait(t *testing.T) { + runTest(t, func(ctx context.Context, e *Engine) { + key := statsinfo.StatsInfoKey{ + DatabaseID: 100, + TableID: 10003, + TableName: "t", + DbName: "d", + } + gs, _ := newSynchronousStatsGetHarness(t, ctx, e, key) + waitEntered := make(chan struct{}) + var waitOnce sync.Once + gs.beforeStatsWait = func(statsinfo.StatsInfoKey, *updateRecord) { + waitOnce.Do(func() { close(waitEntered) }) + } + result := make(chan *statsinfo.StatsInfo, 1) + go func() { result <- gs.Get(ctx, key, true) }() + + var staleJob statsUpdateJob + select { + case staleJob = <-gs.updateC: + case <-time.After(time.Second): + t.Fatal("GlobalStats.Get did not transfer the refresh job") + } + select { + case <-waitEntered: + case <-time.After(time.Second): + t.Fatal("GlobalStats.Get did not reach cond.Wait") + } + + gs.RemoveTid(key.TableID) + select { + case info := <-result: + require.Nil(t, info) + case <-time.After(time.Second): + t.Fatal("GlobalStats.Get was not released by cleanup after cond.Wait") + } + gs.coordinateStatsUpdateJob(staleJob) + gs.queueWatcher.del(key.TableID) + }) +} + +func TestGlobalStatsGetDoesNotOutliveCanceledSharedProducer(t *testing.T) { + runTest(t, func(ctx context.Context, e *Engine) { + key := statsinfo.StatsInfoKey{ + DatabaseID: 100, + TableID: 10004, + TableName: "t", + DbName: "d", + } + gs, _ := newSynchronousStatsGetHarness(t, ctx, e, key) + + // Occupy the table stripe so the first producer can be canceled after + // worker admission but before it starts object work. + releaseStripe, err := gs.acquireStatsRefresh(context.Background(), key) + require.NoError(t, err) + stripeHeld := true + defer func() { + if stripeHeld { + releaseStripe() + } + }() + + producerStarted := make(chan struct{}) + var producerOnce sync.Once + gs.afterAutomaticUpdateStarted = func(statsinfo.StatsInfoKey, *updateRecord) { + producerOnce.Do(func() { close(producerStarted) }) + } + + producerCtx, cancelProducer := context.WithCancel(ctx) + defer cancelProducer() + producerResult := make(chan *statsinfo.StatsInfo, 1) + go func() { producerResult <- gs.Get(producerCtx, key, true) }() + var producerJob statsUpdateJob + select { + case producerJob = <-gs.updateC: + case <-time.After(time.Second): + t.Fatal("first synchronous read did not enqueue its producer") + } + producerDone := make(chan struct{}) + go func() { + gs.coordinateStatsUpdateJob(producerJob) + close(producerDone) + }() + select { + case <-producerStarted: + case <-time.After(time.Second): + t.Fatal("first producer did not reach worker admission") + } + + sharedResult := make(chan *statsinfo.StatsInfo, 1) + go func() { sharedResult <- gs.Get(ctx, key, true) }() + var sharedJob statsUpdateJob + select { + case sharedJob = <-gs.updateC: + case <-time.After(time.Second): + t.Fatal("second synchronous read did not enqueue its shared job") + } + // This job coalesces behind the in-progress producer. Its waiter must + // still be released if that producer is canceled before publication. + gs.coordinateStatsUpdateJob(sharedJob) + cancelProducer() + + select { + case <-producerDone: + case <-time.After(time.Second): + t.Fatal("canceled producer did not leave refresh admission") + } + select { + case info := <-producerResult: + require.Nil(t, info) + case <-time.After(time.Second): + t.Fatal("producer caller did not observe cancellation") + } + select { + case info := <-sharedResult: + require.Nil(t, info) + case <-time.After(time.Second): + t.Fatal("shared waiter outlived its canceled producer") + } + + stripeHeld = false + releaseStripe() + gs.queueWatcher.del(key.TableID) + }) +} + +func TestGlobalStatsGetReturnsWhenStatsWorkersStop(t *testing.T) { + runTest(t, func(ctx context.Context, e *Engine) { + key := statsinfo.StatsInfoKey{ + DatabaseID: 100, + TableID: 10005, + TableName: "t", + DbName: "d", + } + gs, _ := newSynchronousStatsGetHarness(t, ctx, e, key) + workerCtx, stopWorkers := context.WithCancel(ctx) + gs.ctx = workerCtx + context.AfterFunc(workerCtx, gs.notifyStatsWaiters) + waitEntered := make(chan struct{}) + var waitOnce sync.Once + gs.beforeStatsWait = func(statsinfo.StatsInfoKey, *updateRecord) { + waitOnce.Do(func() { close(waitEntered) }) + } + + result := make(chan *statsinfo.StatsInfo, 1) + go func() { result <- gs.Get(ctx, key, true) }() + var abandonedJob statsUpdateJob + select { + case abandonedJob = <-gs.updateC: + case <-time.After(time.Second): + t.Fatal("synchronous read did not enqueue before worker shutdown") + } + select { + case <-waitEntered: + case <-time.After(time.Second): + t.Fatal("synchronous read did not wait for its queued producer") + } + + stopWorkers() + select { + case info := <-result: + require.Nil(t, info) + case <-time.After(time.Second): + t.Fatal("synchronous read outlived the statistics worker lifecycle") + } + gs.unregisterStatsUpdateJob(key, abandonedJob.expectedRecord) + gs.queueWatcher.del(key.TableID) + }) +} + +func TestStatsUpdateGenerationRequiresLiveSubscriptionOwner(t *testing.T) { + key := statsinfo.StatsInfoKey{DatabaseID: 10, TableID: 42} + e := &Engine{} + gs := &GlobalStats{ + engine: e, + updateC: make(chan statsUpdateJob, 1), + queueWatcher: newQueueWatcher(), + } + gs.updatingMu.updating = make(map[statsinfo.StatsInfoKey]*updateRecord) + gs.mu.statsInfoMap = make(map[statsinfo.StatsInfoKey]*statsinfo.StatsInfo) + gs.mu.cond = sync.NewCond(&gs.mu) + e.pClient.subscribed.rw.Lock() + e.pClient.subscribed.m = make(map[uint64]*subEntry) + e.pClient.subscribed.rw.Unlock() + + require.False(t, gs.PrefetchTableMeta(context.Background(), key)) + gs.updatingMu.Lock() + _, retained := gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + require.False(t, retained, + "prefetch without a subscription cleanup owner must not create a generation") + + oldEnt := &subEntry{dbID: key.DatabaseID, state: Subscribed} + e.pClient.subscribed.rw.Lock() + e.pClient.subscribed.m[key.TableID] = oldEnt + e.pClient.subscribed.rw.Unlock() + oldGeneration, ok := gs.currentOrCreateExactSubscribedUpdateRecord(key, oldEnt) + require.True(t, ok) + require.True(t, gs.PrefetchTableMeta(context.Background(), key)) + job := <-gs.updateC + require.Same(t, oldGeneration, job.expectedRecord) + gs.queueWatcher.del(key.TableID) + + gs.RemoveTid(key.TableID) + gs.coordinateStatsUpdateJob(job) + gs.updatingMu.Lock() + require.Zero(t, oldGeneration.queued) + gs.updatingMu.Unlock() + newEnt := &subEntry{dbID: key.DatabaseID, state: Subscribed} + e.pClient.subscribed.rw.Lock() + e.pClient.subscribed.m[key.TableID] = newEnt + e.pClient.subscribed.rw.Unlock() + _, ok = gs.currentOrCreateExactSubscribedUpdateRecord(key, oldEnt) + require.False(t, ok, + "work captured from an old subscription must not target its replacement") + gs.updatingMu.Lock() + _, retained = gs.updatingMu.updating[key] + gs.updatingMu.Unlock() + require.False(t, retained, + "rejecting an old subscription must not create idle replacement metadata") + newGeneration, ok := gs.currentOrCreateExactSubscribedUpdateRecord(key, newEnt) + require.True(t, ok) + require.NotSame(t, oldGeneration, newGeneration) +} + func TestEnqueueStatsUpdateForceReturnsWhenContextCanceled(t *testing.T) { gs := &GlobalStats{ updateC: make(chan statsUpdateJob, 1), @@ -2102,10 +2426,11 @@ func TestEnqueueStatsUpdateForceReturnsWhenContextCanceled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - accepted := gs.enqueueStatsUpdate(statsinfo.StatsInfoKeyWithContext{ + generation := gs.currentOrCreateUpdateRecord(statsinfo.StatsInfoKey{TableID: 2}) + accepted := gs.enqueueStatsUpdateForRecord(statsinfo.StatsInfoKeyWithContext{ Ctx: ctx, Key: statsinfo.StatsInfoKey{TableID: 2}, - }, true) + }, true, generation) require.False(t, accepted) require.Equal(t, queued, (<-gs.updateC).wrapKey) } From b3aa6492846d48b9aab39a2437de6d8fe230f2ae Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 12:23:52 +0800 Subject: [PATCH 16/18] fix(disttae): make stats refresh lifecycle authoritative --- docs/design/analyze_stats_publication.md | 36 ++++- pkg/vm/engine/disttae/engine_stats_test.go | 57 +++++++- pkg/vm/engine/disttae/stats.go | 150 ++++++++++++++++++--- pkg/vm/engine/disttae/stats_test.go | 42 +++++- pkg/vm/engine/disttae/txn_table.go | 42 ++++-- pkg/vm/engine/disttae/util_test.go | 144 ++++++++++++++++++++ 6 files changed, 427 insertions(+), 44 deletions(-) diff --git a/docs/design/analyze_stats_publication.md b/docs/design/analyze_stats_publication.md index fa2db25334c73..cef8392cc36d9 100644 --- a/docs/design/analyze_stats_publication.md +++ b/docs/design/analyze_stats_publication.md @@ -81,7 +81,15 @@ It intentionally does not provide: 3. Admission and executor submission observe caller cancellation. The shared traversal context also observes executor lifecycle cancellation, so shutdown cannot leave a producer blocked on a full queue, a running S3 task using an - orphaned request, or a caller waiting for abandoned queued work. + orphaned request, or a caller waiting for abandoned queued work. Async + cancellation callbacks are notification mechanisms, never terminal + predicates: refresh admission, zero-object fast paths, joined traversal, and + cache publication synchronously re-read the owning lifecycle context before + reporting or publishing success. Subscription and object I/O use a + refresh-local context that preserves request values/deadlines and is canceled + by either the request or the owner lifecycle. That linked context is created + only after fixed-stripe admission, bounding simultaneously registered owner + callbacks by the 64 refresh-admission tokens. 4. Unrelated tables normally remain parallel. A bounded hash-stripe collision may serialize refresh control work but cannot affect query execution. 5. A synchronous first-read waiter may sleep only while the exact refresh @@ -298,9 +306,16 @@ error and waits for all admitted work before returning. Waiting is required because callbacks mutate a refresh-local accumulator; returning early would let old work race a discarded accumulator. Callback I/O receives the request context, so cancellation terminates the expensive work without polling or -sleeps. After joining all admitted work, traversal also checks the shared task -context itself: executor shutdown remains a failed traversal even if a running -callback ignored cancellation and returned `nil`. +sleeps. The executor lifecycle context is the durable predicate; the shared task +context is only its cancellation-delivery path. Traversal uses a +check/register/check sequence around that delivery callback and re-reads both +contexts after joining all admitted work. Executor shutdown therefore remains a +failed traversal even if callback dispatch is delayed or a running callback +ignored cancellation and returned `nil`. The enclosing refresh applies the same +owner-lifecycle predicate at admission and cache publication, including the +zero-object path that does not traverse objects at all. A successful refresh +linearizes at the final lifecycle and generation checks performed while the +publication lock is held. Cancellation and deadline errors remain cancellation/deadline errors at the public refresh boundary. Other object/metadata failures may be wrapped with @@ -358,8 +373,10 @@ approximately 181 ms and changed the next plan from stale non-shuffle to a sampling mode, S3 requests, and end-to-end ANALYZE latency. The executor repair keeps the existing closure and wait-group shape. It adds a -success-path branch and error accumulator, plus executor lifecycle bookkeeping; -it must not create one result channel or goroutine per object. Focused +constant number of lifecycle reads per refresh, a success-path branch and error +accumulator, plus executor lifecycle bookkeeping; it does not add a check, result +channel, allocation, or goroutine per object. The refresh-local owner callback is +registered only after fixed-stripe admission, so at most 64 are active. Focused benchmarks or allocation tests are required if the final implementation changes that property. @@ -458,6 +475,13 @@ Decision log: - Treat condition-variable broadcasts only as hints. The cache/context/exact generation predicates are checked under the condition mutex, and every producer generation is captured under a live subscription cleanup owner. +- Treat lifecycle callbacks only as cancellation delivery. Admission, joined + traversal, zero-object completion, and publication re-read the request and + owner contexts as durable predicates; linked owner callbacks are created only + after fixed-stripe admission. +- Give each admitted automatic refresh one terminal owner that commits the + cache, advances metadata only when that commit succeeds, and finally releases + admission in that order. Open approval item: an independent reviewer must approve this exact design revision before the implementation is considered deliverable. There are no diff --git a/pkg/vm/engine/disttae/engine_stats_test.go b/pkg/vm/engine/disttae/engine_stats_test.go index 9d2f4cff5bd39..0231213777bea 100644 --- a/pkg/vm/engine/disttae/engine_stats_test.go +++ b/pkg/vm/engine/disttae/engine_stats_test.go @@ -89,6 +89,50 @@ func TestOptimizerStatsRefreshAdmissionIsTableScopedAndCancelable(t *testing.T) require.Nil(t, releaseCanceled) } +func TestOptimizerStatsRefreshAdmissionRejectsStoppedOwner(t *testing.T) { + ownerCtx, stopOwner := context.WithCancel(context.Background()) + stopOwner() + gs := &GlobalStats{ctx: ownerCtx} + gs.initStatsRefreshAdmission() + + release, err := gs.acquireStatsRefresh( + context.Background(), pb.StatsInfoKey{AccId: 1, TableID: 42}) + require.ErrorIs(t, err, context.Canceled) + require.Nil(t, release, + "a stopped GlobalStats owner must not transfer a refresh admission token") +} + +func TestStatsRefreshContextPreservesRequestAndObservesOwnerStop(t *testing.T) { + type requestValueKey struct{} + ownerCtx, stopOwner := context.WithCancel(context.Background()) + requestCtx := context.WithValue(context.Background(), requestValueKey{}, "request-value") + gs := &GlobalStats{ctx: ownerCtx} + + refreshCtx, stopRefresh, err := gs.newStatsRefreshContext(requestCtx) + require.NoError(t, err) + t.Cleanup(stopRefresh) + require.Equal(t, "request-value", refreshCtx.Value(requestValueKey{})) + + stopOwner() + select { + case <-refreshCtx.Done(): + require.ErrorIs(t, context.Cause(refreshCtx), context.Canceled) + case <-time.After(time.Second): + t.Fatal("owner shutdown did not cancel downstream refresh work") + } +} + +func TestStatsRefreshContextClosesOwnerWatcherRegistrationRace(t *testing.T) { + ownerCtx := newDelayedLifecycleContext() + ownerCtx.cancelOnRegister = true + gs := &GlobalStats{ctx: ownerCtx} + + refreshCtx, stopRefresh, err := gs.newStatsRefreshContext(context.Background()) + require.ErrorIs(t, err, context.Canceled) + require.Nil(t, refreshCtx) + require.Nil(t, stopRefresh) +} + func TestCoordinateStatsUpdateCancellationReleasesUpdateGeneration(t *testing.T) { gs := &GlobalStats{} gs.initStatsRefreshAdmission() @@ -115,10 +159,12 @@ func TestCoordinateStatsUpdateCancellationReleasesUpdateGeneration(t *testing.T) "cancellation while waiting for refresh admission must close the update generation") } -func TestCompleteStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { +func TestCompleteAutomaticStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { gs := &GlobalStats{} gs.initStatsRefreshAdmission() gs.updatingMu.updating = make(map[pb.StatsInfoKey]*updateRecord) + gs.mu.statsInfoMap = make(map[pb.StatsInfoKey]*pb.StatsInfo) + gs.mu.cond = sync.NewCond(&gs.mu) key := pb.StatsInfoKey{AccId: 1, TableID: 42} generation := &updateRecord{inProgress: true} @@ -141,10 +187,11 @@ func TestCompleteStatsRefreshKeepsMetadataInsideAdmission(t *testing.T) { // Waiting here after releasing forces the newer refresh to commit between // release and any code that might incorrectly update the old baseline late. var newerErr error - gs.completeStatsRefresh(key, generation, true, 1, 1.0, func() { - oldRelease() - newerErr = <-newerDone - }) + gs.completeAutomaticStatsRefresh( + key, generation, &pb.StatsInfo{}, true, 1, 1.0, func() { + oldRelease() + newerErr = <-newerDone + }) require.NoError(t, newerErr) gs.updatingMu.Lock() diff --git a/pkg/vm/engine/disttae/stats.go b/pkg/vm/engine/disttae/stats.go index a167141fa974f..a47c9619bc4ed 100644 --- a/pkg/vm/engine/disttae/stats.go +++ b/pkg/vm/engine/disttae/stats.go @@ -367,15 +367,74 @@ func (gs *GlobalStats) acquireStatsRefresh( ctx context.Context, key pb.StatsInfoKey, ) (func(), error) { + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { + return nil, cause + } admission := gs.refreshAdmission[optimizerStatsRefreshStripe(key)] select { case admission <- struct{}{}: + // Cancellation can race the select and make both cases ready. Recheck + // the authoritative caller and owner contexts before transferring the + // admission token to the caller. + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { + <-admission + return nil, cause + } return func() { <-admission }, nil case <-ctx.Done(): return nil, context.Cause(ctx) + case <-gs.lifecycleDone(): + return nil, gs.statsRefreshCancellationCause(ctx) } } +// statsRefreshCancellationCause treats the request context and the GlobalStats +// owner lifecycle as durable predicates. Async callbacks may wake blocked work, +// but they are never the source of truth for admission or publication. +func (gs *GlobalStats) statsRefreshCancellationCause(ctx context.Context) error { + if cause := context.Cause(ctx); cause != nil { + return cause + } + if gs.ctx != nil { + return context.Cause(gs.ctx) + } + return nil +} + +// newStatsRefreshContext preserves request values and deadlines while linking +// downstream subscription and object I/O to the GlobalStats owner lifecycle. +// The returned context delivers cancellation; admission and publication still +// re-read statsRefreshCancellationCause as their durable predicate. +func (gs *GlobalStats) newStatsRefreshContext( + ctx context.Context, +) (context.Context, func(), error) { + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { + return nil, nil, cause + } + if gs.ctx == nil { + return ctx, func() {}, nil + } + refreshCtx, cancelRefresh := context.WithCancelCause(ctx) + stopOwnerWatch := context.AfterFunc(gs.ctx, func() { + cause := context.Cause(gs.ctx) + if cause == nil { + cause = context.Canceled + } + cancelRefresh(cause) + }) + stop := func() { + stopOwnerWatch() + cancelRefresh(nil) + } + // Close the owner check/register race without depending on callback + // dispatch. No downstream operation is admitted on the error path. + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { + stop() + return nil, nil, cause + } + return refreshCtx, stop, nil +} + // RemoveTid removes every GlobalStats entry owned by the given table ID. // Called from cleanMemoryTableWithTable (1+ hour after unsubscribe/drop) // to prevent both published statistics and refresh-scheduling metadata from @@ -1206,15 +1265,27 @@ func (gs *GlobalStats) completeAutomaticStatsCacheUpdate( generation *updateRecord, stats *pb.StatsInfo, updated bool, -) { +) bool { gs.mu.Lock() defer gs.mu.Unlock() + // GlobalStats shutdown is an authoritative failed-publication predicate. + // In particular, do not install a first-generation nil sentinel here: the + // lifecycle watcher already wakes waiters, and shutdown must leave the cache + // and its scheduling baseline unchanged. + if gs.ctx != nil && context.Cause(gs.ctx) != nil { + if gs.mu.cond != nil { + gs.mu.cond.Broadcast() + } + return false + } // The update record is also the automatic generation's table-lifetime // token. RemoveTid deletes it under the same gs.mu -> updatingMu order; an // old worker that completes afterward must not resurrect either cache. if !gs.statsUpdateGenerationActive(key, generation) { - gs.mu.cond.Broadcast() - return + if gs.mu.cond != nil { + gs.mu.cond.Broadcast() + } + return false } if updated { gs.mu.statsInfoMap[key] = stats @@ -1222,7 +1293,10 @@ func (gs *GlobalStats) completeAutomaticStatsCacheUpdate( } else if _, ok := gs.mu.statsInfoMap[key]; !ok { gs.mu.statsInfoMap[key] = nil } - gs.mu.cond.Broadcast() + if gs.mu.cond != nil { + gs.mu.cond.Broadcast() + } + return updated } func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { @@ -1244,6 +1318,7 @@ func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { var updated bool var actualObjectCount int64 var samplingRatio float64 + var stats *pb.StatsInfo release, err := gs.acquireStatsRefresh(wrapKey.Ctx, wrapKey.Key) if err != nil { // Worker admission opened this generation before refresh admission. Close it @@ -1259,15 +1334,21 @@ func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { return } defer func() { - gs.completeStatsRefresh( - wrapKey.Key, generation, updated, actualObjectCount, samplingRatio, release) + gs.completeAutomaticStatsRefresh( + wrapKey.Key, generation, stats, updated, + actualObjectCount, samplingRatio, release) }() + refreshCtx, stopRefresh, err := gs.newStatsRefreshContext(wrapKey.Ctx) + if err != nil { + return + } + defer stopRefresh() // Get the latest partition state of the table. //Notice that for snapshot read, subscribing the table maybe failed since the invalid table id, //We should handle this case in next PR if needed. ps, err := gs.engine.pClient.toSubscribeTable( - wrapKey.Ctx, + refreshCtx, uint64(wrapKey.Key.AccId), wrapKey.Key.TableID, wrapKey.Key.TableName, @@ -1279,12 +1360,11 @@ func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { wrapKey.Key.TableID, wrapKey.Key.TableName, err) - gs.completeAutomaticStatsCacheUpdate(wrapKey.Key, generation, nil, false) return } - stats := plan2.NewStatsInfo() + stats = plan2.NewStatsInfo() - newCtx := perfcounter.AttachS3RequestKey(wrapKey.Ctx, crs) + newCtx := perfcounter.AttachS3RequestKey(refreshCtx, crs) updated, samplingRatio = gs.executeStatsUpdate(newCtx, ps, wrapKey.Key, stats) // Get actual object count for baseline update @@ -1301,21 +1381,25 @@ func (gs *GlobalStats) coordinateStatsUpdateJob(job statsUpdateJob) { DeleteMul: crs.FileService.S3.DeleteMulti.Load(), }) - gs.completeAutomaticStatsCacheUpdate(wrapKey.Key, generation, stats, updated) } -// completeStatsRefresh commits the automatic-refresh scheduling metadata -// before another same-table refresh can enter. The statistics cache and its -// object-count/sampling baseline therefore advance in one serialized order. -func (gs *GlobalStats) completeStatsRefresh( +// completeAutomaticStatsRefresh is the sole terminal owner for an admitted +// automatic refresh. Cache publication decides whether the result committed; +// only that committed result may advance scheduling metadata, and both happen +// before the table-scoped admission token is released. +func (gs *GlobalStats) completeAutomaticStatsRefresh( key pb.StatsInfoKey, generation *updateRecord, - updated bool, + stats *pb.StatsInfo, + calculated bool, actualObjectCount int64, samplingRatio float64, release func(), ) { - gs.markAutomaticUpdateComplete(key, generation, updated, actualObjectCount, samplingRatio) + committed := gs.completeAutomaticStatsCacheUpdate( + key, generation, stats, calculated) + gs.markAutomaticUpdateComplete( + key, generation, committed, actualObjectCount, samplingRatio) release() } @@ -1339,16 +1423,24 @@ func (gs *GlobalStats) refreshStatsWithMode( return nil, err } defer release() + refreshCtx, stopRefresh, err := gs.newStatsRefreshContext(ctx) + if err != nil { + return nil, err + } + defer stopRefresh() // Get partition state ps, err := gs.engine.pClient.toSubscribeTable( - ctx, + refreshCtx, uint64(key.AccId), key.TableID, key.TableName, key.DatabaseID, key.DbName) if err != nil { + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { + return nil, cause + } return nil, moerr.NewInternalErrorNoCtxf("failed to subscribe table: %v", err) } @@ -1389,9 +1481,9 @@ func (gs *GlobalStats) refreshStatsWithMode( } // Execute stats update - samplingRatio, err := CollectAndCalculateStats(ctx, req, gs.concurrentExecutor) + samplingRatio, err := CollectAndCalculateStats(refreshCtx, req, gs.concurrentExecutor) if err != nil { - if cause := context.Cause(ctx); cause != nil { + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { return nil, cause } if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { @@ -1399,11 +1491,14 @@ func (gs *GlobalStats) refreshStatsWithMode( } return nil, moerr.NewInternalErrorNoCtxf("failed to update stats: %v", err) } - if cause := context.Cause(ctx); cause != nil { + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { return nil, cause } if !gs.publishStatsForGeneration(key, generation, stats) { + if cause := gs.statsRefreshCancellationCause(ctx); cause != nil { + return nil, cause + } return nil, moerr.NewInternalErrorNoCtxf( "table statistics refresh crossed cleanup boundary for table %d", key.TableID) } @@ -1430,6 +1525,9 @@ func (gs *GlobalStats) publishStatsForGeneration( } gs.mu.Lock() defer gs.mu.Unlock() + if gs.ctx != nil && context.Cause(gs.ctx) != nil { + return false + } if !gs.statsUpdateGenerationActive(key, generation) { if gs.mu.cond != nil { gs.mu.cond.Broadcast() @@ -1842,6 +1940,16 @@ func CollectAndCalculateStats(ctx context.Context, req *updateStatsRequest, exec defer func() { v2.TxnStatementUpdateStatsDurationHistogram.Observe(time.Since(start).Seconds()) }() + // The zero-object fast path below does not enter ForeachVisibleObjects. Keep + // executor shutdown as a failed refresh by checking its authoritative + // lifecycle before any successful early return. + if executor != nil { + if lifecycle := executor.LifecycleContext(); lifecycle != nil { + if cause := context.Cause(lifecycle); cause != nil { + return 0, cause + } + } + } lenCols := len(req.tableDef.Cols) - 1 /* row-id */ info := plan2.NewTableStatsInfo(lenCols) if req.approxObjectNum == 0 { diff --git a/pkg/vm/engine/disttae/stats_test.go b/pkg/vm/engine/disttae/stats_test.go index 24fce09aafddd..daed5f4c76fb7 100644 --- a/pkg/vm/engine/disttae/stats_test.go +++ b/pkg/vm/engine/disttae/stats_test.go @@ -1758,7 +1758,8 @@ func TestRemoveTid(t *testing.T) { assert.False(t, enqueueAfterCleanup) assert.Nil(t, queuedAfterCleanup) gs.completeAutomaticStatsCacheUpdate(k1, generation, plan2.NewStatsInfo(), true) - gs.completeStatsRefresh(k1, generation, true, 3, 1, func() {}) + gs.completeAutomaticStatsRefresh( + k1, generation, plan2.NewStatsInfo(), true, 3, 1, func() {}) gs.mu.Lock() _, ok1 := gs.mu.statsInfoMap[k1] @@ -1785,7 +1786,8 @@ func TestRemoveTid(t *testing.T) { gs.updatingMu.updating[k1] = replacement gs.updatingMu.Unlock() gs.completeAutomaticStatsCacheUpdate(k1, generation, plan2.NewStatsInfo(), true) - gs.completeStatsRefresh(k1, generation, true, 4, 0.5, func() {}) + gs.completeAutomaticStatsRefresh( + k1, generation, plan2.NewStatsInfo(), true, 4, 0.5, func() {}) _, oldGenerationStarted := gs.startAutomaticUpdate(k1, generation) assert.False(t, oldGenerationStarted, "an old queued generation should be rejected") @@ -1888,6 +1890,42 @@ func TestRemoveTid(t *testing.T) { } +func TestStatsPublicationRejectsStoppedOwnerLifecycle(t *testing.T) { + ownerCtx, stopOwner := context.WithCancel(context.Background()) + stopOwner() + key := statsinfo.StatsInfoKey{AccId: 1, DatabaseID: 10, TableID: 42} + lastGood := plan2.NewStatsInfo() + lastGood.TableCnt = 7 + generation := &updateRecord{ + inProgress: true, + baseObjectCount: 7, + samplingRatio: 0.25, + } + gs := &GlobalStats{ctx: ownerCtx} + gs.mu.statsInfoMap = map[statsinfo.StatsInfoKey]*statsinfo.StatsInfo{key: lastGood} + gs.mu.cond = sync.NewCond(&gs.mu) + gs.updatingMu.updating = map[statsinfo.StatsInfoKey]*updateRecord{key: generation} + + fresh := plan2.NewStatsInfo() + fresh.TableCnt = 42 + releases := 0 + gs.completeAutomaticStatsRefresh( + key, generation, fresh, true, 42, 1, func() { releases++ }) + require.False(t, gs.publishStatsForGeneration(key, generation, fresh)) + require.Equal(t, 1, releases) + + gs.mu.Lock() + require.Same(t, lastGood, gs.mu.statsInfoMap[key], + "shutdown must preserve the last successfully published statistics") + gs.mu.Unlock() + gs.updatingMu.Lock() + require.False(t, generation.inProgress) + require.Equal(t, int64(7), generation.baseObjectCount, + "failed publication must not advance the object-count baseline") + require.Equal(t, 0.25, generation.samplingRatio) + gs.updatingMu.Unlock() +} + func TestGlobalStatsGetDoesNotHoldMuWhileSubscribing(t *testing.T) { runTest(t, func(ctx context.Context, e *Engine) { gs := e.globalStats diff --git a/pkg/vm/engine/disttae/txn_table.go b/pkg/vm/engine/disttae/txn_table.go index 524a4416dce14..33c02f0c530ac 100644 --- a/pkg/vm/engine/disttae/txn_table.go +++ b/pkg/vm/engine/disttae/txn_table.go @@ -373,6 +373,15 @@ func ForeachVisibleObjects( if cause := context.Cause(ctx); cause != nil { return cause } + var executorLifecycle context.Context + if executor != nil { + executorLifecycle = executor.LifecycleContext() + if executorLifecycle != nil { + if cause := context.Cause(executorLifecycle); cause != nil { + return cause + } + } + } iter, err := state.NewObjectsIter(ts, true, visitTombstone) if err != nil { return err @@ -381,16 +390,21 @@ func ForeachVisibleObjects( taskCtx, cancelTasks := context.WithCancelCause(ctx) defer cancelTasks(nil) - if executor != nil { - if lifecycle := executor.LifecycleContext(); lifecycle != nil { - stopLifecycleWatch := context.AfterFunc(lifecycle, func() { - cause := context.Cause(lifecycle) - if cause == nil { - cause = context.Canceled - } - cancelTasks(cause) - }) - defer stopLifecycleWatch() + if executorLifecycle != nil { + stopLifecycleWatch := context.AfterFunc(executorLifecycle, func() { + cause := context.Cause(executorLifecycle) + if cause == nil { + cause = context.Canceled + } + cancelTasks(cause) + }) + defer stopLifecycleWatch() + // Close the check/register race. AfterFunc is only a cancellation + // delivery mechanism; its callback runs asynchronously and is not the + // authoritative lifecycle predicate. + if cause := context.Cause(executorLifecycle); cause != nil { + cancelTasks(cause) + return cause } } var ( @@ -448,6 +462,14 @@ func ForeachVisibleObjects( if cause := context.Cause(taskCtx); cause != nil { return cause } + // The lifecycle callback can be scheduled but not yet run. Read the + // executor-owned predicate directly before declaring the joined group a + // success. + if executorLifecycle != nil { + if cause := context.Cause(executorLifecycle); cause != nil { + return cause + } + } } if err != nil { return err diff --git a/pkg/vm/engine/disttae/util_test.go b/pkg/vm/engine/disttae/util_test.go index c970f0a064157..15d936058201d 100644 --- a/pkg/vm/engine/disttae/util_test.go +++ b/pkg/vm/engine/disttae/util_test.go @@ -744,6 +744,96 @@ func visibleObjectStateForExecutorTest(t *testing.T, count int) *logtailreplay.P return state } +// delayedLifecycleContext models a conforming lifecycle whose Done predicate is +// observable before its AfterFunc notification is dispatched. This keeps the +// cancellation race deterministic without sleeps or scheduler assumptions. +type delayedLifecycleContext struct { + done chan struct{} + + mu sync.Mutex + err error + callbacks []*delayedLifecycleCallback + // cancelOnRegister closes Done while context.AfterFunc is registering its + // callback, but before that callback is dispatched. + cancelOnRegister bool +} + +type delayedLifecycleCallback struct { + active bool + fn func() +} + +func newDelayedLifecycleContext() *delayedLifecycleContext { + return &delayedLifecycleContext{done: make(chan struct{})} +} + +func (c *delayedLifecycleContext) Deadline() (time.Time, bool) { return time.Time{}, false } +func (c *delayedLifecycleContext) Done() <-chan struct{} { return c.done } +func (c *delayedLifecycleContext) Value(any) any { return nil } + +func (c *delayedLifecycleContext) Err() error { + c.mu.Lock() + defer c.mu.Unlock() + return c.err +} + +func (c *delayedLifecycleContext) AfterFunc(fn func()) func() bool { + c.mu.Lock() + callback := &delayedLifecycleCallback{active: true, fn: fn} + c.callbacks = append(c.callbacks, callback) + if c.cancelOnRegister && c.err == nil { + c.err = context.Canceled + close(c.done) + } + c.mu.Unlock() + return func() bool { + c.mu.Lock() + defer c.mu.Unlock() + if !callback.active { + return false + } + callback.active = false + return true + } +} + +func (c *delayedLifecycleContext) cancelWithoutNotification() { + c.mu.Lock() + defer c.mu.Unlock() + if c.err != nil { + return + } + c.err = context.Canceled + close(c.done) +} + +type inlineLifecycleExecutor struct { + lifecycle context.Context +} + +func (e *inlineLifecycleExecutor) AppendTask( + ctx context.Context, + task concurrentTask, + complete func(error), +) error { + if cause := context.Cause(ctx); cause != nil { + return cause + } + err := task() + if complete != nil { + complete(err) + } + return nil +} + +func (*inlineLifecycleExecutor) Run(context.Context) {} + +func (e *inlineLifecycleExecutor) LifecycleContext() context.Context { + return e.lifecycle +} + +func (*inlineLifecycleExecutor) GetConcurrency() int { return 1 } + func TestForeachVisibleObjectsPropagatesConcurrentTaskError(t *testing.T) { state := visibleObjectStateForExecutorTest(t, 2) ex := newConcurrentExecutor(2) @@ -918,6 +1008,60 @@ func TestForeachVisibleObjectsRejectsExecutorShutdownWhenTaskReturnsNil(t *testi } } +func TestForeachVisibleObjectsReadsLifecyclePredicateAfterJoin(t *testing.T) { + state := visibleObjectStateForExecutorTest(t, 1) + lifecycle := newDelayedLifecycleContext() + ex := &inlineLifecycleExecutor{lifecycle: lifecycle} + + err := ForeachVisibleObjects( + context.Background(), state, types.MaxTs(), + func(context.Context, objectio.ObjectEntry) error { + lifecycle.cancelWithoutNotification() + return nil + }, + ex, + false, + ) + require.ErrorIs(t, err, context.Canceled, + "executor shutdown is a failed traversal even before async notification dispatch") +} + +func TestForeachVisibleObjectsClosesLifecycleWatcherRegistrationRace(t *testing.T) { + state := visibleObjectStateForExecutorTest(t, 1) + lifecycle := newDelayedLifecycleContext() + lifecycle.cancelOnRegister = true + ex := &inlineLifecycleExecutor{lifecycle: lifecycle} + var called atomic.Bool + + err := ForeachVisibleObjects( + context.Background(), state, types.MaxTs(), + func(context.Context, objectio.ObjectEntry) error { + called.Store(true) + return nil + }, + ex, + false, + ) + require.ErrorIs(t, err, context.Canceled) + require.False(t, called.Load(), + "work must not be admitted after lifecycle cancellation becomes observable") +} + +func TestCollectAndCalculateStatsRejectsStoppedExecutorOnZeroObjectFastPath(t *testing.T) { + lifecycle, cancel := context.WithCancel(context.Background()) + cancel() + ex := &inlineLifecycleExecutor{lifecycle: lifecycle} + req := &updateStatsRequest{ + statsInfo: plan2.NewStatsInfo(), + tableDef: &plan.TableDef{Cols: []*plan.ColDef{{Name: "__mo_rowid"}}}, + approxObjectNum: 0, + } + + _, err := CollectAndCalculateStats(context.Background(), req, ex) + require.ErrorIs(t, err, context.Canceled, + "the zero-object fast path must not bypass executor lifecycle failure") +} + func TestCollectAndCalculateStatsDoesNotApplyFailedObjectScan(t *testing.T) { ctx := context.Background() state := logtailreplay.NewPartitionState("", true, 42, false) From b17b46e6ada68a012559e992ea4d2ef7ef93073a Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 18:49:48 +0800 Subject: [PATCH 17/18] ci: bound SCA lint memory on small runners --- .github/workflows/entrypoint.yaml | 2 +- Makefile | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/entrypoint.yaml b/.github/workflows/entrypoint.yaml index 444bb082b4a73..5e8c0608b1a20 100644 --- a/.github/workflows/entrypoint.yaml +++ b/.github/workflows/entrypoint.yaml @@ -73,7 +73,7 @@ jobs: name: Matrixone CI needs: check-pr-valid if: ${{ needs.check-pr-valid.outputs.pr_valid == 'true' && github.base_ref != '3.0-dev' }} - uses: matrixorigin/CI/.github/workflows/ci.yaml@main + uses: matrixorigin/CI/.github/workflows/ci.yaml@codex/cap-sca-lint-memory with: ut_parallel: 8 secrets: inherit diff --git a/Makefile b/Makefile index 38b896af73d73..967042d9fe2e6 100644 --- a/Makefile +++ b/Makefile @@ -1315,11 +1315,13 @@ install-static-check-tools: @go install github.com/apache/skywalking-eyes/cmd/license-eye@v0.4.0 .PHONY: static-check +GOLANGCI_LINT_CONCURRENCY ?= +GOLANGCI_LINT_CONCURRENCY_FLAG := $(if $(strip $(GOLANGCI_LINT_CONCURRENCY)),--concurrency $(strip $(GOLANGCI_LINT_CONCURRENCY))) static-check: config err-check $(CGO_OPTS) go vet $(GO_MODULE_MODE) -vettool=`which molint` ./... $(CGO_OPTS) license-eye -c .licenserc.yml header check $(CGO_OPTS) license-eye -c .licenserc.yml dep check - $(CGO_OPTS) golangci-lint run -v -c .golangci.yml ./... + $(CGO_OPTS) golangci-lint run -v $(GOLANGCI_LINT_CONCURRENCY_FLAG) -c .golangci.yml ./... fmtErrs := $(shell grep -onr 'fmt.Errorf' pkg/ --exclude-dir=.git --exclude-dir=vendor \ --exclude=*.pb.go --exclude=*_test.go --exclude=system_vars.go --exclude=Makefile) From 15534e2ed6277c7d29487ba52ef5c60cc5fc596d Mon Sep 17 00:00:00 2001 From: XuPeng-SH Date: Fri, 28 Aug 2026 18:54:36 +0800 Subject: [PATCH 18/18] ci: keep reusable workflow on trusted main --- .github/workflows/entrypoint.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/entrypoint.yaml b/.github/workflows/entrypoint.yaml index 5e8c0608b1a20..444bb082b4a73 100644 --- a/.github/workflows/entrypoint.yaml +++ b/.github/workflows/entrypoint.yaml @@ -73,7 +73,7 @@ jobs: name: Matrixone CI needs: check-pr-valid if: ${{ needs.check-pr-valid.outputs.pr_valid == 'true' && github.base_ref != '3.0-dev' }} - uses: matrixorigin/CI/.github/workflows/ci.yaml@codex/cap-sca-lint-memory + uses: matrixorigin/CI/.github/workflows/ci.yaml@main with: ut_parallel: 8 secrets: inherit