Fix corrupt-tombstone-page reuse in the deferred-free chain (#427) - #429
Draft
tjgreen42 wants to merge 2 commits into
Draft
Fix corrupt-tombstone-page reuse in the deferred-free chain (#427)#429tjgreen42 wants to merge 2 commits into
tjgreen42 wants to merge 2 commits into
Conversation
The index FSM is a non-crash-safe hint whose GetFreeIndexPage() claim is not atomic across backends. Either property can offer a block that is still referenced by a live pg_textsearch structure -- the deferred-free tombstone chain (#380/#427) or the on-disk memtable chain (#426). The three allocators (allocate_segment_page, tp_memtable_alloc_page, tombstone_alloc_page) reinitialized whatever block the FSM handed back without checking, clobbering the live page. A later tombstone drain then raised "corrupt tombstone page ..." on every write path, permanently wedging the index; REINDEX was the only recovery. Prevention: stamp every page returned to the FSM with TP_FREE_PAGE_MAGIC (tp_record_free_index_page) and reuse a block only when it still carries that stamp (tp_fsm_claim_free_buffer / tp_fsm_claim_free_block) -- the same recyclability discipline B-tree uses via _bt_page_recyclable. The buffer-locked claim also serializes the non-atomic FSM hand-out, so two memtable inserts racing under the shared per-index lock can no longer double-allocate a page (#426). A block the FSM wrongly offers for a live page is skipped, not overwritten. The stamp is a small WAL delta (pd_lower collapsed as in tp_tombstone_page_init) and is verified under wal_consistency_checking=all and crash recovery. Recovery: tp_tombstone_drain now self-heals an already-corrupt chain instead of aborting. It drops the unverifiable node and chain tail (pointing the predecessor, or the metapage head, at Invalid), warns, and lets writes proceed; the corrupt node's blocks are never freed, so a double-owned live page is left intact. Leaked pages are reclaimed by REINDEX. Tests: tombstone_reuse (prevention) and tombstone_recover (recovery) reproduce the exact production error deterministically via superuser-only internal scaffolds. Full regression suite green on PG 17 and 18.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Self-review found that the block-returning claim helper released the buffer lock with the free stamp still on the page, so its "caller holds the per-index lock EXCLUSIVE" contract was silently violated by VACUUM's pre-V5 segment rebuild (which runs under LW_SHARED). A concurrent memtable insert, handed the same block by the non-atomic GetFreeIndexPage(), could then still see the stamp and double-allocate the page -- the #411/#426 corruption class this change otherwise closes. Clear the free stamp under the claim lock (WAL-logged) before returning the block. A racing allocator blocks on the buffer lock, then observes a non-recyclable page and skips it, so the claim is atomic regardless of the caller's per-index lock mode. A crash before the caller reinitializes the page only leaks it (out of the FSM, linked nowhere; REINDEX reclaims). Verified under wal_consistency_checking=all with the full suite green on PG 17 and 18.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #427 (same root-cause class as #426). Affected BM25 indexes fail with
ERROR: pg_textsearch: corrupt tombstone page N in index "...", which wedgesevery write path — the drain runs on auto‑spill→merge and on VACUUM — until
REINDEX.Root cause
Index pages come from the Postgres index FSM, a non‑crash‑safe hint whose
GetFreeIndexPage()claim is not atomic across backends. Either property canhand back a block still referenced by a live structure: the deferred‑free
tombstone chain off
metap.pending_free_head(#380/#427) or the on‑diskmemtable chain (#426). The three allocators (
allocate_segment_page,tp_memtable_alloc_page,tombstone_alloc_page) reinitialized the returnedblock without checking, clobbering the live page; the next tombstone drain then
ereport(ERROR)s on the overwritten page and wedges the index. This is thesame recycled‑page family as #411/#413/#418/#420, which never added an
allocation‑side recyclability check.
Fix
Prevention — recyclable free‑page stamping (
src/index/freepage.{c,h}),mirroring B‑tree's
_bt_page_recyclable. Every page returned to the FSM isWAL‑stamped with
TP_FREE_PAGE_MAGIC(tp_record_free_index_page); allocatorsreuse a block only if it still carries the stamp, and skip — never overwrite —
a block the FSM offers for a live page. The claim is atomic against the
non‑atomic FSM hand‑out:
tp_fsm_claim_free_buffer()returns the page EXCLUSIVE‑locked and the callerre‑inits under that lock, closing the memtable insert‑vs‑insert race (BM25 memtable outer walk reaches continuation page after fragment chain corruption #426).
tp_fsm_claim_free_block()clears the stamp under the claim lock beforereturning the block, so it is atomic even for a caller holding only a shared
per‑index lock (VACUUM's pre‑V5 segment rebuild).
All FSM allocators and free sites (tombstone drain,
tp_segment_free_pages,VACUUM dead‑memtable reclaim) go through these.
Recovery — self‑healing drain (
tp_tombstone_drain). On a corrupt chainnode the drain drops that node and the unverifiable tail (predecessor — or the
metapage head — pointed at
Invalid), emits aWARNING, and returns so writesproceed. It never frees the corrupt node's blocks, so a double‑owned live page
is left intact. Leaked pages are reclaimed by
REINDEX. This un‑wedges indexesalready corrupted by an older binary.
Tests
Two regression tests reproduce the production error deterministically via
superuser‑only internal scaffolds:
tombstone_reuse— returns a live tombstone page to the FSM, drives anallocation, and asserts the chain stays intact.
tombstone_recover— corrupts a chain node and asserts VACUUM's drainself‑heals.
Both fail on
mainand pass with the fix.Test plan
makeclean on PG 17 and 18 (no new warnings)make installcheck— full suite green on PG 17 and 18 (73 tests, incl. 2 new)make format-checkwal_consistency_checking = allclean across the free/reuse/recovery flowfsync=onimmediate‑crash recovery preserves the parked tombstone chainNotes
Pages freed by an older binary carry no stamp; after upgrade the claim helpers
skip them — a one‑time leak reclaimed by
REINDEX, never corruption. Noon‑disk format change (metapage v6/v7/v8 unaffected).