Skip to content

fix: fence DDL visibility across CN admission - #27756

Open
ck89119 wants to merge 32 commits into
matrixorigin:mainfrom
ck89119:issue-27743-main
Open

fix: fence DDL visibility across CN admission#27756
ck89119 wants to merge 32 commits into
matrixorigin:mainfrom
ck89119:issue-27743-main

Conversation

@ck89119

@ck89119 ck89119 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #27743

What this PR does / why we need it:

A successful client DDL commit could be followed by a fresh proxy-routed connection on another CN whose applied logtail was still older than the DDL commit. The new connection could therefore fail catalog lookup with no such table.

This PR adds a two-sided DDL visibility fence:

  • a CN publishes DDLVisibilityBarrierReady only after QueryService is listening, synchronizes the maximum published frontier, and only then opens public ingress;
  • startup rollback and ordinary shutdown stop periodic heartbeats, publish DDLVisibilityBarrierReady=false and ingress-ready=false, wait for the authoritative inventory to observe the withdrawal, and only then close QueryService;
  • a DDL commit synchronizes every barrier-ready CN generation, refreshes authoritative membership after fan-out, and retries until the generation/address set is stable;
  • if an RPC from an older membership snapshot fails, the sender authoritatively revalidates the exact service/generation/address tuple: a withdrawn or replaced target restarts fan-out, while a still-current target preserves the partial-barrier error;
  • DDL no-op commits synchronize the transaction's observed catalog frontier;
  • automatic DDL fencing uses the new cancellable SyncCommitV2 method, gated by MORPC v36, so rolling upgrades never invoke the legacy receiver's fatal wait path; upstream v35 remains exclusively assigned to scaled-variance wire state;
  • live pre-v36→v36 activation establishes a distributed DDL-producer cut: the control plane concurrently sends the complete CN target set; every CN blocks/drains local DDL, keeps its barrier receiver reachable, waits for all targets to report Prepared, applies the converged frontier, waits for all targets to report Fenced, then restores ingress and releases DDL;
  • compiled/default runtime v36 is not treated as proof of that global cut: local startup Prepared/Fenced and complete-target activation completion are separate states, and only a successful full-target activation enables the idempotent fast path;
  • activation failures remain ingress-withdrawn with DDL blocked while retaining the v36 receiver capability; retries resolve hidden CNs through authoritative raw inventory and fence the recovery RPC by generation/query-address identity;
  • periodic heartbeat snapshot/send is serialized with activation publication, preventing stale same-generation readiness from overtaking a withdrawal; activation also preserves pre-start ingress=false until all public listeners are live;
  • activation synchronously blocks new DDL admission before attempting the fallible ingress withdrawal, then drains admitted old-protocol DDL; withdrawal/drain failures retain the blocked gate, and client-owned background DDL admitted before shutdown retains cross-CN fan-out eligibility after gate close while bootstrap background DDL remains local;
  • the receiver propagates logtail lag, timeout, and cancellation instead of acknowledging an incomplete fence.

The startup handshake and post-fan-out membership check close both admission orderings: a CN published before the final check is included in the DDL retry, while a CN published afterwards catches up from the frontier already stored by the completed fan-out.

Validation:

  • deterministic membership-addition, generation-replacement, stale-target withdrawal/replacement, persistent-target failure, revalidation failure, mixed-version, startup-frontier, concurrent DDL-vs-activation linearization, distributed Prepared/Fenced convergence, activation fail-closed/retry, shutdown-withdrawal ordering, withdrawal-failure, lag/cancel, and negative-path unit tests;
  • full tests for frontend, CN service, query service client, cluster service, HAKeeper/logservice metadata, and transaction client owners/consumers;
  • focused race tests with -count=20 and full package race tests for shared-state owners;
  • go vet for all affected production packages;
  • generated protobuf outputs regenerated from source with the repository generator;
  • a two-CN embedded topology check that creates DDL on CN-0 and immediately reads the table through CN-1.

@ck89119 ck89119 changed the title fix: wait for DDL visibility on all working CNs fix: fence DDL visibility across CN admission Aug 27, 2026
@matrix-meow matrix-meow added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/M Denotes a PR that changes [100,499] lines labels Aug 27, 2026

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

Re-reviewed the full diff and all prior threads at exact head 6b1e497. The previous stale-heartbeat ordering and pre-start ingress issues are closed: heartbeat snapshot/send is serialized with activation, and activation restores the captured ingress state. Two blocking issues remain: failed live activation cannot be retried once the target is withdrawn from admission, and this branch collides with the MORPC v35 already on current main. Exact-head package tests, focused race tests, go vet, and diff checks passed; the withdrawn-target counterexample fails as described inline.

Comment thread pkg/sql/plan/function/ctl/cmd_rpc_version.go Outdated
Comment thread pkg/defines/const.go Outdated

@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 of exact head 8dc075e. I read all reviews, inline comments, author replies, and resolved/unresolved thread state; compared the increment since 6b1e497; and rechecked the complete DDL visibility protocol, startup/admission/heartbeat/shutdown lifecycle, DDLCommitGate, fan-out, protobuf, and protocol-version diff. Both remaining blockers are materially closed. Activation recovery now authoritatively refreshes and resolves the raw CN inventory, so an ingress/admission-hidden or draining pending CN stays reachable; the request carries the selected generation/address and the receiver rejects a stale replacement before touching activation state. Current main’s v35 remains the scaled-variance contract, while this feature consistently uses v36 for activation, sender gating, SyncCommitV2, latest version, protobuf, and tests. Startup readiness publication is also serialized and directly heartbeated while holding the activation owner lock, avoiding stale publication and self-deadlock. Q1-Q3 audit found bounded target/result state, context-bounded refresh/RPC/wait paths, release of all responses, and fail-closed DDL/ingress ownership across retry and shutdown. Exact-head full tests for all eight owning packages pass; focused activation/recovery races pass five runs; go vet and diff checks pass. Two exact-head CI jobs are still queued/running, with no failure reported. No blocking correctness, rolling-upgrade, lifecycle, concurrency, or performance issue remains.

@XuPeng-SH XuPeng-SH 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-reviewed exact head 8dc075e across the complete 32-file change map, prior review threads, protocol-version wiring, DDL commit fan-out, CN startup/admission/heartbeat/shutdown, activation recovery, generated protobuf contracts, and Q1-Q3 lifecycle paths.

One blocking rolling-upgrade correctness gap remains; details are inline. Production runtime initialization assigns MORPCLatestVersion to every new process, which is now v36. Consequently a normally upgraded CN never executes the distributed pre-v36-to-v36 producer cut: startup marks the local activation prepared/fenced, and a later v36 activation command returns through the already-current fast path. A still-public pre-v36 CN is not barrier-ready and is excluded from startup frontier collection, so the upgraded CN can open ingress at an older catalog frontier and reproduce the linked missing-table failure.

I confirmed this with an exact-head deterministic counterexample: the pre-v36 public peer held DDL frontier 300 while the new default-v36 CN held 100; prepareDDLVisibilityBarrier succeeded after waiting only for 100. The temporary review test was removed after execution.

Other evidence is green: focused activation, fail-closed recovery, heartbeat ordering, shutdown, DDL fan-out, method-gate, and commit-gate tests passed; focused shared-state race tests passed three runs; diff checks passed; exact-head CI is green; latest-main has no overlap in the touched dependency closure. The blocker is a missing production rollout entry condition, not an internal race in the tested activation implementation.

Comment thread pkg/cnservice/server_ddl_visibility.go Outdated

@iamlinjunhong iamlinjunhong 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.

Reviewed exact head 8dc075e against freshly synchronized origin/main 958a4ce (merge-base b789245). Two confirmed P1 correctness blockers remain; details inline. I inspected the complete 32-file diff, the CN startup/live-activation/shutdown state machine, DDL producer gate and commit fan-out, authoritative membership/generation recovery, protocol compatibility and generated protobuf changes, Q1-Q3 resource/wait/boundedness paths, the changed tests, and all prior review comments plus author replies. All 26 current checks are completed with no failure. No P0 or additional P2/P3 finding.

Comment thread pkg/frontend/txn.go Outdated
Comment thread pkg/cnservice/server_ddl_visibility.go Outdated

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

Blocking rolling-upgrade correctness gap remains on exact head 9a7eebd: the code still conflates a local startup phase with participation in the current distributed activation cut. prepareDDLVisibilityBarrier marks every default-v36 startup Prepared and Fenced while ActivationComplete is false (server_ddl_visibility.go:47-52), and Start then enables public DDL and publishes ingress=true (server.go:469-486; server_ddl_visibility.go:113-120). The new Complete bit only guards the local SetProtocolVersion fast path. DDL fan-out is enabled solely by runtime version (frontend/txn.go:977-982), and peer phase convergence still accepts only Version plus Prepared/Fenced (server_ddl_visibility.go:399-419); there is no activation-attempt or target-generation proof.

Consequently, if one target never receives this activation RPC, or is replaced after the control-plane snapshot, its startup-only Prepared/Fenced response satisfies every other target. Those targets can set Complete, reopen ingress, and unblock DDL while the missed target never drained and never applied the full-target frontier. Before any activation command arrives, a default-v36 CN also still opens ingress and excludes a public barrier=false legacy peer from startup/fan-out, preserving the exact unsafe window from the previous review.

I confirmed the first counterexample with a temporary exact-head regression: a peer was barrier-ready and returned the startup Version=36, Prepared=true, Fenced=true state without receiving this activation attempt; waitForDDLVisibilityActivationPhase(..., requireFenced=true) returned nil, so the fail-closed assertion failed. The temporary test was removed.

Please separate compiled capability/startup-local readiness from activated protocol state. Either keep public ingress and the v36 sender disabled until a complete-target cut succeeds, or carry an activation attempt/target-set identity plus exact generation/address through Set/GetProtocolVersion and require that exact proof before any participant reopens. Add regressions for (1) default-v36 startup with a public barrier=false peer before the control command, and (2) a dropped/stale activation RPC whose target still exposes startup phase flags; all successful participants must remain withdrawn.

Validation otherwise passed: diff check, focused tests for all touched owners, and three focused race runs for frontend/cnservice. Exact-head CI is green. Performance is not the blocker here, but steady-state DDL now pays two authoritative membership refreshes plus sequential O(CN) SyncCommitV2 RPCs, so N-CN DDL latency should be measured before merge.

@iamlinjunhong iamlinjunhong 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.

Reviewed exact head 9a7eebd against freshly synchronized origin/main 9745d3c (merge-base 52336df). One confirmed P1 rolling-upgrade correctness blocker remains; details inline. I inspected the complete 32-file diff, default protocol initialization, CN startup/live-activation/heartbeat/admission/shutdown state machine, DDL producer gate and commit fan-out, raw membership/generation recovery, protobuf compatibility, changed tests, and all prior reviews, threads, conversation, and author replies. The author replies to my previous two P1 findings are materially addressed: live client-owned background DDL now participates after public-listener enablement, and activation withdraws ingress before waiting for admitted DDL to drain. Q1-Q3 found no additional confirmed leak, hang, or unbounded-growth issue. All 26 current checks are terminal with no failure or pending result (12 successful, 14 skipped/neutral). The isolated worktree lacks the required cgo/libmo.dylib, so local CGo-transitive focused/race tests could not start; package selection and diff checks passed.

Comment thread pkg/cnservice/server_ddl_visibility.go Outdated

@XuPeng-SH XuPeng-SH 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 of exact head e426854 found one remaining P1 restart/rolling-upgrade correctness hole.

The new compiled-capability separation fixes fresh first-time upgrade startup, but it loses the deployment activation state across an ordinary CN restart. Reproduction by state transition:

  1. CN A and CN B have completed the distributed v37 cut. Both accept DDL only with the cross-CN SyncCommitV2 fence.
  2. CN B restarts. MORPCLatestVersion initializes its runtime to v37, while ddlVisibilityActivationComplete is process-local and therefore false.
  3. prepareDDLVisibilityBarrier unconditionally downgrades B to v36 at server_ddl_visibility.go:47-52. prepareDDLVisibilityBarrierLocked then treats the feature as unsupported, publishes barrier-ready without the startup frontier protocol, and publishDDLVisibilityIngressAfterStart opens public ingress because ddlVisibilityBarrierSupported is false.
  4. Before the control plane replays activation, a client DDL on B enters and commits. syncDDLCommitToBarrierReadyCNs returns immediately at frontend/txn.go:978-981 because B reports v36. CN A can again serve a fresh proxy connection before applying that DDL, reproducing #27743 after the feature had already been activated cluster-wide.

This is not a narrow timing curiosity: CN restart is a normal unhappy path, and the code comment says reactivation is required while the implementation leaves the legacy producer public during that reactivation window. Local in-memory ActivationComplete cannot distinguish first deployment from restart after a completed cut. Please source the activated/deployed protocol or activation epoch from authoritative persistent control-plane state, and keep a restarting CN fail-closed until it has rejoined that exact cut. Add a deterministic regression for completed v37 deployment -> one CN restart -> DDL attempted before replayed activation; the gate must reject/block it, then reopen only after the full target-set fence.

I rechecked the remaining state machine as well: generation/address revalidation, heartbeat serialization, failure-withdrawn retry, cancellation/timeouts, response release, bounded target maps, and shutdown ordering have no additional confirmed blocker. Steady-state DDL still performs two authoritative refreshes and sequential O(CN) RPCs, so please attach an N-CN latency measurement, but that is secondary to the restart correctness issue above.

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

Additional cross-PR compatibility blocker found while reviewing the live protocol queue at exact head e42685498960a34a4f4a2fb721414cecad349517:

MORPCVersion37 is concurrently used by #27695 for mo_current_roles, by #27553 for the MongoDB explicit-query payload, and by this PR for the cross-CN DDL fence. These are three independent receiver capabilities and cannot share one oldest-live gate. A v37 binary containing only the first merged capability would make another features v37 readiness check pass even though its receiver semantics are absent.

This is independent of the restart-before-control-replay P1 in my previous review. After merge order is established, rebase the first landed v37 owner and allocate the next free version for this capability, updating the feature declaration, runtime gate, tests, and rolling-upgrade evidence together. Do not resolve the textual conflict by retaining the shared value.

@iamlinjunhong iamlinjunhong 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.

Reviewed exact head e426854 against freshly synchronized origin/main 88be4ef (merge-base 1a4f656). One confirmed P1 restart/rolling-upgrade correctness blocker remains; details inline. I inspected the complete 33-file diff, runtime protocol initialization, CN startup/live-activation/heartbeat/admission/shutdown state machine, DDL producer gate and commit fan-out, raw membership/generation recovery, ctl dispatch, protobuf compatibility, changed tests, and all prior reviews, threads, conversation, and author replies. The author's reply to my previous P1 is material for fresh first-upgrade startup: startup-only phase flags no longer prove the distributed cut. However, the new downgrade path cannot distinguish that first deployment from an ordinary restart after the cut has completed. Q1-Q3 found no additional confirmed leak, hang, or unbounded-growth issue. All 27 current checks are terminal with no failure (13 successful, 12 skipped, 2 neutral). No P0, P2, or P3 finding.

Comment thread pkg/cnservice/server_ddl_visibility.go Outdated

@XuPeng-SH XuPeng-SH 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 review completed on exact head b4bf842. Two confirmed P1 correctness gaps remain. They share one root: the distributed activation target set is neither authoritative nor linearized with CN membership and the durable cluster epoch.

  1. An omitted public legacy CN is accepted as a complete cut. The ctl path validates only target syntax, validateDDLVisibilityActivationTargets only requires the local CN, and waitForDDLVisibilityActivationPhase explicitly ignores every DDLVisibilityBarrierReady=false CN. Such a pre-v38 CN can still have ViewMetadataIngressReady=true and commit DDL, while syncStartupDDLVisibilityFrontier also excludes it. I confirmed this with an exact-head deterministic counterexample: self was at frontier 100, an omitted public legacy peer was at 300, and the supplied target list contained only self. setProtocolVersion returned nil, persisted v38, marked activation complete, and reopened ingress after waiting only for 100. This recreates the missing-catalog failure the PR is intended to prevent.

  2. Markerless startup can miss a cluster epoch committed during Start. prepareDDLVisibilityBarrier reads DDLVisibilityDeployedProtocol once before publishing barrier readiness. A valid ordering is: the old activation finishes its final phase scan; a joining CN reads epoch 0; the joining CN publishes barrier readiness; an old participant commits and heartbeats epoch 38; then the joining CN reaches publishDDLVisibilityIngressAfterStart with runtime v37 and never revalidates the now-committed epoch. It opens public ingress and public background DDL as a legacy producer after the cluster cut. An exact-head deterministic regression that advanced the fake cluster epoch from 0 to 38 between prepare and ingress publication failed because both ingress and the public DDL gate opened.

Please establish an authoritative generation/address activation set and an admission or epoch handshake that covers both public legacy producers and CNs arriving across the final commit boundary. A markerless CN must remain fail-closed until it has proved participation in the current cut. Add both counterexamples as regressions; merely re-reading at another unversioned point does not close the ordering race.

The previous restart persistence, provisional marker, HAKeeper capability, withdrawal, drain, response ownership, timeout, and recovery-identity fixes were also rechecked and their focused exact-head tests pass. All current CI is terminal green, but it does not cover these two state transitions. Temporary review tests were removed and the isolated worktree is clean.

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

Labels

kind/bug Something isn't working size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: New proxy-routed CN can miss a committed CREATE TABLE

5 participants