Codex/abort column - #26345
Conversation
# Conflicts: # pkg/vm/engine/tae/rpc/dump_table.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? |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The two blockers from the previous review are fixed: backup reads now combine abort and tombstone masks in original physical-row coordinates, and publication filtering preserves original offsets through compaction and sorting. Earlier blockers involving CDC propagation, cache-backed reads, aborted tombstone matching, duplicate-key retries, and mixed-version object layout remain fixed. The intervening merge only resolves the MORPC version collision by assigning abort-column support to v10; no upstream-only change is attributed to this PR. No current finding remains. git diff --check passes; focused tests, build, and vet could not complete because the checkout lacks cgo/libmo.dylib and required third-party C headers.
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
The latest commit correctly restores the leading-RowID contract for persisted change batches, and the previous abort-filtering fixes remain intact. One production-reachable mixed-version data-correctness blocker remains in the v9 compatibility path.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head 30f350d169e8173cd9ced765214627d0b4c2991d.
The latest commit correctly restores the leading-RowID contract for persisted change batches, and the earlier abort-column fixes remain intact. One production-reachable data-correctness blocker remains:
P1 — Preserve physical RowID coordinates in the pre-v10 compatibility layout (pkg/vm/engine/tae/tables/mnode.go:186)
When PersistedAObjectAbortSupported() is false, getDataWindowOnWriteSchema removes abort holes with inner.Compact(). This changes the physical offset of every later live row. The snapshot-flush path then writes that compacted batch for the same appendable object and updates its location/stats, while normal persisted reads reconstruct RowIDs from the new physical positions in buildRowidColumn. Existing tombstones still reference the original physical offsets; the merge transfer map cannot repair reads of the old aobject required by historical snapshots.
Concrete counterexample: original rows [offset 0 live, offset 1 aborted, offset 2 live+tombstoned] are flushed in v9 as physical rows [0, 1]. Original row 2 is now reconstructed as offset 1, but its tombstone remains (objectID, offset 2), so the deleted row can become visible again.
Please keep physical coordinates stable in the compatibility path, or comprehensively remap every coordinate consumer before publishing the new location. Add an end-to-end v9 regression with an interior abort hole, a tombstone for a later live row, a table-tail snapshot flush, and a historical read proving that row remains invisible.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
The v9 RowID-coordinate blocker is fixed: rollback holes now retain their physical offsets and are encoded with UncommitTS, and the new historical-snapshot regression exercises the later tombstone correctly. One independent persisted-abort visibility path remains blocking.
P1 — Filter aborted rows when backup readers use no snapshot cutoff (pkg/vm/engine/tae/blockio/read.go:375)
BlockDataReadBackup resolves and filters SEQNUM_COMMITTS/SEQNUM_ABORT only inside if !ts.IsEmpty(). Its empty-TS branch applies tombstones alone. This PR makes rollback rows persistable, so an empty cutoff must still exclude rows that can never be visible: abort=true in the v10 layout and commitTS=UncommitTS in the new v9 compatibility layout.
This is production-reachable, not a helper-only edge: SnapshotMeta.GetSnapshot calls this function with types.TS{} at snapshot.go:1087, and PITR/ISCP use specialTableInfo.processObjects, which does the same at snapshot.go:418. Those calls also request only selected user columns, so the current load does not include either special vector. A rolled-back append to these system tables can therefore be processed as live snapshot/PITR/ISCP metadata, affecting GC and checkpoint retention decisions.
I reproduced the leak on exact head 04a43fd7cae2 by extending the existing backup-read regression's persisted rows [0 live, 1 abort, 2 tombstoned, 3 live]: with a non-empty cutoff it returns [0,3], but the same call with types.TS{} returns [0,1,3]. The PR's focused blockio/memory-node/v9 end-to-end tests pass without that assertion.
Please make abort visibility unconditional while making only the ordinary commit-time cutoff optional. For selected-column reads, load the needed special columns without exposing them in the returned schema (or pass a real cutoff through all of these callers). Cover empty-cutoff reads for both v10 abort=true and v9 UncommitTS, plus at least one SnapshotMeta consumer regression.
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
The latest main merge drops upstream fixes and would regress unrelated production paths. fa10ccb has 607a926 (the current PR base) as its second parent, but its tree is byte-for-byte identical to the first parent a718fc1. As a result, the PR now carries 105 changed files and effectively reverts all four commits added on main since 8fc0655: #26641 CDC resume recovery, #26497 session-reset request fencing, #26635 ONLY_FULL_GROUP_BY enforcement, and #26700 snapshot-restore role-privilege ID rebinding. Concrete examples are the deletion of pkg/sql/plan/mysql_full_group_by.go and pkg/frontend/snapshot_catalog_restore.go. These are normal-business regressions unrelated to abort metadata. Please rebase or redo the main merge while retaining the upstream side, then verify the PR diff is limited to the rollback-hole change. The a718fc1 backup-read change itself addresses the previous empty-TS issue for both v9 UncommitTS and v10 abort-column layouts.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head fa10ccb333052e672164ef215766325a6c17f7d5.
The a718fc1a change fixes the previous empty-cutoff blocker: backup reads now filter both v10 abort=true rows and v9 UncommitTS rows for full and selected-column reads, and the new focused and SnapshotMeta regressions pass. One independent P1 blocker remains in the final merge commit.
P1 — Redo the main merge without discarding the upstream tree
fa10ccb3 records current base 607a9263 as its second parent, but its tree c0dfd7d4 is byte-for-byte identical to first parent a718fc1a. Therefore the merge accepted none of the upstream side. Relative to the current base, this PR now changes 105 files (+3358/-4680) and reverts all four commits added on main since common ancestor 8fc0655a: #26700 snapshot-restore role privilege ID rebinding, #26635 ONLY_FULL_GROUP_BY enforcement, #26497 session-reset request fencing, and #26641 CDC resume recovery.
Concrete evidence includes deleting pkg/frontend/snapshot_catalog_restore.go, pkg/sql/plan/mysql_full_group_by.go, pkg/tests/issues/issue_26640_test.go, and their regression coverage. The full PR diff --check also fails on old cte.sql whitespace restored by the same merge error. Merging this head would regress unrelated production behavior even though branch CI is green because the corresponding tests were reverted too.
Please rebase or recreate the merge while retaining the upstream side, then verify that the PR diff is limited to the rollback-hole/abort-column feature.
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
Deep review of exact head 3e95f76d23cb7dbbf2cc3fa8e084748742f46cc3. The previous bad-merge blocker is fixed: the PR is again limited to the rollback-hole feature and retains the upstream fixes. Earlier blockers around CDC, cache reads, abort-aware tombstones, duplicate offsets, RowID preservation, publication rewriting, and empty-cutoff backup reads also remain fixed. Two production-reachable v9 compatibility blockers remain. Both occur because v9 deliberately encodes a rollback hole as commitTS=UncommitTS without an abort column, while these consumers recognize only the abort column. git diff --check passes. I did not launch broad BVT/UT jobs, per the requested code-review-only/resource-conscious scope; the repository CI is still running.
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
Re-reviewed current head a898420.
The latest commit closes both previously requested correctness changes:
- logtail generation now filters both v10 abort=true rows and v9 commitTS=UncommitTS rollback sentinels before publishing data/tombstone batches;
- persisted appendable-block duplicate checks and tombstone Contains now skip the v9 rollback sentinel before matching/nulling candidates, for both fixed and varlen paths.
I also re-checked the remaining commitTS/abort consumers and their boundary semantics. Cache-backed reads and change/range paths use finite snapshot/range upper bounds, so the MaxTS sentinel is excluded there; the unbounded logtail and persisted-dedup cases are now explicitly covered. Batch/vector ownership and cleanup paths remain balanced, and the added row scans are bounded by batch size.
No further blocking production issue found. git diff --check is clean. I did not run broad local UT/BVT to avoid duplicating the resource-heavy CI; several CI jobs are still in progress.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head 3afb38a24b86992fd73c6c26b291fee9f04f38eb.
The previous blockers are closed. The bad main merge is corrected; the v9 path preserves physical RowID coordinates and represents rollback holes with commitTS=UncommitTS; empty-cutoff backup reads filter both the v9 sentinel and v10 abort metadata; and the latest change closes the remaining unbounded logtail plus persisted dedup/tombstone-Contains gaps.
I re-audited the same-shape commitTS consumers. Cache-backed reads, CDC/change collection, publication, snapshot, and backup range paths use finite snapshot/range bounds, so the v9 MaxTS sentinel stays invisible there. The consumers that can use an unbounded upper bound now reject the sentinel explicitly. Hidden-column layout resolution supports legacy commitTS-only objects and both RowID placements without exposing abort metadata.
Lifecycle audit also closes: rollback retains a single MVCC owner marked aborted; reader waits release the MVCC lock before waiting on txn completion; retained holes are bounded by the appendable object and reclaimed through its flush/object lifecycle; vector/cache release paths are balanced.
Validation: all 19 focused regression tests across objectio, blockio, logtail replay, publication, TAE tables/index/update/DB packages pass under the controlled CGo environment; git diff --check is clean; required CI is green. No blocking issue remains.
Merge Queue Status
This pull request spent 1 hour 5 minutes 17 seconds in the queue, including 1 hour 4 minutes 51 seconds running CI. Required conditions to merge
|
6 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #26344
What this PR does / why we need it:
This PR adds end-to-end support for rollback holes and persisted abort metadata
in appendable objects.
Keeps rolled-back AppendNode ranges in the MVCC chain and marks them aborted,
preserving ownership of already-applied physical rows and PK-index entries.
Makes memory scans and tombstone collection paths consume rollback holes.
Adds a unified special-column layout resolver instead of inferring commitTS
from the last column or column count.
Persists and reads the abort column for new appendable objects.
Filters aborted rows across snapshot reads, deduplication, tombstone
processing, merge/flush, logtail, publication, and custom object readers.
Extends WAL serialization to preserve the aborted state while retaining V1
compatibility.
Supports legacy objects containing only commitTS; a missing abort column is
interpreted as false.
Supports both valid special-column layouts: