Phase F: physical end-truncation (pgcolumnar.truncate) — REVIEW before merge

[jdatcmd]

Adds pgcolumnar.truncate(regclass) — return trailing reclaimed blocks of a columnar relation's main fork to the OS. Opt-in (pgcolumnar.enable_end_truncation) and best-effort: takes AccessExclusiveLock conditionally for the brief physical step (as PostgreSQL's own lazy-VACUUM truncation does) and returns 0 without waiting if the table is busy.

First truncation WAL in the extension and corruption-critical — requesting review before merge, not auto-merging.

Safety model

• Safe point = end of the highest-offset live row group across the base + every projection storage sharing the file (footprints are page-aligned).
• Horizon guard: the trailing region is removed only when every free_space range there was freed before the oldest-xmin horizon. A more recent retirement could still be resolved by a snapshot older than it, whose read would fault past the shrunken EOF. (Online compact is gated the same way, so tail free space only exists once retirement was already safe — the guard is belt-and-suspenders.)
• Main-fork only: ColumnarTruncateMainFork WAL-logs a main-fork xl_smgr_truncate (SMGR_TRUNCATE_HEAP) and flushes it before the physical op, so recovery and standbys truncate identically. The VM fork (indexed by row-number-derived blocks) is untouched.
• Crash/abort self-heal: smgrtruncate is non-transactional, so an abort/crash leaves "highwater ahead of EOF"; the gap-tolerant write path (Phase F: gap-tolerant writes (end-truncation prerequisite) #91) re-extends on the next write. Readers only touch live groups (none in the truncated region), so nothing is lost and no read faults.

Tests

• native_truncate.sh: shrink-after-compaction, parity vs heap mirror, usability, idempotence, GUC-off no-op, and abort self-heal (truncate in a rolled-back txn, then a successful insert).
• Three isolation specs: truncate_vs_reader (horizon guard — old snapshot keeps the tail and reads correctly), truncate_vs_writer (conditional AEL yields to an uncommitted writer), truncate_vs_truncate (serialize on SUEL).
• Assert-only no-overlap validator runs after truncation.

Review focus

• The WAL/crash/abort ordering (ColumnarTruncateMainFork + ColumnarSetReservedOffset + free_space deletes; self-heal via gap-tolerant writes).
• The conditional-AEL best-effort model vs the "AEL is a killer" preference.
• xl_smgr_truncate.rlocator field name and smgrtruncate signature across PG15-19 (validated on PG17; the daily full-matrix gate will exercise 15/16/18/19).

PG17 full-suite gate green.

🤖 Generated with Claude Code

[ChronicallyJD]

Review: physical end-truncation (pgcolumnar.truncate)

The design document is genuinely good, the isolation specs pin exactly the right hazards, and the horizon-guard reasoning is sound. But this review found one corruption bug and two build breaks, so the recommendation is request changes, do not merge yet.

---

1. CRITICAL: abort/crash leaves a double-allocation corruption hazard

The design doc and the comment in columnar_do_end_truncation claim that on abort "the metapage highwater and free_space deletes are [rolled back], leaving highwater ahead of EOF." The highwater half of that claim is wrong. ColumnarSetReservedOffset mutates the metapage buffer and logs an FPI (log_newpage_buffer); page changes are never rolled back on transaction abort, and the FPI replays unconditionally in recovery. The free_space deletes, by contrast, are transactional heap deletes and do roll back.

So BEGIN; SELECT pgcolumnar.truncate(...); ROLLBACK; (or a crash mid-transaction) leaves the inverted state:

• reservedOffset = lowered to liveEnd (persisted)
• trailing free_space rows at offsets >= liveEnd restored
• file truncated to liveEnd

From there:

1. A plain INSERT reserves from the lowered highwater and writes a live group at [liveEnd, liveEnd+X) (ColumnarReserveOffset, columnar_storage.c).
2. A later compaction write calls ColumnarAllocateFreeSpace (columnar_write_state.c:904), which has no guard against ranges at or above the highwater. It best-fits the restored row covering the same bytes (its freed_xid is old, so the horizon check passes) and writes a second group over the first. Silent data corruption.

Even without step 2, the state violates the no-overlap invariant: COLUMNAR_ASSERT_NO_OVERLAP would fire if it ran after step 1. Concrete repro: in native_truncate.sh, add a SELECT pgcolumnar.compact('n') after the "insert self-heals after aborted truncate" step; the assert-enabled PG17 gate should go red as-is.

The safe abort state is the one the doc intended: highwater high + rows restored + file short (that combination genuinely self-heals via the gap-tolerant writes of #91). The problem is specifically that the highwater lowering is non-transactional while the deletes are transactional. Fix directions, in order of robustness:

• Architectural: move free-list bookkeeping into a non-transactional structure (metapage or dedicated pages) so all three effects share one persistence class. Bigger change; probably right long-term.
• Practical: (a) PreventInTransactionBlock() so a user-level ROLLBACK cannot produce the state (VACUUM-style; it matches the nature of the operation and keeps the AEL genuinely brief, see item 5); (b) purge any free_space row at or above reservedOffset first thing under the AEL (under AEL such rows are stale by definition), which makes the residual crash window self-healing on the next truncate; (c) optionally teach the allocator to skip rows at or above the highwater as defense in depth. Note (c) alone is racy against a concurrent reserve, so (b) is the load-bearing part.
• Whatever the choice, fix the incorrect claim in design/PHASE_F_END_TRUNCATION_PLAN.md and the columnar_do_end_truncation comment.

2. BUILD BREAKS: PG15 and PG18/19 (the exact items the PR flagged for review)

Verified against installed headers for all five majors:

• PG15: xl_smgr_truncate has RelFileNode rnode; the rlocator field name arrives in PG16. xlrec.rlocator = ... in ColumnarTruncateMainFork will not compile on PG15. Needs a compat shim next to the existing COLUMNAR_SMGR_LOCATOR in columnar_compat.h.
• PG18/19: smgrtruncate takes five args (old_nblocks added); the 4-arg call breaks. PG15-17 all ship smgrtruncate2 with the identical 5-arg signature (backpatched with the extension-race CVE fix), so the clean shim is: call smgrtruncate2 on <=17 and smgrtruncate on >=18, passing &oldnblocks, which the function already has in hand.

Good instinct flagging both in the PR body; both are real.

3. Missing upstream truncation protections in ColumnarTruncateMainFork

Upstream RelationTruncate wraps WAL-log + smgrtruncate in MyProc->delayChkptFlags |= DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE and a critical section, because truncation drops dirty buffers a checkpoint would otherwise have flushed; a checkpoint completing in that window plus a crash can leave stale block contents that break replay. The PR has neither.

Honest severity assessment: pgcolumnar's exposure is lower than heap's. Every page write in columnar_storage.c is a full-page image, FPI replay never reads prior contents, and the truncated region contains no live groups; no concrete failure was constructed. But the guard costs two lines plus a critical section, keeps the safety argument simple ("we mirror RelationTruncate"), and avoids relying on invariants that may erode. For a corruption-critical first truncation WAL, add it. Same reasoning applies to RelationPreTruncate() (pending-sync tracking for wal_level=minimal same-transaction-created tables): call it or document why it is unnecessary.

4. GUC default contradicts the stated model

The PR body says opt-in and the design doc says "default off initially, flipped to on once proven", but columnar_enable_end_truncation defaults to true (columnar_vacuum.c and the _PG_init registration). Given item 1, ship with off until the abort path is fixed and matrix-validated, then flip in a follow-up. Also decide whether PGC_USERSET is right or whether this should be PGC_SUSET as an operational safety valve.

5. Privilege and lock-duration notes

• No ownership check: any user can call pgcolumnar.truncate('any_columnar_table') and take a (conditional) AEL plus SUEL-until-commit on someone else's table. This is a pre-existing pattern (compact and recluster have no check either), but truncate is the strongest of the family. Recommend an object_ownercheck/pg_class_ownercheck gate across all the maintenance functions; fine as a follow-up PR, but worth a tracking note now.
• The "brief" AEL is only brief under autocommit; it is held to transaction end, so inside a user transaction block it blocks everything until commit. PreventInTransactionBlock() (from item 1) fixes this too, which is why that option is preferred.

6. Minor

• columnar_truncate calls PG_GETARG_OID(0) before the PG_ARGISNULL(0) check. Harmless, but reorder or declare the function STRICT.
• columnar_end_truncation_storages O(n^2) dedup: fine at realistic projection counts.
• New function + GUC have .sql comments but no user-doc update; add to whatever DBA-facing doc covers compact.

What's good

• The horizon guard being belt-and-suspenders over compact's own gating is correctly reasoned and correctly tested. truncate_vs_reader is exactly the right spec, and the "reclaimed = f while reader holds snapshot, t after commit" expected output proves the guard works rather than just not-crashing.
• Conditional AEL matching lazy VACUUM's model is the right call and satisfies the lock-minimization bar: no weaker lock makes smgrtruncate's buffer contract safe.
• Main-fork-only scoping with the VM-fork rationale documented at the call site.
• The gap-tolerant-writes-first sequencing (#91 before this) was the right decomposition.

Verdict

Request changes. Fix order: item 2 (mechanical), item 1 (the real work; add the compact-after-abort repro to native_truncate.sh so the fix is pinned by a test), items 3/4 (cheap hardening + default flip), then the full 15-19 matrix before merge. The core design is sound; the abort-path persistence mismatch is the one genuine flaw, and it is fixable without rethinking the approach.

---

🤖 Review by Claude Code

[jdatcmd]

Thank you — this was a genuinely valuable review. It caught a real corruption bug and two real build breaks. All addressed in f36d45c.

1. CRITICAL abort-state corruption — fixed

Confirmed exactly as described: the metapage highwater lowering is an FPI that persists across abort, while the free_space deletes are transactional and roll back, so the inverted state was reachable. Fixed with the "practical" combination plus a reorder:

1. No transaction block: columnar_truncate now errors via IsTransactionBlock() (VACUUM-style), so a user ROLLBACK cannot produce the state, and the AEL is genuinely brief (also resolves the item-5 lock-duration note).
2. Truncate before lowering the highwater: reordered so a crash in the large window leaves "highwater high + file short", which the gap-tolerant path self-heals.
3. Purge stale free ranges (offset ≥ highwater, stale by definition under the lock) at the start of every truncation, to clean the residual crash window on the next run.

The doc's incorrect "highwater rolls back" claim is corrected with the persistence-class reasoning and an honest write-up of the remaining narrow crash window. The architectural fix (free list in the metapage, one persistence class) is tracked as a follow-up in the design doc. On the requested repro: with the transaction-block prohibition the BEGIN; truncate; ROLLBACK path is now an error rather than a corruption, so the test asserts that refusal plus "truncate + reinsert into reclaimed space + compaction keeps the no-overlap validator green."

2. Build breaks — fixed and verified

• PG15 rnode vs PG16+ rlocator: new COLUMNAR_XLREC_SET_LOCATOR in columnar_compat.h.
• PG18/19 5-arg smgrtruncate: new COLUMNAR_SMGRTRUNCATE calls smgrtruncate2 on ≤17 and smgrtruncate on ≥18, passing &oldnblocks.

Build-clean (0 warnings) on PG15, PG18, and PG19, in addition to the PG17 full-suite gate.

3. Upstream truncation protections — added

ColumnarTruncateMainFork now mirrors RelationTruncate: RelationPreTruncate, MyProc->delayChkptFlags |= DELAY_CHKPT_COMPLETE across the record + physical op, and a critical section.

4. GUC default — fixed

Default flipped to off and changed to PGC_SUSET.

5. Ownership check — tracked as follow-up

Agreed it belongs across the whole maintenance family (compact/recluster/vacuum* share the gap). Tracked in the design doc's follow-ups for a single PR with the PG15/16 object_ownercheck compat.

6. Minor — done

Null-arg check moved before PG_GETARG_OID; the two GUCs added to configuration.md and pgcolumnar.truncate to administration.md. The O(n²) projection dedup is left as-is (fine at realistic counts).

Full 15-19 matrix will run in the daily gate before this is merge-ready. Still holding for your re-review rather than self-merging.

[ChronicallyJD]

Re-review of f36d45c

This is a strong response. Every mechanical item was verified fixed against the code: both compat shims are correct (smgrtruncate2 on <=17 is exactly the right version-switch), the GUC is off/SUSET, the truncation envelope now has RelationPreTruncate + critical section, the null-check ordering is fixed, and the new docs are in house style. The test rework (refusal assertion + truncate/reinsert/compact validator cycle) is sensible.

However, verification found one new bug introduced by the fix and one residual hole, plus a two-word omission. Still requesting changes, but this is close.

1. NEW BUG: the self-heal purge breaks the very path it exists for (missing CCI)

columnar_do_end_truncation purges stale free_space rows (offset >= highwater) via CatalogTupleDelete, but does not CommandCounterIncrement() afterward. The subsequent ColumnarTrailingFreeSpaceSafe and ColumnarDeleteFreeSpaceAtOrAbove(liveEnd) scans run in the same command, so MVCC still shows the purged tuples as visible (cmax == curcid). Since liveEnd <= highwater, the second delete pass collects the same tuples and the repeat CatalogTupleDelete -> simple_heap_delete fails with elog(ERROR, "tuple already updated by self").

Stale rows only exist when there is crash residue to heal, so the healing truncate errors exactly when it has work to do -- and normal passes never exercise the purge, so the green PG17 gate does not cover it. Fix: CommandCounterIncrement() immediately after the purge loop. Consider a test that manufactures a stale row (e.g. via the debug hook plus a manual free_space insert) to pin the healing path.

2. RESIDUAL: the abort hazard is narrowed, not closed

IsTransactionBlock() returns false during a DO block or any implicit single-statement transaction. This still reproduces the inverted state:

SET pgcolumnar.enable_end_truncation = on;
DO $$ BEGIN PERFORM pgcolumnar.truncate('n'); RAISE EXCEPTION 'x'; END $$;

The abort restores the trailing free_space rows while the lowered highwater persists (the same applies to any error raised later in the same statement that called the function). Two notes on the mitigations as implemented:

• The reorder (truncate before lowering) helps crash windows only. Abort semantics are order-independent: all non-transactional effects persist and all transactional ones roll back regardless of ordering, so the abort end-state is identical to the pre-fix one.
• The purge heals only if the next truncate() runs before any insert. Once an insert advances the highwater past the stale rows, the purge threshold can no longer identify them, and a later compaction reuse double-allocates over the live group.

The airtight fix is allocation-time overlap validation in ColumnarAllocateFreeSpace: before consuming a candidate range, check it against the live row-group footprints of every storage sharing the file, and skip (or skip-and-delete) any overlapping row. It only runs on the SUEL compaction path, so an O(live groups) check per allocation is acceptable, and it closes every producer of the state -- DO-block aborts, mid-statement errors, and crash residue that survived past an insert. Recommended now, with the metapage-free-list follow-up as the architectural closure. If you instead choose accept-and-document (defensible given SUSET + default-off), the design doc's residual section should name the DO-block/mid-statement abort path explicitly, not just the crash window.

3. DELAY_CHKPT_START omitted

Upstream RelationTruncate sets DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE. Its second documented reason applies verbatim here: smgrtruncate registers a sync request that must be processed before any checkpoint whose redo pointer follows the truncate record; otherwise the WAL record can precede the checkpoint while the actual sync waits for the next one. The fix took only DELAY_CHKPT_COMPLETE. Add DELAY_CHKPT_START (and mirror it in the Assert).

Verdict

Fix 1 and 3 are one-liners; 2 is a judgment call where the allocator-side check is recommended. After those plus the full 15-19 matrix, this is merge-ready from my side.

---

🤖 Review by Claude Code