Skip to content

fix(frontend): publish ANALYZE stats by table generation - #27758

Merged
XuPeng-SH merged 29 commits into
mainfrom
xp/codex/analyze-table-stats-publication
Aug 28, 2026
Merged

fix(frontend): publish ANALYZE stats by table generation#27758
XuPeng-SH merged 29 commits into
mainfrom
xp/codex/analyze-table-stats-publication

Conversation

@XuPeng-SH

@XuPeng-SH XuPeng-SH commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What changed

This PR fixes the statistics-publication half of #27728. A successful ANALYZE TABLE now establishes a synchronous, CN-local optimizer-statistics boundary:

  • refresh and swap the table's disttae optimizer statistics before returning success;
  • version cache dependencies by the physical (account ID, table ID) owner rather than globally flushing session caches;
  • preserve the physical owner through snapshot-tenant, publication-publisher, cluster-table, and special system-table resolution;
  • record the versions actually read by each cached statement, including plans whose final shape no longer exposes every statistics dependency;
  • reject stale plan-cache admission when publication crosses compilation;
  • reject a slow old-generation statistics read from repopulating a session cache after publication;
  • lazily evict only plans and session statistics that depend on the analyzed table;
  • serialize same-table explicit publications and explicit/automatic engine refreshes with context-cancelable, fixed-size striped admission.

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

  • The version advances only after the engine refresh and local cache swap succeed. Refresh errors, cancellation, and nil results do not invalidate usable plans or statistics.
  • A statement already compiling may finish with its captured statistics, but it cannot enter the plan cache after crossing the publication boundary.
  • Updating one statement in a multi-statement cached query replaces only that statement's dependency set; dependencies from the other statements remain protected.
  • Unrelated tables and the same table ID in another account remain cached. Reusing a session cache under a different physical account cannot expose the old account's entry.
  • Only persistent physical relation kinds participate. Views, external/sequence/source/temporary relations, and unresolved relations do not publish optimizer statistics.
  • Physical-account resolution follows relation lookup precedence, including publisher ownership for publication-backed tables and system ownership for special system tables.
  • Engines without the optional synchronous refresh capability retain the existing ANALYZE result behavior.
  • The guarantee is CN-local. Cross-CN invalidation and prepared-statement lifecycle changes are not claimed here.
  • Multi-table ANALYZE publishes each successfully refreshed table independently; optimizer statistics are freshness metadata, not transactional table data.

All new process-lifetime state is bounded:

  • 64 fixed frontend publication stripes and 64 fixed engine refresh stripes;
  • at most 64K explicit (account, table) version entries, followed by reset-token compaction that makes every older label conservatively stale;
  • session version metadata resets in lockstep with the existing bounded statistics cache;
  • per-plan dependency maps are bounded by statement dependencies and the existing plan-cache LRU.

Validation

The branch is integrated with main at 1d3483ac97. Full package tests pass for:

  • pkg/frontend
  • pkg/sql/plan
  • pkg/vm/engine
  • pkg/vm/engine/disttae

Full race-enabled package tests pass for pkg/frontend and pkg/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:

package main candidate
frontend 63.837401% (27828/43592) 63.916349% (27996/43801)
sql/plan 78.787938% (40523/51433) 78.789293% (40530/51441)
disttae 40.591949% (4951/12197) 40.666176% (4969/12219)

The cache-hit version check is allocation-free and measured locally at:

actual statistics dependencies time
0 ~1.5ns
1 ~47ns
4 ~56ns
16 ~131–136ns

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 NDV 2, and the service remains healthy;
  • ANALYZE TABLE base_t(a) still publishes ordinary-table statistics and returns NDV 2;
  • the existing 10,000,000-row Q35 reproduction changes an already connected session from the stale non-shuffle plan to 16-way hash ownership immediately after a 181ms ANALYZE TABLE hits(URL), without reconnecting or restarting;
  • the deterministic 50,000-row IVF-FLAT gate returns the same top-10 result, continues to use Vector Index Scan, and shows no measurable performance regression.

Part of #27728
Related: #27685, #27744, #27753

Latest request-changes closure

At head 7ba35a4cb7:

  • ANALYZE inside 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.
  • automatic-refresh scheduling metadata is committed before same-table admission is released, including explicit cleanup of a generation canceled during admission;
  • synchronous refresh returns the exact statistics object published inside admission, avoiding a second map read after release.

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

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

Systemic follow-up pushed in ec522c3.

The repair now treats the refresh path as one state machine rather than independent wake-up patches:

  • request-driven generations require the exact live subscription owner;
  • queued and running producer ownership is accounted under the scheduling mutex;
  • synchronous Get waits on a durable predicate covering cache publication, exact-generation producer liveness, cleanup/replacement, caller cancellation, and GlobalStats shutdown;
  • every terminal producer transition notifies waiters;
  • lifecycle notification is one callback per GlobalStats, not one callback per miss.

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.

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

@aptend The two blockers from your review at d3e3b28286 are closed on the current head b3aa6492846d; I have re-requested your review.

  • Active user transactions keep the workspace-visible ANALYZE result and skip process-global committed-only stats publication. Regressions cover transaction-created tables and uncommitted DML.
  • Automatic refresh publication and scheduling metadata now complete before same-table admission is released. Explicit refresh returns the exact object published inside admission, avoiding the post-release re-read race.

Fresh exact-head validation:

  • focused race stress: both directly affected tests pass with -count=100;
  • full pkg/frontend race: pass;
  • full pkg/vm/engine/disttae race: pass.

No additional code change was made for the stale review state; both existing review threads are already resolved.

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@XuPeng-SH

XuPeng-SH commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

SCA runner-loss fix prepared in MatrixOne head 15534e2ed6 and CI PR matrixorigin/CI#437.

Root cause: the failed job produced no lint finding; the self-hosted arm64-mo-shanghai-4c8g runner lost communication during static analysis. On the exact PR head, a cold full-repository golangci-lint 2.6.2 run peaked at 8.56 GiB, exceeding the runner's 8 GiB capacity.

The fix preserves the complete SCA rule/package set:

  • MatrixOne make static-check accepts an opt-in concurrency bound while keeping automatic concurrency by default.
  • CI#437 makes only the 8 GiB ARM SCA job set the bound to 1.
  • Cold-cache peak fell to 6.23 GiB; the scan passed with 0 issues.
  • Full make static-check GOLANGCI_LINT_CONCURRENCY=1 passed.

Because this workflow is triggered by pull_request_target, GitHub reads MatrixOne's entrypoint from the base branch; an unmerged reusable-workflow branch cannot be selected from this PR head. Therefore CI#437 must merge first, then this PR's SCA job should be rerun. The ineffective temporary workflow reference was removed; the final MatrixOne delivery diff for this fix is Makefile-only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants