fix: reclaim spilled memtable chain pages

[mory91]

Fixes #379.

Problem

When tp_spill_finalize unlinks the in-flight memtable chain, the old chain pages remain on the index main fork with no reclaim path. Under many spills the relation grows by O(N×K) instead of O(active chain).

Fix

• tp_memtable_mark_chain_dead: before tp_spill_finalize, WAL-stamp each spilled chain page (including fragment continuations) DEAD with ReadNextFullTransactionId() as dead_fxid.
• tp_reclaim_dead_memtable_pages: in tp_vacuumcleanup under LW_SHARED, scan main-fork blocks after the metapage; when dead_fxid precedes the global horizon (FullTransactionIdPrecedes vs FullTransactionIdFromAllowableAt(ReadNextFullTransactionId(), GetOldestNonRemovableTransactionId(...))), call RecordFreeIndexPage.
• tp_memtable_alloc_page: prefer GetFreeIndexPage + zeroed reuse over ExtendBufferedRel on bootstrap, extend, and fragment paths.
• bm25_memtable_dead_pages() SRF for regression / replication checks.

Testing

• make installcheck
• memtable_spill_dead, memtable_reclaim (single-cycle reuse + five spill→VACUUM cycles with bounded main-fork growth)
• vacuum_extended.out updated for FSM-affected L0 block numbers
• test/scripts/replication_memtable_dead_reclaim.sh (physical replication)

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[tjgreen42]

Review by Copilot; posting via @tjgreen42's account.

Two correctness concerns plus one design note. Build is clean and tests pass; comments are about robustness under crash and an orphan class that isn't actually covered.

1. Mark-then-finalize ordering is not crash-safe.

tp_do_spill (src/access/build.c:196-209) calls tp_memtable_mark_chain_dead first, then tp_spill_finalize. The mark loop emits one GenericXLog record per chain page (~64 records at default memtable_pages_threshold), then finalize emits one more record that clears metap.head. A crash between the last mark record and the finalize record leaves a recoverable state where every chain page has flags & DEAD and dead_fxid = pre-crash-fxid, but metap.head still points to the chain. chain_source.c doesn't check DEAD, so the chain stays queryable; that part's fine.

The problem is what happens after the global xmin advances past dead_fxid. tp_reclaim_dead_memtable_pages (vacuum.c:1033-1060) only looks at the DEAD flag and the horizon — it doesn't check reachability — so it RecordFreeIndexPages blocks that are still live in metap.head → next_block → .... A subsequent INSERT pulls one of those blocks back from the FSM via tp_memtable_alloc_page, RBM_ZERO_AND_LOCKs it, and re-initialises it as a fresh chain page. The same block is now reachable via two paths and the index is silently wrong.

The window between last mark and finalize is small (sub-millisecond on a hot system) but it's real, and the post-crash → next-autovacuum window for the corruption to land is long (minutes to hours).

Two ways to fix:

• Reverse the order in tp_do_spill: finalize first (publishes metap.head = Invalid), then mark. A crash between leaves the chain unreachable but un-stamped, which is safe but leaks until the next spill or REINDEX.
• Add a reachability check in tp_reclaim_dead_memtable_pages. Walk metap.head → next_block → ..., including fragment continuations, and skip any candidate block in that set. Costs one chain walk per vacuum cycle (small) and makes the ordering moot.

I'd do both — reverse the order so the failure mode is "leak, not corrupt", and add the reachability check to cover everything the eager stamp misses (including #2 below).

Regression: a debug GUC that PANICs between tp_memtable_mark_chain_dead and tp_spill_finalize, plus a shell test that triggers a spill under it, waits past the horizon, runs VACUUM, then inserts and queries to confirm no corruption.

2. Lost-bootstrap-race orphans in memtable_append_fragment aren't actually reclaimed.

log.c:411-419 returns InvalidBlockNumber on a lost bootstrap race and the comment says "reclaim in Step 2", but the just-extended Tnew + continuations + Thead are unreachable from metap.head and have no DEAD stamp. tp_reclaim_dead_memtable_pages skips non-DEAD pages (vacuum.c:1045-1049), so these leak permanently until REINDEX. Bounded per occurrence (3-5 pages), unbounded over time on a contended index.

Fix: either stamp these DEAD before the early return, or rely on the reachability check from #1 — it handles this class for free.

3. Design note.

An alternative shape worth considering is to skip the spill-side DEAD stamping entirely and let vacuum do the work: walk reachability from metap.head once per vacuum cycle, and reclaim any unreachable memtable page directly. That moves the cost from "1 WAL record per chain page on every spill" to "1 chain walk per vacuum cycle", removes the crash window in #1 by construction, and handles #2 without a separate code path. The tradeoff is two vacuum cycles to fully reclaim (mark + reclaim) instead of one. With the fixes in #1 the two approaches converge in practice, so this is mostly an FYI — not a request to redo the PR.

Otherwise the shape is good: FSM integration is correct, the SRF + replication test are useful, and docs/memtable_v2.md updates land in the right places. Block on #1 and #2 before merge.

[codecov]

Codecov Report

❌ Patch coverage is 86.95652% with 33 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/access/vacuum.c	79.20%	21 Missing ⚠️
src/memtable/log.c	92.18%	10 Missing ⚠️
src/access/build.c	84.61%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[tjgreen42]

Hey @mory91, thanks much for getting this fix going! I'm pushing to cut a release by end of this week and would like to include this fix. If you've got time tonight/tomorrow morning to address the CI failures and PR feedback, that'd be great. Otherwise I will likely take over tomorrow afternoon to get this thing over the finish line in time.

[mory91]

Thanks for the thorough review. Both issues are now addressed:

1. Mark-then-finalize ordering

Implemented both suggested fixes:
• Reversed the order in tp_do_spill: finalize first (clears metap.head), then mark dead. Crash between them leaves pages unreachable but un-stamped — leak not corrupt.
• Added reachability check in tp_reclaim_dead_memtable_pages: walks metap.head chain (including fragment continuations) and skips any block in that set before calling RecordFreeIndexPage.

2. Lost-bootstrap-race orphans

Fixed by calling tp_memtable_mark_chain_dead(rel, thead_blkno, horizon) before the early return. The orphaned pages (thead → continuations → tnew) are already linked as a chain, so the existing function handles them. They'll be reclaimed once the horizon advances.

[tjgreen42]

LGTM