Repository navigation
fix(frontend): publish prepared result metadata after schema retry - #29775
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
XuPeng-SH
left a comment
There was a problem hiding this comment.
The retry/schema publication path is otherwise well scoped, and the current CI is green. I found one saved-result lifecycle gap that should be closed before approval.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed the complete range 72d4a49db7d750520e1a1654cb6091fc7169882b...6efb5fdfdaad60c2f458963ab143ec5ae5ec812f, all 15 changed files, the existing owners/consumers and the previous review thread. Approve: the saved-result failure/reuse blocker is fixed, and no additional blocking finding remains.
My previous saved-result lifecycle finding is closed by code and observed behavior, not just the resolved-thread flag. The ordinary SELECT failure guard clears the session's block index, saved counters and saved plan on Run, cancellation, metadata or finalization failure. Successful finalization still uses the existing saveMeta reset owner; cursor staging retains its separate Abort owner. The public regression actually persists a batch, fails afterward, drains the error, checks the same connection ID, then saves and replays the next BIGINT result without stale block references.
The responsibility split is appropriate. Frontend owns metadata publication; Compile remains the authority for retry generations and frozen-schema compatibility. The statement-owned sync.Once publisher ensures one header precedes any positive output, including concurrent callbacks; it is not another retry state machine. Empty results publish only after the successful generation is known. Rebuilt binary executions discard stale column packets. The existing compiler guard still rejects incompatible metadata after publication, and cursor decoding retains its required early freeze and post-transaction response boundary. Generation entry/Close clear the new state. Cached-plan admission remains one production path after extraction; SELECT and PERFORM share the existing saved-result finalizer rather than adding another storage path.
Scoped lifecycle audit:
| Dimension | Closure checked |
|---|---|
| Ownership | Statement owns the publisher; successful finalization or failure guard clears saved session state; cursor staging keeps its own terminal owner. |
| Waits | Concurrent publishers wait on the existing header write, not another producer. runOnce drains submitted scope results; retry quiesces the failed generation before replacing its plan. |
| Growth | Fixed publisher state per statement, cleared on reuse/close; no new cache, queue or result materialization. Existing saved-result size/storage policy and cursor limits remain in place. |
Independent validation on this commit, Go 1.27.1 / darwin-arm64, with matching native inputs:
- Focused metadata ordering/final-generation/error/cancellation/finalization/reuse and saved-state tests PASS normally and under race.
TestSelectResultMetadataConcurrentCallbacksadditionally passed 100 exact race repetitions after its measured duration was below clock resolution. TestPreparedSchemaCommitBeforeMetadata: all nine real MySQL cases PASS normally (8.85s test time) and under race (9.64s measured test time), followed by three exact in-process race repetitions under the 30s adaptive budget.- Independent adjacent-boundary probe using a Go overlay: the same deterministic DDL commit barrier now both widens
bto BIGINT and adds a third projected BIGINT column. All four text/binary x empty/nonempty cases PASS with exact three-column metadata and values, including(2, 2147483648, 7). This challenges column-count changes and stale binary packets in addition to the author's single-type-change cases; no probe code is committed. - Existing cursor metadata/packet/error/commit-failure controls, PERFORM controls, frozen-schema rejection/equivalent-plan acceptance and SELECT INTO retry arity checks PASS.
- Reused exact-head CI owning-suite/race/static evidence from run 37956686401, including the explicitly reported public test pass. Required CI, coverage, Proxy and pessimistic BVT are green.
Implementation is +114/-33, tests +593/-12, tracked documentation +0/-0. The two-CN pessimistic fixture has a concrete role in the cross-CN DDL race and is shared across its cases; the wait point observes actual binding, and the client assertions cover types, values, errors and same-connection recovery. The removed compile-only response stub is replaced with the real admission/Compile phase; it does not remove the cache-generation oracle.
Performance assessment: this adds one statement-owned publisher/initializer and a sync.Once fast-path check per positive batch, while moving the existing metadata derivation rather than repeating it. There is no new per-row conversion, registry scan, RPC, retry loop or result buffering. First metadata is intentionally delayed until the executing schema is known. This is a justified bounded correctness cost; I am not claiming a measured TP throughput improvement or zero overhead.
Fresh main was 4942f889ef08ae26b2ebeb02a1eda11634eb83fd; its changes after the merge-base do not touch frontend, compile or sqlintegration. This review approves this independent kernel fix only; it does not approve or close the downstream Sirius feature/release obligations.
Merge Queue Status
This pull request spent 2 minutes 47 seconds in the queue, with no time running CI. ReasonPull request #29775 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Waiting for any of
All conditions
Tick the box to put this pull request back in the merge queue (same as
|
) Opt-in embedded MO-reader queries now lower MO exact Decimal64/128/256 values and descriptors to Sirius GPU execution. One immutable capability-scoped profile owns admission, physical types/literals, bound scalar/aggregate overloads, input publication, result reconstruction and public numeric error classes. Terminal evidence records verified descriptors and a schema digest without logging headings or values. This implements approved PR C from #29690 and the versioned [design/validation map](https://github.com/matrixorigin/matrixone/blob/a4e34ef05de6a6d02ceb0fa81219cacd452ae6c8/docs/design/sirius-mo-exact-lowering.md). Physical width is preserved at narrow declared precision; MO-bound division metadata and 32-byte Decimal256 coefficients use the existing credit-before-copy and borrowed-result contracts. Embedded fetch defaults and DATE extraction use supported native wire forms. All prerequisites are merged and inherited: - Sirius [#29](matrixorigin/sirius#29): `908ffc75a58b3be496b2172795e416326416e7ec`, with merged importer #5 at `99c7ca3b6f8f3159239e119ed2982d42f98c4690`. - Kernel [#29775](#29775): `af3f7232a31879a2d6ed590785c8e9a560361c3f`. Prepared SELECT metadata comes from the executed generation; incomplete saved SELECTs clear session state before reuse. - Main [#29794](#29794) supplies the existing MORPC shutdown-test race correction. Main now also includes the histogram-export synchronization fix; C uses main's bounded two-send notification wait unchanged. Final release/native integration at executable C head `ad61b5b5d3`, on merged-kernel main `af3f7232a3`, passes: - Default, Sirius-only and combined Sirius/cuVS release builds. The clean merged SDK, all 73 fingerprints, compiler/header/proto identity and runtime package/provenance guards pass. - All 22 native preparations without reader/GPU-task admission. The complete public MySQL numeric fixture passes exact values, metadata/NULLs, arithmetic/aggregates/joins/sort, operation-owned errors, masked errors, healthy reuse and prepared division increments 0/4/10/30/4. Native MO supplies the independent oracle. - All 31 public terminal events show capability 31, `mo-exact-decimal-v1`, completed GPU tasks, no fallback, healthy cleanup and zero retained input/result credit. - Four C-owning packages plus frontend pass default normal/race checks; native-tag owning checks and the complete combined bridge suite pass. cuVS-before/Sirius/cuVS-after passes in one process. - All nine public prepared-metadata/saved-result cases pass normal mode and three race repetitions, including a real persisted-batch failure followed by successful save/replay on the same physical connection. - Native data/cancellation passes 68 race repetitions in one process (measured 0.44s; 30s budget). The exact MORPC shutdown test passes 100 race repetitions. Metrics focused/owning normal/race and 100 focused race repetitions pass. The delivered head is `b34111b4c9e94cda034d199a2c362072fe98d808`, normally merged with authoritative main `e5dd4724f782067e1c76c38c9d2f55cebceda4a9`. Review fixes are committed in `af4f342066`: typed DATE lowering preserves persisted MO zero dates; exact CASE retains the MO-bound descriptor around IfThen; ordinary embedded integer add/subtract/multiply declines before readers because the pinned consumer does not implement MO checked overflow. Flight encoding/admission, reader bytes, native ABI and the merged Sirius pin are unchanged. The installed and loaded host drivers now both report NVIDIA 615.78.08; final GPU checks run directly with the installed driver. The bounded local native fixture uses 256 MiB GPU and 2 GiB host capacity. An initial 512 MiB host fixture correctly rejected larger progress-window reservations; only the test fixture was corrected. Production windows, limits and assertions remain unchanged. Review-resolution validation at prior head `a4e34ef05d`: - Both incoming counterexamples have real GPU red/green proof. Zero DATE now returns 0/0/0 alongside year-0001/leap-day/year-9999/NULL controls, with SQL mode restored. Nullable division conditions with two required decimal arms execute with native-MO values and nullable DECIMAL(15,2) metadata. - The existing public fixture covers integer +/-/* overflow declines and healthy integral/decimal reuse. No new cluster, production hook, native library dependency, sleep or skip was added. - All 22 native preparations pass. Full public GPU normal mode passes (9.596s); measured race mode followed by three in-process repetitions passes (29.571s). Its 34 terminal events verify exact profile/capability31, completed GPU work, no fallback, healthy cleanup and zero retained input/result credits. - Fresh combined release/native provenance and full bridge/coexistence pass. Exporter/compiler/CN owning normal/race, focused new wire/domain regressions, config and full mandatory pre-push SCA pass with zero lint findings. Self-review has no unresolved implementation blocker. **Ready for review; merge gates remain required CI and reviewer approval.** Both incoming findings were reproduced on actual GPU/public MySQL and fixed with regressions. A separate admitted integer-overflow counterexample returned -2 instead of native MO 1690/22003; it now declines before execution. Fresh CI is pending for the delivered head, and the reviewer has been asked to reassess the fixes. Local passes do not establish required CI success. C remains opt-in. D begins after C merges and stays one complete public campaign PR for all-22 SF1/SF10, bounded comparison, lifecycle/resource/process evidence and performance gates. E/F retain their separate recovery-readiness design. References #28968 and #28966; neither issue is closed by C. Conflict resolution at `b34111b4c9` (2026-10-10): normally merged authoritative main `e5dd4724f7`. The single conflict was two equivalent asynchronous histogram-test fixes; main's stopped-timer notification barrier is retained unchanged. All 28 prior C paths were checked: only this test changed, while the DATE/CASE/integer review fixes and merged native pins remain intact. Validation at this merged head passes: affected owning packages normal/race, 100 focused metrics race repetitions, fresh native generations and combined release/package, all 22 actual preparations (1.116s), full public GPU normal/race (9.762s/11.999s), and full combined bridge/coexistence (1.108s). Both public runs' 34 terminal events verify completed GPU work, exact profile/capability31, healthy cleanup, no fallback and zero retained credits. Mandatory pre-push SCA passes with zero findings; self-review has no unresolved implementation blocker. New-head required CI and human reviewer approval remain the merge gates. --------- Co-authored-by: XuPeng-SH <xupeng3112@163.com>
This branch was successfully deployed
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
References #29757, #28968 and #28966. These feature issues remain open.
What this PR does / why we need it:
A prepared SELECT can bind an old table definition, then observe a concurrent
ALTER TABLE ... MODIFY b BIGINTcommit before its pipeline starts. Previously the frontend froze the old result schema before Run, so an otherwise valid definition retry returned20631/HY000: txn need retry in rc mode, def changed. The deterministic public reproduction also fails on mainb6fa8ccfc7with only an inert fault point added; this is the failure class observed by #29757's pessimistic Compose BVT atpessimistic_transaction/prepare.sqlrow 10.Publish SELECT metadata once from the actual execution generation, immediately before its first positive output batch or after successful completion for zero rows. Rebuilt binary executions discard stale cached column packets. Once a consumer retains or publishes metadata, the existing incompatible-schema retry guard still rejects schema drift; its flag is atomic for the output callback boundary. Cursor decoding retains its earlier freeze and its existing response-after-commit behavior.
Saved results finalize metadata after whole-query success, where write errors can propagate, rather than from a pipeline-local reset callback. PERFORM reuses the same existing finalizer. The unchanged cached-plan admission phase is extracted so its compile-only test continues exercising the real owner without relying on early result packets. The legacy protocol test producer now observes the same metadata-before-rows boundary.
Validation includes the original publication-boundary evidence and the current review fix at
6efb5fdfdaad60c2f458963ab143ec5ae5ec812f, based on authoritative main72d4a49db7d750520e1a1654cb6091fc7169882b:pkg/frontendandpkg/sql/compilesuites pass in normal and race modes using verified CGo artifacts.make config, the full mandatory pre-push SCA gate, and lifecycle self-review pass; the branch is zero commits behind freshly fetched authoritative main.This is an independent kernel prerequisite for C's CI closure. After it merges, C can inherit the merged fix; C also still requires merged Sirius #29 and its final merged-pin release/public checks. D implementation begins only after C merges.
Review follow-up (2026-10-10): SELECT now clears session saved-result state on a late Run/cancellation/metadata/finalization failure, following the existing PERFORM cleanup guard. The prior-head unit reproduction fails and the fixed error/cancellation controls pass. A public test persists a real batch, injects an error afterward, fully drains it, verifies the connection ID is unchanged, then saves and replays an exact BIGINT on that same connection. Cursor staging keeps its independent abort owner. All nine public cases and full frontend/compiler suites pass normally/under race; three in-process public race repetitions and full pre-push SCA pass. The reviewer thread is answered and resolved; fresh review/CI are requested on the new head.