Phase F: move the reclaim free list into block 1 (for review)

[jdatcmd]

Replaces the pgcolumnar.free_space catalog table with a non-transactional free list stored in block 1 of each columnar relation's main fork (previously reserved and empty; data starts at block 2). The free list and the metapage highwater now share one persistence class.

Corruption-critical (on-disk format + reclaim crash/abort behavior). Requesting review, not auto-merging. Design: design/PHASE_F_FREE_LIST_METAPAGE_PLAN.md.

Why

End-truncation (PR #92) had a documented residual abort/crash window because the highwater lowering (a non-transactional FPI) and the free_space deletes (transactional) were different persistence classes. Moving the free list to block 1 makes them one class, so purging trailing entries + lowering the highwater + smgrtruncate are all non-transactional page/file ops — a crash or abort leaves the same consistent state as a commit. The window is eliminated, not narrowed. Secondary wins: removes a catalog table and the per-allocation systable scan; reduces reclaim to a single durability class.

How

• Block 1 is self-describing: a packed ColumnarFreeEntry array (storage_id, file_offset, byte_length, freed_xid), count = pd_lower, each update one FPI write (ColumnarFreeListRead/Write). No metapage struct change; format minor bumped 2.2 → 2.3.
• record_free_space / ColumnarAllocateFreeSpace / ColumnarTrailingFreeSpaceSafe / ColumnarDeleteFreeSpaceAtOrAbove / the assert no-overlap validator all operate on the block-1 array (Relation threaded through). All mutations are under SUEL, so a buffer lock replaces MVCC; coalescing / split / horizon gating unchanged.
• Capacity is one page (~255 entries). Coalescing keeps it compact; on overflow a range is simply not recorded (reclaimed by the next full rewrite) — graceful degradation, logged, never corruption.
• pgcolumnar.free_list(regclass) replaces the queryable table. TRUNCATE TABLE clears block 1. Truncate's txn-block guard/ordering retained as belt-and-suspenders (the window they guarded is now closed); relaxing them is a noted follow-up.

Review focus

• The SUEL-serialization invariant (buffer lock instead of MVCC) — is every record_free_space/ColumnarAllocateFreeSpace path really under SUEL?
• The overflow-leak fallback and its cap.
• Block-1 lifecycle: init (already PageInit'd), TRUNCATE TABLE reset, crash/replication via FPI.

Tests

All reclaim/behavior suites pass unchanged (native_reclaim*, native_gap, native_truncate, isolation, differential, …); native_reclaim/frag now introspect via free_list(). New opt-in native_free_overflow.sh fills the page to exactly its 255-entry capacity and asserts graceful degradation with the no-overlap validator green. PG17 full-suite gate green; full 15-19 matrix runs in the daily gate.

🤖 Generated with Claude Code

[ChronicallyJD]

Review: move the reclaim free list into block 1

This is the architectural fix I flagged as the #92 follow-up, and the mechanics are well executed: block 1 is PageInit'd and WAL-logged at creation, ColumnarFreeListWrite uses the standard log_newpage_buffer(page_std=true) layout correctly (entries below pd_lower, hole above elided/zeroed on replay), coalescing/split/horizon logic is preserved, and the introspection swap (free_space table -> free_list() SRF) is clean. Truncate's own abort seam is genuinely closed now.

But verification found a critical regression: the change closes the truncate seam by opening the same class of bug on the compaction path. Request changes.

1. CRITICAL: retirement/reuse is no longer crash/abort-atomic with the row_group catalog

The design unifies the free list with the metapage highwater (both non-transactional) and reasons entirely about truncate. But the free list must ALSO stay consistent with the row_group catalog, which is still transactional — and moving the list to a non-transactional page breaks that second relationship on the retirement/reuse path.

record_free_space is now a non-transactional block-1 write (ColumnarFreeListWrite = buffer + log_newpage_buffer, not rolled back on abort). It is called from ColumnarRetireGroup, inside compact / compact_rewrite / recluster, whose row_group catalog deletes/inserts ARE transactional. None of those three has a transaction-block guard (only truncate got one at line 1597, verified).

Repro (trivial, no crash needed):

BEGIN;
SELECT pgcolumnar.compact('t');   -- retires fully-deleted groups:
                                  --   row_group deletes  (transactional)
                                  --   record_free_space  (block 1, NON-transactional)
ROLLBACK;

After the ROLLBACK: the row_group deletes roll back (the groups are live again), but the block-1 free entries for those groups' ranges persist. Now live groups and free-list entries cover the same bytes. A later compact/insert calls ColumnarAllocateFreeSpace, best-fits one of those stale entries, and writes a new group on top of a live group -> silent data corruption. The freed_xid < oldestXmin gate only delays this (the aborted xid falls behind the horizon soon enough); it does not prevent it. The same happens on any mid-statement error during compaction in autocommit, and on crash.

Under the OLD catalog design both effects were transactional, so retirement was atomic and abort-consistent. This PR made truncate atomic (both non-transactional) but made retirement non-atomic (mixed) — it moved the persistence-class seam from truncate to compaction rather than eliminating it, and compaction is the common path with no txn-block guard.

COLUMNAR_ASSERT_NO_OVERLAP would fire on the next compaction in an assert build, but no test aborts a compaction (the overflow test never aborts; the truncate-abort test is now guarded), so the matrix is green while the hazard is live in production builds.

Fix options:

• Allocator-side overlap validation (the same mechanism I recommended for #92, now effectively mandatory): before consuming a free entry in ColumnarAllocateFreeSpace, reject/drop any entry overlapping a live row_group footprint of any storage in the file. This neutralizes stale entries regardless of how they arose (aborted retirement, crash) and is cheap on the SUEL path. Pair it with a reconciliation pass at compaction start that purges free entries overlapping live groups.
• Derive the free list instead of storing it: treat free space as the gaps between live-group footprints below the highwater, computed from the catalog, with block 1 as a pure cache. Then there is no free-list/catalog persistence-class relationship to violate at all. Bigger change, but it is the version that actually eliminates the seam instead of relocating it.
• A txn-block guard on compaction would only cover the explicit-ROLLBACK case, not mid-statement errors or crashes, so it is not sufficient alone.

Please also add a test that aborts a compaction mid-flight (explicit BEGIN; compact; ROLLBACK, and ideally an injected mid-loop error) and then asserts the no-overlap validator stays green and a subsequent reuse does not double-allocate.

2. MEDIUM: the SUEL-serialization invariant is documented but not asserted

The whole justification for dropping MVCC and using a released-between-read-and-write buffer pattern is "every mutation happens under ShareUpdateExclusiveLock, which self-conflicts, so mutations are globally serialized." The design doc explicitly says "Implementation must assert this invariant." It is not asserted anywhere — ColumnarFreeListWrite only checks the count bounds.

ColumnarFreeListRead/Write release the buffer lock between the read and the write, so a read-modify-write is atomic ONLY because SUEL/AEL keeps mutators single-file. That is load-bearing and invisible; a future caller that invokes record_free_space/ColumnarAllocateFreeSpace (or the free-list writers directly) without SUEL would silently corrupt with no failing test. Add, in ColumnarFreeListWrite:

Assert(CheckRelationLockedByMe(rel, ShareUpdateExclusiveLock, false) ||
       CheckRelationLockedByMe(rel, AccessExclusiveLock, false));

(AEL covers truncate and TRUNCATE TABLE's ColumnarFreeListWrite(rel, NULL, 0).)

3. MINOR

• Version gate is major-only. ColumnarReadMetapage checks versionMajor but not versionMinor, and the minor bump 2.2 -> 2.3 is the on-disk change. A pre-existing 2.2 table opened by 2.3 code passes the check and reads block 1 as an empty list (its old catalog free_space is gone) — silent, not a clear "recreate the table" error. Since the PR says there is no migration path, consider bumping the major (or rejecting versionMinor < 3) so old tables fail loudly instead of silently losing tracked free space.
• ColumnarFreeListRead count arithmetic relies on unsigned wraparound. (int)((pd_lower - SizeOfPageHeaderData) / sizeof(...)) is evaluated in size_t; for a hypothetical pd_lower < 24 it wraps huge and the count < 0 guard only catches it because the cast-to-int happens to land negative (implementation-defined). Block 1 is always PageInit'd so this is unreachable today, but it is fragile — read pd_lower into a signed local and check pd_lower >= SizeOfPageHeaderData explicitly.
• Orphaned entries on projection drop. Block 1 is per-file, keyed by storage_id. If a projection storage is dropped while the base file lives, its free-list entries have no owner to ever match them again and sit until a full rewrite — a slow capacity leak against the 255-entry cap. Worth a note or a purge-by-storage in ColumnarDeleteMetadata's projection path.
• Doc says "~253" in one place and "~255" in another; the code is 255. Align.

Positives

• Block-1 lifecycle is correct end to end: PageInit + log_newpage + smgrimmedsync at creation, re-init on TRUNCATE TABLE, FPI on every mutation for crash/replication parity.
• The page format (packed array, count = pd_lower, no line pointers) with page_std WAL is a clean, self-describing choice and removes the per-allocation systable scan.
• Removing the catalog table and its two indexes is a real simplification, and the same-transaction coalescing that keeps the list compact is retained intact.
• The overflow/graceful-degradation design (drop-and-log, never corrupt, reclaim on rewrite) is sound and the new native_free_overflow.sh exercises it at exactly the page cap.

Verdict

Request changes. Item 1 is the blocker and, I think, forces a design decision: either the allocator-side overlap check (make the free list self-defending against stale entries) or deriving free space from the catalog (remove the seam entirely). The store-it-in-block-1 approach as written trades #92's truncate hazard for a compaction hazard that is easier to hit. Items 2-3 are small but item 2 should land with any version of the fix, since the SUEL invariant is what the whole no-MVCC design rests on. Full 15-19 matrix before merge, plus the mid-compaction-abort test.

---

🤖 Review by Claude Code

[jdatcmd]

Another sharp review — item 1 was a real regression I introduced, and you're exactly right about the mechanism. Fixed in 9c9f1a2.

1. CRITICAL: retirement no longer atomic with row_group — fixed

Confirmed: moving the free list off the catalog made truncate atomic but made retirement mixed (non-transactional block-1 write + transactional row_group deletes), so BEGIN; compact; ROLLBACK (or a mid-statement error / crash) left stale free entries overlapping now-live groups. I went with your first option (self-defending free list):

• ColumnarReconcileFreeList(rel) runs at the start of every reuse op (compact_rewrite, recluster), under SUEL, and drops any free entry overlapping a live row-group footprint (base or projection storage) as-of the latest committed state. Between an abort and the next reuse only plain inserts run, and inserts never reuse (SUEL-gated), so no stale entry is ever consumed. This keeps the two persistence classes consistent regardless of how the staleness arose.

Pinned by the new native_reclaim_abort.sh: BEGIN; compact_rewrite; ROLLBACK leaves a stale entry (asserted count > 0), then a real compaction reconciles and data stays intact against a heap mirror. Verified it's load-bearing — with the reconcile removed, the test fails with ERROR: columnar reclaim: overlapping ranges [16336,+16336) and [16336,+196032) (your exact double-allocation) and parity breaks. It's in the default suite.

I kept the stored-list-plus-reconcile approach rather than deriving free space from the catalog (your option 2): it's surgical and self-defends against any cause (abort, crash, mid-statement error), and the derive-from-catalog horizon-gating rework is a larger change I've noted as a possible future simplification.

2. SUEL invariant asserted — done

ColumnarFreeListWrite now Asserts CheckRelationLockedByMe(rel, ShareUpdateExclusiveLock, false) || CheckRelationLockedByMe(rel, AccessExclusiveLock, false), with a comment that this is what the no-MVCC design rests on.

3. Minor — all done

• Version gate: ColumnarReadMetapage now rejects versionMinor < 3 with a "recreate the table" errdetail, so a pre-2.3 table fails loudly instead of silently reading block 1 as an empty list.
• pd_lower arithmetic: read into a signed local; explicit >= SizeOfPageHeaderData check, no unsigned-wraparound reliance.
• Orphaned projection entries: drop_projection now purges the dropped storage's block-1 entries (defensive; projections write no data yet, so it's a no-op today).
• Doc 253 vs 255: aligned to 255.

PG17 full-suite gate green (incl. the new abort test). The full 15-19 matrix is required before merge (format change + new buffer path) — I'll run it on your go, or it'll run in the daily gate. Holding for re-review.

[ChronicallyJD]

Re-review of 9c9f1a2

The critical regression is genuinely fixed and I verified the fix is load-bearing and correctly scoped. ColumnarReconcileFreeList drops exactly the stale entries (overlapping a live footprint = an aborted/crashed retirement) and keeps the legitimately-free ones (no live group occupies them in the committed state), it runs before any ColumnarAllocateFreeSpace on both reuse paths under SUEL, and consumption only happens on those paths — so no stale entry can ever be handed out over a live group. The corruption path is closed. Items 2 and 3 are all correctly done (SUEL assert covers SUEL||AEL for every mutator, pd_lower read is now signed-safe, version gate rejects < 3 with a recreate hint, projection purge, doc alignment).

One residual, smaller than before. Recommend one more change before merge, but this is close.

1. RESIDUAL: the no-overlap invariant is now only restored at reuse, but compact and truncate assert it eagerly without reconciling

The reconcile runs at the start of compact_rewrite and recluster (the reuse ops). But COLUMNAR_ASSERT_NO_OVERLAP(rel) also runs at the end of plain compact (verified: columnar_compact retires then asserts, with no reconcile) and at the end of truncate. Neither reconciles, and after an aborted retirement the invariant they assert does not hold until some later reuse op reconciles.

Concrete, in an assert build (the entire version matrix builds with asserts):

BEGIN; SELECT pgcolumnar.compact_rewrite('t', 0.1); ROLLBACK;  -- leaves a stale entry over a live, partially-deleted group
SELECT pgcolumnar.compact('t');   -- retires nothing relevant, then COLUMNAR_ASSERT_NO_OVERLAP -> fires

truncate after the same abort trips identically (its start-purge only removes entries >= highwater; the stale entry sits below the highwater, over the live group). This is production-benign — compact and truncate never call ColumnarAllocateFreeSpace, so there is no double-allocation — but it is an assert-build fragility, and it means stale-entry cleanup depends on a rewrite/recluster eventually running rather than being restored by the next maintenance op of any kind.

It is currently latent: the new native_reclaim_abort.sh uses compact_rewrite (which reconciles) as its recompaction, so nothing in the matrix does aborted-rewrite -> plain compact or -> truncate. That is exactly why it slips through green.

Fix: run ColumnarReconcileFreeList(rel) wherever the no-overlap assert runs, not only before reuse — i.e. also at the start of columnar_compact and columnar_do_end_truncation. It is one cheap row_group scan, and it makes the invariant hold eagerly everywhere it is asserted (and makes plain compact, the most common op, self-heal stale entries too, instead of leaving them until a rewrite). Then extend native_reclaim_abort.sh to assert that plain compact and truncate after the aborted rewrite both stay green — that addition would fail today and is what pins the gap closed.

2. Optional / notes (non-blocking)

• The reconcile drops a whole entry on any overlap. That is correct here because an aborted retirement's entry is always the full page-rounded footprint (same-transaction entries are never split, since the horizon gate blocks same-txn reuse), so it exactly matches the restored live footprint. Worth a one-line comment saying so, so a future reader does not think partial-overlap could silently discard genuinely-free tail space.
• Version gate accepts versionMinor > 3 (a hypothetical future format opened by this build passes). Fine for a dev extension; just flagging the asymmetry in case you want strict equality later.

Verdict

The corruption fix is correct and verified — this is no longer a blocker on that axis. I would merge after (a) reconciling in compact and truncate so the no-overlap assert holds eagerly, (b) extending the abort test to cover plain compact/truncate after an abort, and (c) the full 15-19 matrix (format change + new buffer path). Everything else in this update is right.

---

🤖 Review by Claude Code

[jdatcmd]

Closing in favor of keeping the MVCC free_space catalog. Discussion concluded that moving the free list to a non-transactional page optimized the rare truncate path at the cost of the common retirement path (which MVCC made atomic for free), and that the reconciliation this PR added is the universal fix that works on the MVCC design too. Reconciliation will be ported onto the MVCC free_space catalog in a small follow-up (keeps retirement atomicity, MVCC concurrency, and an unbounded free list; no format change, no 255-entry cap, no SUEL-serialization dependency). See the PR discussion for the analysis.