fix(frontend): publish ANALYZE stats by table generation - #27758
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review conclusion: merge-blocking.\n\nReviewed exact base 32053f5 and head ecce917. Because this is my own PR, GitHub does not allow me to submit REQUEST_CHANGES; please treat this COMMENTED review as request changes.\n\n1. [blocking / service crash] The new post-ANALYZE refresh is invoked for views, although views do not own physical optimizer statistics.\n\npkg/frontend/mysql_cmd_executor.go:2086-2134 excludes historical, temporary, and publication-backed objects, but does not require a physical relation kind before calling StatsRefresher. pkg/vm/engine/disttae/txn_table.go:255-260 already encodes the opposite contract: Stats returns nil for relkind V.\n\nI verified this with a real A/B run on 10.222.1.55, using the same persisted schema/data and port:\n\n- base behavior: ANALYZE TABLE view_v(a) succeeds, returns approx_count_distinct(a)=2, and costs about 0.003s;\n- exact PR head: the same statement loses the client connection and crashes the embedded service with panic: fake primary key not existed: v-view_v. The stack is Schema.getFakePrimaryKey -> HandleSyncLogTailReq -> LogtailServer.getSubLogtailPhase after subscribing table 457107 view_v.\n\nThis is not only an ANALYZE error; one valid SQL statement terminates the whole service. The refresh eligibility contract should be positive: refresh only relation kinds that physically own optimizer statistics, rather than growing an exclusion list. Add an end-to-end SQL regression proving ANALYZE VIEW preserves the legacy derived-query result and never subscribes or refreshes the view table. Derive and test the complete supported/non-physical relation-kind matrix as well.\n\n2. [blocking / incomplete invalidation] The claimed accountID+tableID dependency is reduced to tableID plus the current session account, so cross-account physical statistics cannot be invalidated correctly.\n\n- TxnCompilerContext.Stats calls getStatsCacheVersion(tableID) before doStatsHeavyWork resolves the physical relation account.\n- Session.optimizerStatsKey always uses ses.GetAccountId at pkg/frontend/session.go:808-812.\n- The wrapper and cached plan retain only map[uint64]uint64 at pkg/frontend/computation_wrapper.go:112 and 213-221.\n- Admission validates every dependency using one ses.GetAccountId at pkg/frontend/session.go:1538-1541 and pkg/frontend/server.go:705-724.\n- But relation access can switch to a snapshot tenant, publication publisher, or sys account for cluster/system tables at pkg/frontend/compiler_context.go:432-454 and 1185-1203. ResolveViewDependencyAccount already documents the required resolution order.\n- The ANALYZE publisher also takes defines.GetAccountId(ctx) at pkg/frontend/mysql_cmd_executor.go:2116 instead of the effective physical owner.\n\nConcrete counterexample: a tenant plan reads optimizer statistics for a cluster/system table from the sys account, but records and later validates tenant+tableID. A sys-account ANALYZE advances sys+tableID, while that tenant session cache and plan cache remain current indefinitely. The current same-tableID/different-account unit test only proves isolation; it never represents a plan whose source account differs from the session account.\n\nCarry optimizerStatsTableKey, including effective source account, end to end through session stats cache, plan dependency recording, cache admission, slow-writeback validation, and ANALYZE publication. Resolve that account with the same rules as physical relation lookup. Add regressions for tenant reads of cluster/system tables and each supported snapshot/publication path.\n\nI also checked publication ordering, stale slow-path writeback rejection, cache admission, 64K version-map compaction, striped same-key admission/cancellation release, multi-table partial-publication policy, and reset/reuse paths. I did not find another merge blocker there.\n\nThe current multi-CN BVT failure is revoked_default_role_login.sql expecting the recently revoked default role but receiving public. The reviewed diff does not touch the role/authentication paths; I am not using that failure as evidence for either finding above, and I did not wait for the remaining pending UT.
…tats-publication-final # Conflicts: # pkg/frontend/computation_wrapper.go # pkg/frontend/mysql_cmd_executor.go # pkg/frontend/plan_cache.go # pkg/frontend/session.go
|
Systemic follow-up pushed in ec522c3. The repair now treats the refresh path as one state machine rather than independent wake-up patches:
Deterministic regressions cover cleanup both before and after Cond.Wait registration, shared-producer cancellation, worker shutdown, subscription replacement/ownerlessness, and unchanged nonblocking publication semantics. Full disttae package tests, package race tests, go vet, and adaptive race stress (11-14 repetitions per new concurrency case) pass locally. Exact-head package timing was 8.145s before and 8.198s after, within noise; the normal cache-hit path is unchanged. CI was triggered by the push but was not awaited. |
|
@aptend The two blockers from your review at
Fresh exact-head validation:
No additional code change was made for the stale review state; both existing review threads are already resolved. |
aptend
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 29f69dd.
I read the complete review history, inline discussions, author replies, and resolved thread state, then checked both the increment since the previously reviewed head and the complete PR diff. The two previous blockers are closed in code: ANALYZE publication is suppressed for statements that begin in an active transaction, and automatic refresh now commits cache/scheduling metadata before releasing table-scoped admission while explicit refresh returns its exact collected result.
I also audited the full publication/versioning path and the engine refresh lifecycle, including cleanup ownership, waiter termination on cancellation/shutdown/unsubscribe, generation-token identity, same-table serialization, bounded queues/maps/caches, and failed-refresh last-good behavior. I found no remaining blocking correctness issue.
Validation on this exact head:
- go test ./pkg/frontend ./pkg/sql/plan ./pkg/vm/engine ./pkg/vm/engine/disttae -count=1
- targeted -race tests for frontend stats/ANALYZE/publication/refresh paths and disttae stats/refresh/executor/visible-object paths, count=3
- go vet on the four affected packages
- git diff --check
|
SCA runner-loss fix prepared in MatrixOne head Root cause: the failed job produced no lint finding; the self-hosted The fix preserves the complete SCA rule/package set:
Because this workflow is triggered by |
What changed
This PR fixes the statistics-publication half of #27728. A successful
ANALYZE TABLEnow establishes a synchronous, CN-local optimizer-statistics boundary:(account ID, table ID)owner rather than globally flushing session caches;The implementation does not assume an NDV when statistics are missing and does not add query-, schema-, or data-shape special cases.
Correctness and lifecycle boundaries
ANALYZEresult behavior.ANALYZEpublishes each successfully refreshed table independently; optimizer statistics are freshness metadata, not transactional table data.All new process-lifetime state is bounded:
(account, table)version entries, followed by reset-token compaction that makes every older label conservatively stale;Validation
The branch is integrated with main at
1d3483ac97. Full package tests pass for:pkg/frontendpkg/sql/planpkg/vm/enginepkg/vm/engine/disttaeFull race-enabled package tests pass for
pkg/frontendandpkg/vm/engine/disttae. Focused publication, account ownership, physical-relation eligibility, plan-generation interaction, cache-boundary, compaction, same-table ordering, cancellation, failed admission, automatic/explicit refresh admission, and stale-writeback tests pass for 20 repetitions under race detection.go build,go vet, format, and diff checks pass for all affected Go packages.The new tests use deterministic in-memory state, shared setup helpers, no sleeps, no retries, and no large fixtures.
Exact statement coverage against the integrated main does not decrease:
The cache-hit version check is allocation-free and measured locally at:
Plans with no optimizer-statistics dependency take a lock-free fast path.
Real-service validation
On
10.222.1.55, the public MySQL path was validated against the existing persisted deployment:ANALYZE TABLE view_v(a)no longer reaches TAE statistics refresh for the view, returns the correct NDV2, and the service remains healthy;ANALYZE TABLE base_t(a)still publishes ordinary-table statistics and returns NDV2;ANALYZE TABLE hits(URL), without reconnecting or restarting;Vector Index Scan, and shows no measurable performance regression.Part of #27728
Related: #27685, #27744, #27753
Latest request-changes closure
At head
7ba35a4cb7:ANALYZEinside a pre-existing user transaction keeps the transaction-visible derived result and skips process-global statistics publication; transaction-created tables and uncommitted DML are covered.Exact-head validation includes full affected-package UTs, full frontend/disttae race packages, focused race stress x100, and a real embedded SQL reproduction for both transactional cases. Coverage versus exact integrated main is non-decreasing: frontend 63.7% to 63.9%, sql/plan 78.642825624% to 78.644184842%, vm/engine unchanged at 9.2%, and disttae 40.4% to 40.6%.