Repository navigation
[4.2-dev] fix(disttae): restore RC statement transfer state on rollback - #30580
Conversation
…-dev Backport matrixorigin#29754 (aee2a0b). Save transfer frontier and pending debt at statement entry, recover retained in-memory RowIDs through intermediate objects, and defer target preparation until a workspace delete requires transfer. Keep the branch trace and execution APIs. Add real-engine rollback/merge, immediate commit, partial I/O recovery, and prefetch admission coverage. Related to matrixorigin#29710.
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? |
|
mo-bot review · Commit: Review completed. Read the assessment. |
|
mo-bot review · Commit: "interrupted" |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review — no material blocker found on ddb684decf0c555fd86745e248c119f15766ca91.
This is a COMMENT review because the authenticated reviewer is also the PR author. It records the review conclusion, not a formal independent approval. No subagents or source changes were used in this review.
Scope: all eight changed files against base/merge-base 35273bcd6a5cfa9468bcf7ca4031fe17560b5ab1; freshly verified remote 4.2-dev has the same base. Current files match the tested candidate manifest and tree 5f370a53720c5d17ffbf47823ae62b60e2a96808.
Design and ownership
- The existing workspace transaction remains the owner. Statement-entry transfer frontier/debt replaces the old timestamp history and stays paired with write offsets. Rollback restores the discarded statement's entry state; it does not rewind SnapshotTS or replay an earlier retained successful transfer.
- Restored debt forces recovery before immediate COMMIT even when the transaction is eligible for age-based transfer skipping. Old field, diagnostics and terminal cleanup references have been replaced consistently.
- Intermediate deleted objects are admitted only for in-memory RowID recovery, since retained batches are mutated in place. Default PK and persisted-source projections remain unchanged and share the existing traversal. Strict cardinality checks, branch tracing and batching boundaries remain intact.
- No duplicate state machine, storage path, worker, lock, cache, configuration or format was added.
Independent QA and unhappy-path closure
Traced retained successful replacements followed by a later rollback; rollback of the transferring statement; repeated rollback/retry; first-statement restoration; immediate COMMIT; partial memory success before persisted I/O failure/cancellation; full transaction rollback; and terminal cleanup. Existing ownership and synchronization remain coherent.
Challenged whether rewritten memory RowIDs could spill into persisted tombstones before recovery: non-commit workspace dumping does not dump DELETE batches, and COMMIT transfers before its delete dump, so that escape is not reachable on this branch. Error recovery continues to require the existing statement/full rollback protocol; this does not introduce direct retry without rollback as a supported contract.
Performance and engineering quality
Production delta is four files, +58/-37; tests are four files, +374/-6; repository documentation/generated delta is zero. Each increment was reviewed separately. History remains O(statement count), with rollback truncation and terminal release. The expanded source projection can add workspace scans, which are necessary for retained intermediate RowIDs. Target materialization/prefetch occurs only after matching workspace demand. No end-to-end throughput, latency or physical I/O improvement is claimed.
Tests reuse the existing 20-row real-engine fixture. The nine memory/persisted/mixed × rollback-boundary cells, immediate-COMMIT cells, partial-I/O/cancellation controls, object-window assertions and prefetch admission checks prove distinct contracts. Fresh-transaction durable reads and exact nonempty rollback PK sets strengthen the oracles; no scheduler sleeps or production hooks were added.
Evidence inspected and reused
- Exact-candidate full disttae/logtailreplay normal and race runs: PASS. One existing fixture-dependent real-object test is skipped and is not claimed as executed.
- All seven targeted engine consumers, 33 named tests/subtests: normal PASS (24.040 s), race PASS (27.737 s).
- Separate bounded race repetitions: admission 100, statement rollback 4, second merge 3, partial-I/O recovery 15 — PASS.
- External MySQL probe against the candidate embedded cluster: retained DELETE + later error + continuation/COMMIT, error + immediate COMMIT, and full rollback pass fresh-connection result checks.
- Isolated old-memory-projection mutation makes the real-engine transferring-statement rollback test fail with expected zero, got ten rows. The candidate passes. The persisted expect-20/got-10 baseline witness is recorded separately; historical main validation is not substituted for candidate evidence.
- Vet, configured molint, configured incremental golangci-lint, formatting and diff integrity: PASS. This review verified source identity and terminal artifacts instead of rerunning equivalent checks.
Comments and limits
At review capture there were no actionable inline threads or human change requests on this PR; Qodo's paused notice and two in-progress bot reviews are not resolved code findings. Future bot findings must be assessed when available. Remote CI was still running, so this is not a claim that all CI has passed.
The #29754 backport contract is satisfied. The raw lineage snapshot caller correction in #30576/#29539 remains separate; this PR alone does not establish the original #30541 caller behavior. Deterministic public SQL reproduction of the forced merge interval, crash recovery and workload-scale performance are outside the evidence claimed here.
Conclusion: no verified correctness, lifecycle, compatibility, performance or unjustified-complexity blocker found in this commit.
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: ddb684decf0c555fd86745e248c119f15766ca91
Assessment: approve
Reviewed all eight changed files and their statement, retry, commit, transfer and cleanup callers. The design restores transfer state alongside the retained write boundary without rewinding the operator snapshot. It reuses the existing transaction owner, preserves persisted-source selection and strict cardinality checks, and correctly includes intermediate objects for rewritten in-memory RowIDs. Demand-gated target preparation avoids unnecessary lookup and prefetch work. No actionable correctness, lifecycle, compatibility or design defects remain.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
Validation:
-
Isolation preflight passed. Built native prerequisites through the repository Makefiles; verified 173 native input files against the frozen source. Source remained unchanged.
-
Full disttae and logtailreplay packages passed normally and under race detection.
-
All seven engine regression groups passed normally and under race detection, covering memory/persisted/mixed deletes, rollback across successive merges, immediate COMMIT, partial I/O failure, cancellation and fresh-transaction results.
-
Bounded race repetitions passed: both new unit tests 100 times each; statement rollback 4 times; merge recovery 3 times; partial I/O recovery 15 times.
-
Diagnostic control restoring only the old in-memory projection failed with 10 surviving rows; the unchanged candidate passed the same regression.
-
go vet passed for all three affected packages; diff integrity passed. Commands, source fingerprints, exit receipts and retained logs reconcile in /home/xupeng/.local/state/mo-bot/jobs/e7eb8c47214b2ff47aee1245/attempt-1/validation-audit.json. Final audit receipt: checks/1792395947315181066-3.json.
Limitations:
-
Supplied substantive CI was pending and local_validation was empty. Author-reported runs were not counted as independently verified execution.
-
Local execution used Linux amd64, Go 1.26.4 and CPU CGo. Public MySQL and Darwin validation were not independently repeated; linked-issue text was unavailable beyond the supplied PR description.
-
No workload throughput benchmark was run. Expected lookup/prefetch savings are supported by admission tests; intermediate-source map storage scales with object transitions in the transfer window.
…30576) ## What type of PR is this? - [ ] API-change - [x] BUG - [x] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #30541. Backports the applicable correction from #29539 (main commit `7b3d0c3bcfa`); related to #29540. ## What this PR does / why we need it: Repeated COPY ALTER in an RC transaction can leave a catalog DELETE pointing at a row's old Rowid after TN flush/merge relocates it. The lineage refresh previously updated only the transaction operator's snapshot, exposing both the relocated committed row and the transaction's replacement. The next internal DROP then panics on the `mo_tables` singleton-result guard. Use the existing `Workspace.AdvanceSnapshot` owner through one shared physical-lineage timestamp helper, covering COPY ALTER, internal PITR, and frontend CLONE/SNAPSHOT callers. Remove ALTER's timestamp-only rewind: once tombstones transfer, restoring just the timestamp cannot restore the paired workspace state. Preserve the physical history boundary (`updated physical - 1ns`), tenant-less frontend behavior and publication locks. This branch has no TRUNCATE lineage refresh to backport. Reuse the existing forced-flush harness for both in-memory and persisted tombstones, checking exact replacement keys, repeated refresh, commit/full rollback, and statement rollback followed by immediate COMMIT. Add a real catalog DROP/rename/flush regression that checks the replacement table ID and successfully executes the next `db.Delete`, with no production test hook or relaxed catalog guard. An isolated operator-only mutation deterministically reproduces two catalog rows and the original `delete table failed: query failed` panic. Validation before rebase, at original head `162473f0a8b`, Go 1.26.4 / Darwin arm64 CPU with branch-built native libraries: - Full `pkg/frontend/databranchutils`, `pkg/sql/compile` and `pkg/frontend` UTs pass normally and with `-race -count=1`. - All seven tests in the affected engine snapshot-advance/catalog fixture closure pass normally and with `-race -count=1`; bounded exact-test race repetitions cover fixture reset/reuse. - Real MySQL TCP probe on the existing embedded cluster passes prior workspace INSERT/UPDATE, repeated transactional ALTER, COMMIT/ROLLBACK, historical reads, CLONE/PITR, exact catalog cardinality and teardown. Existing historical-lineage BVT coverage is retained unchanged; the public probe does not claim a deterministic mid-ALTER flush. - Incremental configured golangci-lint: zero issues. `go vet` and MatrixOne analyzer complete; the analyzer reports no diagnostics in changed files. Formatting and `git diff --check` pass. - Independent solution/design and final overall review: `gpt-6.1-sol`, reasoning `xhigh`. The helper adds constant delegation work and reuses the required existing tombstone-transfer work at lineage boundaries, proportional to workspace deletes and relocated objects. No storage/wire format changes. No scale benchmark or partial-transfer I/O fault-injection claim. CI is pending. ### Rebase update Rebased onto `fe9c5ea3964` (`4.2-dev`, including #30580). New head: `f7523448ec8`. The shared snapshot test fixture conflict was resolved by retaining all seven upstream test bodies (including mixed tombstones, immediate COMMIT, repeated rollback/merge, and partial-I/O recovery) and this PR's lineage helper/replacement-key assertions. Immediate-COMMIT cases already supplied by #30580 were not duplicated. The seven other PR files are identical to the original head. Formatting, diff integrity and range-diff/content preservation checks pass. No test commands were rerun for this rebase; the local execution results above describe the original head. Fresh CI must validate the combined revision.
What type of PR is this?
Which issue(s) this PR fixes:
Related to #29710. Backports #29754 (
aee2a0b3764781a8ca908618548dfef75e336263) to4.2-dev.What this PR does / why we need it:
A later statement rollback could rewind transfer progress past replacements retained from a successful statement. Replaying that interval duplicates persisted tombstone replacements; another merge then fails the strict transfer cardinality check (
expect 20, got 10). Conversely, rolling back the transferring statement must retain recovery debt so immediate COMMIT cannot skip transfer.4.2-dev'sApplyFlushtracing, lookup path, batching, and strict cardinality guard. No format, configuration, or native changes.This is an independent backport. The lineage snapshot caller correction in #30576/#29539 remains separate.
Validation (Go 1.26.4, Darwin arm64, CPU CGo):
disttaeandlogtailreplaypackages pass normally and with race.expect 20, got 10baseline witness was established separately before this backport.go vet, configured molint, configured golangci-lint against the exact branch base, formatting, and diff integrity pass.Reproduce the repository regression checks:
.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=300s ./pkg/vm/engine/disttae ./pkg/vm/engine/disttae/logtailreplay .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=300s -run '^(Test_RCSnapshotAdvancePreservesUncommittedDeletes|Test_RCSnapshotAdvanceHarness(Rollback|Commit|SubsequentStatement|StatementRollback)|TestRCSnapshotRollbackAcrossMerge|TestTransferPartialIORecovery)$' ./pkg/vm/engine/testBoth selections also pass with
-race -count=1 -timeout=600s.No workload throughput or physical I/O improvement is claimed.