Repository navigation
[4.2-dev] fix: preserve tombstones across lineage snapshot advances - #30576
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? |
|
mo-bot review · Commit: Review completed. Read the assessment. |
|
mo-bot review · Commit: "interrupted" |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 162473f0a8b495f4e7a095a1747de4f0245094ce
Assessment: approve
Reviewed the complete eight-file delta and affected callers, transaction ownership, rollback, cancellation, compatibility and tests. The shared helper correctly delegates advancement and tombstone transfer to the existing workspace owner; removing timestamp-only rewind preserves their paired state. No actionable defects remain. Required transfer work is confined to lineage boundaries, without new per-row SELECT/DML work or persistent state.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
Validation:
-
Verified the frozen head, merge base, unchanged source, formatting and native-build provenance. Commands, exits and retained logs are reconciled in /home/xupeng/.local/state/mo-bot/jobs/d9ae4559126456c104dad5ec/attempt-1/validation-audit.json.
-
All ten focused tests passed normally and under the race detector. Another 434 exact-test race repetitions passed, covering catalog replacement, both tombstone modes, repeated advancement, commit and rollback.
-
Full helper and frontend packages passed normally and under the race detector; the full engine test package passed under the race detector.
-
Full compile-package runs failed three Parquet fan-out tests. The same three failures reproduced with the complete delta reverted through a verified merge-base overlay; they are not introduced by this PR.
-
An isolated timestamp-only negative control reproduced duplicate catalog IDs. The candidate regression preserves the sole replacement ID and successfully executes the subsequent catalog deletion.
Limitations:
-
Frozen CI evidence was pending and local_validation was empty; author execution claims were not counted as independently verified checks.
-
The linked issue body was absent. Its concrete failure contract was assessed from the PR description, source and reproduced regression.
-
No independent MySQL TCP/BVT run, scale benchmark or partial-transfer I/O fault injection was performed. Performance assessment is based on the affected call paths and existing transfer ownership.
…ck (#30580) ## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Related to #29710. Backports #29754 (`aee2a0b3764781a8ca908618548dfef75e336263`) to `4.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. - Save the existing workspace transfer frontier and pending debt alongside each statement's write offset; restore both on rollback without rewinding the operator snapshot. - Include intermediate merged objects when recovering in-memory RowIDs, which earlier transfers update in place. Preserve the default snapshot and persisted-source projections. - Prepare targets and prefetch only after finding a matching workspace delete. - Retain `4.2-dev`'s `ApplyFlush` tracing, 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): - Full `disttae` and `logtailreplay` packages pass normally and with race. - All seven snapshot-harness consumer tests pass normally and with race: memory/persisted/mixed × three rollback boundaries across real TN flush and a second merge; immediate COMMIT; partial persisted I/O failure and cancellation after memory transfer; durable fresh-transaction assertions. - Prefetch admission checks no-demand lookup/prefetch avoidance, ten distinct demanded prefetch completions, unchanged RowID on metadata failure, and scratch cleanup. - Counterexample: reverting only the intermediate-memory-source projection makes the new real-engine transferring-statement rollback test fail with ten surviving rows; the candidate passes. The persisted `expect 20, got 10` baseline witness was established separately before this backport. - External MySQL probe against the candidate embedded cluster passes retained DELETE + later statement error + continuation/COMMIT, error + immediate COMMIT, and full rollback with fresh-connection results. This checks public rollback behavior, not deterministic public SQL reproduction of the forced merge interval. - Separate bounded race repetitions pass: admission 100, statement rollback 4, rollback across merge 3, partial I/O recovery 15. Counts use a 30-second per-test budget and measured one-run race durations. - `go vet`, configured molint, configured golangci-lint against the exact branch base, formatting, and diff integrity pass. Reproduce the repository regression checks: ```sh .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/test ``` Both selections also pass with `-race -count=1 -timeout=600s`. No workload throughput or physical I/O improvement is claimed.
Backport the applicable workspace-owned lineage snapshot protocol from 7b3d0c3 (matrixorigin#29539), and remove ALTER's timestamp-only rewind. Cover the real catalog DROP/rename consumer and both tombstone modes, including statement rollback followed by immediate COMMIT. Fixes matrixorigin#30541.
162473f to
f752344
Compare
|
mo-bot review · Commit: "interrupted" |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: f7523448ec8d3ad0508ebc94f141f703421fd02b
Assessment: approve
Reviewed the complete eight-file delta and affected callers, rollback, cancellation, compatibility and tests. The shared helper correctly delegates snapshot advancement and tombstone transfer to the workspace owner. Removing timestamp-only rewind preserves their paired state without introducing duplicate recovery logic. No actionable defects remain. Additional transfer work is confined to lineage boundaries and scales with workspace deletes and relocated objects; ordinary SELECT/DML paths are unchanged.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
Validation:
-
Verified the frozen head, unchanged source, complete patch, formatting and compatible native-library provenance. Commands, source fingerprints, exits and log hashes are reconciled in /home/xupeng/.local/state/mo-bot/jobs/7126f8fd35284fee9bcd8380/attempt-1/validation-audit.json.
-
All 12 focused tests passed normally and under the race detector, including catalog replacement, both tombstone modes, repeated advancement, statement rollback, immediate COMMIT and partial-I/O recovery.
-
All 425 adaptive, individually selected race-test repetitions passed. Selection, durations and repetition counts are retained in race-stress-plan.json.
-
Full databranchutils, frontend and engine test packages passed normally and under the race detector. Both full compile-package runs failed exactly three Parquet tests; identical failures reproduced against the frozen merge base through a verified complete-delta overlay.
-
An isolated timestamp-only negative control reproduced duplicate catalog IDs. The candidate preserves the sole replacement ID and successfully executes the subsequent catalog deletion.
-
Go vet passed for all four affected packages. Final evidence reconciliation passed with receipt 1792400406313975560-4.json.
Limitations:
-
Frozen CI was pending and local_validation was empty. Earlier-head approval and author execution claims were not counted as current-head test results.
-
The linked issue body was unavailable; its failure contract was assessed from the supplied PR description, source and reproduced catalog regression.
-
No independent MySQL TCP/BVT run or scale benchmark was performed. Performance conclusions are bounded source analysis, not measured workload throughput.
…nce (#30602) ## What type of PR is this? - [ ] API-change - [ ] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Related to #30541. Production fixes are already present on main through #29539 and #29754; the release-branch correction is #30576. ## What this PR does / why we need it: Add the missing deterministic catalog regression for COPY ALTER's DROP/rename window. Existing main tests cover transferred data deletes and replacement keys, but do not check the same-name catalog replacement and the next real `txnDatabase.Delete` singleton guard. The test commits an original table, creates its transaction-local replacement, drops and renames through the real engine APIs, synchronously flushes `mo_catalog.mo_tables` on TN, and advances through the shared lineage helper. It then requires exactly the replacement table ID and successfully executes the next real Delete. There is no statement boundary between flush and verification: a normal boundary could repair the stale tombstone and hide the original defect. Setup reuses EnginePack with long checkpoint scanning, one PK column, two table identities, and no user data rows. Pack, setup/RC transactions and query results register cleanup immediately. No production hook, state, guard relaxation or runtime change is introduced. Validation on Go 1.27.1 / Darwin arm64 CPU, with matching native provenance: - New focused regression passes normally and with race; the normal test body takes about 0.75 s. - Full `./pkg/vm/engine/test` passes normally (162.995 s) and with race (226.079 s): 167 top-level passes in each mode; two existing tests remain skipped and are not claimed as executed. - An external Go overlay changes only the shared helper back to raw operator advancement. Its witness confirms both original and replacement IDs, then catches the real `delete table failed: query failed` panic from `db.Delete`. The mutation is not delivered. - The exact new test also passes 37 repetitions under race in one process (31.277 s total); count selected from a 30-second budget and a measured 0.79 s single-test race duration. - After main advanced to `d0e7f9a552f`, the exact regression also passes normally and with race using an external overlay of the complete upstream delta. Full-package results above apply to the frozen `8732827` base; the overlay check is focused compatibility evidence. - `go vet`, repository-pinned molint and configured incremental golangci-lint against `873282742714436bff407dfcdbbe879147162bd3` pass; formatting and diff integrity pass. Commands: ```sh .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=900s ./pkg/vm/engine/test .agents/skills/mo-dev/scripts/mo-cgo-test -race -count=1 -timeout=1200s ./pkg/vm/engine/test ``` This is regression coverage for the existing main fix, not a new production fix. It does not claim deterministic public SQL scheduling of a mid-ALTER flush, execution of the original Matrixflow upgrade workload, crash recovery, or a workload performance improvement. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
What type of PR is this?
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_tablessingleton-result guard.Use the existing
Workspace.AdvanceSnapshotowner 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 originaldelete table failed: query failedpanic.Validation before rebase, at original head
162473f0a8b, Go 1.26.4 / Darwin arm64 CPU with branch-built native libraries:pkg/frontend/databranchutils,pkg/sql/compileandpkg/frontendUTs pass normally and with-race -count=1.-race -count=1; bounded exact-test race repetitions cover fixture reset/reuse.go vetand MatrixOne analyzer complete; the analyzer reports no diagnostics in changed files. Formatting andgit diff --checkpass.gpt-6.1-sol, reasoningxhigh.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.