Phase F: reconcile free_space against live groups (close truncate's abort seam, keep MVCC)

[jdatcmd]

Closes end-truncation's residual abort/crash window on the MVCC free_space catalog — the alternative to moving the free list off the catalog (now-closed PR #94).

Why (and why not #94)

end-truncation lowers the metapage highwater (a non-transactional FPI that persists on abort) while deleting free_space rows (transactional, roll back). A crash in the narrow window between the highwater lowering and commit can leave a lowered highwater with the trailing free rows restored; a later insert then places a live group over one of those ranges, which a compaction could reuse on top of live data.

PR #94 moved the free list into a non-transactional metapage page to make truncate's effects one persistence class. It was closed because it made truncate atomic at the cost of the common retirement path (which MVCC makes atomic for free) — it needed the same reconciliation anyway, plus a fixed 255-entry page cap and an on-disk format change. Reconciliation is the universal fix and keeps MVCC's retirement atomicity, concurrency, and unbounded free list.

How

ColumnarReconcileFreeList(rel) runs at the start of every reuse op (compact_rewrite, recluster), under SUEL and before any ColumnarAllocateFreeSpace, and deletes any free_space row (base or projection storage) overlapping a live row-group footprint as-of the latest committed state. Inserts never reuse, so no stale range is consumed between the crash and the next reuse. Normal retirement stays atomic, so this is purely a defense for the truncate seam.

No format change, page cap, or SUEL-serialization dependency — it's a small, additive change on top of the merged reclaim.

Test

native_reclaim_reconcile.sh injects a stale free_space row over a live group's byte range (the synthesized post-crash state), runs compact_rewrite, and asserts the reconcile purged it and data is intact. Verified load-bearing: with the reconcile removed, the assert-build no-overlap validator fires (overlapping ranges [16336,+16336) and [16336,+196032)).

PG17 full-suite gate green. Full 15-19 matrix before merge on your go.

🤖 Generated with Claude Code

[ChronicallyJD]

Review: reconcile free_space against live groups (keep MVCC)

This is the right call. Closing #94 and applying reconciliation to the MVCC catalog instead of moving the free list off it keeps retirement atomic for free (so the common path never opens a seam), keeps concurrency and an unbounded list, and drops the format change and the 255-entry cap. The change is small, additive, and correctly scoped to the one seam that genuinely needs it (truncate's FPI-vs-transactional crash window).

I verified the core safety claim: ColumnarAllocateFreeSpace has exactly one runtime caller (columnar_write_state.c:904), gated on CheckRelationLockedByMe(rel, ShareUpdateExclusiveLock). Only compact_rewrite and recluster write groups under SUEL — plain compact writes none, and cluster/truncate hold AccessExclusiveLock so that gate is false and they append rather than reuse. Both reuse ops reconcile before any allocation, so no stale range can be consumed. The corruption path is closed. The cross-storage footprint check (a free row of storage A is tested against B's live footprints too) is also correct — such an overlap is likewise invalid state and should be dropped.

Two things worth addressing before merge, plus minors.

1. Scale: reconcile is O(free_rows x live_footprints) on every reuse op, even when there is nothing to reconcile

collect_footprints gathers every live footprint across base + projections, and for each free_space row the inner loop scans all of them linearly. That runs unconditionally at the start of every compact_rewrite/recluster. In normal operation there are zero stale rows, so this is pure overhead — and the unbounded free list (a stated advantage over #94) is exactly what makes it bite: a heavily-churned large table can have thousands of live groups and thousands of free rows, so the reconcile adds millions of comparisons to every reuse op for a defense that fires only after a crash.

Please make it near-linear: sort foots by offset once and binary-search each free row's range against it (O((n+m) log m)), or keep a running max-end while iterating sorted inputs. The correctness is unchanged; this just removes a quadratic term from the hot maintenance path. (Given the project's history of scale bugs that only showed at full size, worth confirming on the 6M-row bench, not just the small matrix.)

2. The no-overlap invariant is restored only at reuse, but compact and truncate assert it without reconciling (carries over from #94, narrower trigger)

COLUMNAR_ASSERT_NO_OVERLAP(rel) runs at the end of plain compact and of truncate, neither of which calls ColumnarReconcileFreeList. After the truncate crash-residual leaves a stale row (below the advanced highwater, over a live group placed by a later insert), a compact or a subsequent truncate that runs before any compact_rewrite/recluster will:

• in an assert build (the whole version matrix), trip the no-overlap validator — truncate's start purge only removes rows >= highwater, and the stale row sits below it, so it is not cleaned; then the end-of-run assert fires;
• in production, be benign (neither op reuses), leaving the row until the next reuse reconciles.

The trigger is much narrower than on #94 — it needs a real crash in truncate's sub-millisecond window on a default-off feature, not a clean abort — so this is low severity, not a blocker. But it is a genuine inconsistency: the code asserts an invariant it only restores lazily. Cleanest fix is to call ColumnarReconcileFreeList(rel) wherever that assert runs (start of columnar_compact and of columnar_do_end_truncation), so the invariant holds eagerly everywhere and any maintenance op self-heals the residual — not just the two reuse ops. Then extend native_reclaim_reconcile.sh to assert that a plain compact (and a truncate) with the injected stale row present stays green; that addition fails today and pins the gap.

3. Minor

• Portability: storages stuffs a uint64 storage id into a List pointer via (void *) (uintptr_t) base. On a 32-bit (ILP32) build uintptr_t is 32 bits and a storage id > 2^32 truncates. The existing columnar_end_truncation_storages deliberately avoids this by palloc(sizeof(uint64)) and storing pointers to heap uint64s — match that idiom here for consistency and portability.
• tids (and its palloc'd ItemPointerDatas) are never pfreed while foots is; harmless in the short-lived op context, but inconsistent — either free both or neither.
• The test injects len = datalength (raw), whereas real free_space rows store the page-rounded footprint. It still triggers the overlap so the test is valid, but a one-line comment noting the simplification would help a future reader.

Verdict

The design decision is correct and the corruption path is genuinely closed — I would not block on that. Address item 1 (it is a real hot-path regression on large tables and undercuts the unbounded-list selling point) and item 2 (cheap, makes the invariant uniform), run the full 15-19 matrix, and this is merge-ready. Nice to see the simpler approach win.

---

🤖 Review by Claude Code

[ChronicallyJD]

Pre-merge checklist

The corruption path is closed and the design is right; these are the items to land before merge. Blocking unless noted.

Correctness / hygiene

• Reconcile wherever COLUMNAR_ASSERT_NO_OVERLAP runs, not only before reuse — add ColumnarReconcileFreeList(rel) at the start of columnar_compact and columnar_do_end_truncation, so a post-crash stale row cannot trip the no-overlap assert (and any maintenance op self-heals it), instead of the invariant being restored lazily only at the next compact_rewrite/recluster.
• Fix the storage-id list portability bug: replace (void *)(uintptr_t) sid with the codebase idiom (palloc(sizeof(uint64)) + pointer, as in columnar_end_truncation_storages) so a storage id > 2^32 does not truncate on 32-bit builds.

Performance (blocking for large tables)

• Make ColumnarReconcileFreeList near-linear: sort foots by offset once and binary-search each free row (or sweep both sorted), removing the O(free_rows x live_footprints) term it currently adds to every reuse op even when there is nothing to reconcile. The unbounded free list makes this matter at scale.

Tests

• Extend native_reclaim_reconcile.sh to assert that a plain compact AND a truncate with the injected stale row present stay green (both fail today without the item-1 change — that is what pins it).
• Run the full PG 15-19 matrix green (not just the PG17 gate).

Minor / non-blocking

• pfree tids (and its entries) for symmetry with foots, or drop both and rely on context reset — just be consistent.
• One-line comment in the test noting the injected len is raw datalength, not the page-rounded footprint real free_space rows store (still triggers the overlap, so the test is valid).

Follow-up (not this PR)

• The maintenance-family ownership check (compact/recluster/truncate take strong locks with no object_ownercheck) is already tracked in the design doc — fine to keep out of this PR.

Full detail and rationale in the review above.

[ChronicallyJD]

Re-review of 38792d7 — concerns resolved

Verified each checklist item against the code, not just the commit message. All alleviated.

1. Scale (was blocking) — resolved. foots is sorted once (footrange_cmp) and each free row is tested with range_overlaps_footprint (binary search), so the reconcile is O((n+m) log m) instead of O(free_rows x footprints). I checked the binary search is correct: live-group footprints are non-overlapping across base + projections and stay non-overlapping after page-rounding (a reservation's rounded end never exceeds the next page-aligned start), so sorted-by-offset implies ends ascending; lo = count of footprints starting before the range's end, and foots[lo-1].end > start is the max end among candidates — a sound single-probe overlap test. nf == 0 returns false correctly.

2. Assert-consistency (compact/truncate) — resolved. ColumnarReconcileFreeList now runs at the start of columnar_compact (before its end-of-run COLUMNAR_ASSERT_NO_OVERLAP) and inside columnar_do_end_truncation after the >= highwater purge (before its assert). The truncate comment correctly documents both residual shapes (a row >= highwater and a row below it overlapping a live group). The invariant now holds eagerly wherever it is asserted, so any maintenance op self-heals the crash-residual rather than leaving it until the next reuse.

3. Portability — resolved. Storage ids are heap uint64s in the list now, not stuffed into pointers via uintptr_t, matching columnar_end_truncation_storages; list_free_deep frees both storages and tids.

Tests — good. native_reclaim_reconcile.sh now injects the stale row and asserts self-heal for compact_rewrite, plain compact, AND truncate — and it sets enable_end_truncation = on, so the truncate step actually enters columnar_do_end_truncation and exercises the reconcile (not a GUC-off no-op). These three fail without the corresponding reconcile calls, so the gap is pinned. The raw-datalength simplification is noted.

Approving. The corruption path was already closed; this update removes the hot-path quadratic, makes the invariant uniform, and fixes the portability nit — nothing left on the code side. The one remaining pre-merge gate is the full PG 15-19 matrix green (only PG17 is confirmed so far); run that and it's ready to merge. Nice iteration — the simpler MVCC-reconcile design landed clean.

---

🤖 Review by Claude Code