fix(view): activate durable metadata refresh - #27734
Conversation
…in-clean-fix # Conflicts: # pkg/bootstrap/versions/v4_0_6/upgrade_test.go
…in-clean-fix # Conflicts: # pkg/sql/compile/view_metadata_recovery.go # pkg/sql/compile/view_metadata_recovery_test.go
…in-clean-fix # Conflicts: # pkg/frontend/mysql_cmd_executor.go # pkg/frontend/plan_cache.go # pkg/frontend/session.go
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.
The mandatory design gate is still open, and the remaining gaps affect the architecture rather than implementation polish.
-
Both the PR body and docs/design/20260827-view-metadata-lifecycle.md explicitly say Pending re-approval and record no auditable approval for the semantic checkpoint. This is an R3 distributed lifecycle/protocol change across HAKeeper, LogService, CN, catalog, frontend, planner, mixed-version restart and rollback. The document's own acceptance gate requires an exact reviewed revision and traceable approval before implementation approval.
-
The failure-containment contract is not closed. After a valid admission response, renewViewMetadataAuthorityLease installs a timer; heartbeat loss beyond that lease reaches expireViewMetadataAuthority -> revokeViewMetadataGeneration, which seals all ingress, stops the frontend, and asynchronously closes the entire CN. Therefore a sufficiently long transient HAKeeper/CN partition can remove every CN and interrupt ordinary SQL, not only lifecycle-sensitive metadata reads. The design needs to state and justify the availability/RTO budget for this blast radius, explain why a narrower fail-closed boundary cannot preserve the invariant, and provide deterministic outage/partition acceptance evidence.
-
The design puts a mutex-backed provisional lease plus an O(AST-nodes) detector on relation-reading statement paths, including prepared/cached paths, but provides no numeric latency/allocation/contention budget or base-vs-head evidence. Add an explicit acceptance threshold and representative before/after measurements for simple TP reads, cached/prepared execution, and metadata-sensitive statements.
Please obtain approval on the exact revised design checkpoint first. Per the design-first gate, implementation approval remains blocked until these decisions are resolved and recorded; I intentionally did not treat green CI as proof of the missing availability and hot-path contracts.
aunjgr
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 706359696ab071fe8cc6c53fe4747c57eb9a61c4 against merge base 1a4f656bb2b5b7f2fb1a6d951c4806ce7d124ba5 and newest main. This is an R3 distributed lifecycle/protocol activation across HAKeeper RSM, LogService wire state, CN generation/admission, catalog durability, frontend ingress, planner classification, restart and rollback. The mandatory design gate remains explicitly open: both the PR body and docs/design/20260827-view-metadata-lifecycle.md say Pending re-approval, record no auditable reviewer/approval for the semantic checkpoint, and state implementation approval is blocked until that record exists.
Two architectural decisions still require approval and evidence before the code can be accepted:
-
Authority-lease expiry has whole-CN blast radius. A long HAKeeper-only partition reaches
expireViewMetadataAuthority -> revokeViewMetadataGeneration, synchronously seals ingress/stops the frontend and requests asynchronous full CN closure. The deterministic test confirms exactly that behavior while an existing metadata reader is blocked. A control-plane partition affecting all CNs can therefore remove the entire SQL serving fleet and interrupt ordinary, metadata-insensitive work. Record and justify the availability/RTO budget, why a narrower metadata-sensitive fail-closed boundary cannot preserve the invariant, and provide deterministic multi-CN partition/recovery acceptance evidence for the chosen blast radius. -
Relation-reading statement admission adds a mutex-backed provisional lease plus O(AST-nodes) classification to ordinary, prepared and cached paths. The design lists the mechanism but no numeric latency/allocation/contention threshold or base-vs-head evidence for simple TP reads, cached/prepared reuse, and lifecycle-sensitive statements. Establish that hot-path budget and demonstrate it on the exact semantic checkpoint.
The epoch ownership, durable required/completion transitions, stale-generation fencing, worker bounds, restart/rollback tests, cursor epoch handling, and catalog publication logic are otherwise extensively covered. Obtain traceable approval of the final design revision after closing these decisions, then record implementation conformance against that exact revision; green CI cannot substitute for the explicitly pending design/availability gate.
What type of PR is this?
Which issue(s) this PR fixes:
issue #26227
What this PR does / why we need it:
This is the final activation layer for the View metadata lifecycle built by #27267 and #27370 on top of the durable admission contract from #27430.
It:
READYstate;DESC,SHOW COLUMNS,information_schema.columns, and current-catalog CTAS while ordinary View queries continue to rebind the stored SQL;The activation authority is the replicated HAKeeper admission epoch. No local
MOClustersnapshot can independently authorize refresh.Validation:
-race -count=100;SHOWand access-control BVT;go vet,git diff --check(excluding established.resulttrailing-space formatting), and a finalmo-servicebuild.Design artifact
d0b5e7ac2d75c67cdac777ced8921ac1072f9401; documentation/evidence head:5d59cccd0d4e3b5e189786ffb122116ffc79c2c5.Pending re-approval. There is currently no auditable reviewer identity or GitHub approval record; this PR must not be treated as design-approved until one is recorded.Review closure evidence
information_schema.columns; comments/formatting cannot bypass the fence, while CTE aliases and string literals do not create false positives.5d59cccd0d4e3b5e189786ffb122116ffc79c2c5; exact old/base binary:6b8987741df912f38cece5bd026349d79672faf9.VARCHAR(12)), one-new/one-old capability-disabled legacy metadata (VARCHAR(12)), all-new activation (VARCHAR(48)), same-UUID CN restart, old-binary rollback denied before ingress, new-binary recovery, snapshot restore revalidation, and immediate whole-cluster restart.6f15067b02fixed Rows lifecycle SCA findings and added bounded lifecycle waits to four pre-existing View BVTs; the lifecycle BVT followed by all four affected tests passed220/220. That historical BVT/build/lint evidence remains supplementary rather than being relabeled as current-head evidence.d0b5e7ac2d75c67cdac777ced8921ac1072f9401separates provisional epoch admission from metadata authority: ordinary base-table and binary prepared/cached planning remain available after authority expiry, while SHOW COLUMNS and information_schema.columns fail closed and recover after renewal. The frontend boundary test passes normally and with-race -count=100; full frontend, compile, and CN owning-package UTs pass.TestIssue26114CrossAccountBranchQuotaAndOwnershipfailure is outside this View lifecycle diff; Coverage failed only because a required producer failed.if: false; those SKIPPED jobs are not counted as evidence.Review round 2 closure
TestViewMetadataInitialRefreshLeaseBlocksFirstAdvanceproves the firstAdvance(1)cannot publish until that lease drains.discovery=5s,owner remaining=30.1syields a 36s budget rather than the old fixed 10s.columnsshadowed by a CTE under defaultinformation_schemais ordinary work, while an explicitly qualified relation and a realinformation_schema.columnsreference inside a CTE still acquire the lease.49364cd460, superseded and not current-head evidence) mixed-version run against base (6b8987741d) passed all-old, partial fail-closed, all-new activation, snapshot restore, same-UUID restart, old-binary rollback denial, recovery, and immediate full-cluster restart.