diff --git a/Makefile b/Makefile index dde965ef1..43bd206ba 100644 --- a/Makefile +++ b/Makefile @@ -59,6 +59,7 @@ OBJS = \ src/index/state.o \ src/index/registry.o \ src/index/metapage.o \ + src/index/freepage.o \ src/index/limit.o \ src/index/resolve.o \ src/index/source.o \ @@ -82,7 +83,7 @@ PG_CPPFLAGS += -Wno-unknown-warning-option -Wno-clobbered -Wno-packed-not-aligne # PG_CPPFLAGS += -DDEBUG_DUMP_INDEX # Test configuration -REGRESS = abort aerodocs basic binary_io bmw bmw_skip_advance bulk_load cache_apply cache_memory_cap cache_source cache_spill catalog_stats chain_source compression concurrent_build coverage deletion vacuum vacuum_bitmap vacuum_extended vacuum_rebuild dropped empty explicit_index expression_index force_merge implicit index inheritance large_documents limits lock manyterms memory memtable_append memtable_page memtable_spill memtable_spill_dead memtable_reclaim merge mixed parallel_build parallel_bmw partitioned partitioned_many partial_index pgstats queries quoted_identifiers rescan schema scoring1 scoring2 scoring3 scoring4 scoring5 scoring6 security segment segment_integrity segment_reclaim strings temp_table text_array text_config unsupported updates vector vector_v1_rejected unlogged_index wand +REGRESS = abort aerodocs basic binary_io bmw bmw_skip_advance bulk_load cache_apply cache_memory_cap cache_source cache_spill catalog_stats chain_source compression concurrent_build coverage deletion vacuum vacuum_bitmap vacuum_extended vacuum_rebuild dropped empty explicit_index expression_index force_merge implicit index inheritance large_documents limits lock manyterms memory memtable_append memtable_page memtable_spill memtable_spill_dead memtable_reclaim merge mixed parallel_build parallel_bmw partitioned partitioned_many partial_index pgstats queries quoted_identifiers rescan schema scoring1 scoring2 scoring3 scoring4 scoring5 scoring6 security segment segment_integrity segment_reclaim tombstone_reuse tombstone_recover strings temp_table text_array text_config unsupported updates vector vector_v1_rejected unlogged_index wand REGRESS_OPTS = --inputdir=test --outputdir=test PG_CONFIG ?= pg_config diff --git a/sql/pg_textsearch--1.4.0-dev.sql b/sql/pg_textsearch--1.4.0-dev.sql index e478ed39c..a7b3c797e 100644 --- a/sql/pg_textsearch--1.4.0-dev.sql +++ b/sql/pg_textsearch--1.4.0-dev.sql @@ -259,11 +259,35 @@ CREATE FUNCTION @extschema@.bm25_pending_free_pages(index_name text) AS 'MODULE_PATHNAME', 'tp_pending_free_pages' LANGUAGE C STRICT STABLE; +-- INTERNAL-ONLY test scaffold (issues #426, #427): return the live +-- head tombstone page to the index FSM so the next allocator can pick +-- it up, reproducing the stale-FSM / non-atomic-claim page-reuse +-- hazard without an actual crash. Superuser-only; not a supported API. +CREATE FUNCTION @extschema@.bm25_test_recycle_tombstone_head( + index_name text) + RETURNS bigint + AS 'MODULE_PATHNAME', 'tp_test_recycle_tombstone_head' + LANGUAGE C VOLATILE STRICT; + +-- INTERNAL-ONLY test scaffold (issue #427): overwrite the head +-- tombstone page's magic so the chain node is corrupt, simulating a +-- page-reuse clobber, to exercise the drain's self-healing recovery. +-- Superuser-only; not a supported API. +CREATE FUNCTION @extschema@.bm25_test_corrupt_tombstone_head( + index_name text) + RETURNS bigint + AS 'MODULE_PATHNAME', 'tp_test_corrupt_tombstone_head' + LANGUAGE C VOLATILE STRICT; + -- Revoke public execute on debug functions (superuser-only). REVOKE EXECUTE ON FUNCTION @extschema@.bm25_dump_index(text) FROM PUBLIC; REVOKE EXECUTE ON FUNCTION @extschema@.bm25_summarize_index(text) FROM PUBLIC; REVOKE EXECUTE ON FUNCTION @extschema@.bm25_pending_free_pages(text) FROM PUBLIC; +REVOKE EXECUTE ON FUNCTION + @extschema@.bm25_test_recycle_tombstone_head(text) FROM PUBLIC; +REVOKE EXECUTE ON FUNCTION + @extschema@.bm25_test_corrupt_tombstone_head(text) FROM PUBLIC; -- The bm25_test_memtable_page / bm25_test_memtable_append / -- bm25_test_chain_source / bm25_memtable_chain / diff --git a/src/access/vacuum.c b/src/access/vacuum.c index f14686279..b48bccf21 100644 --- a/src/access/vacuum.c +++ b/src/access/vacuum.c @@ -32,6 +32,7 @@ #include "access/am.h" #include "access/build_context.h" +#include "index/freepage.h" #include "index/metapage.h" #include "index/state.h" #include "memtable/page.h" @@ -1277,8 +1278,20 @@ tp_reclaim_dead_memtable_pages(Relation indexrel, Relation heaprel) hash_search(reachable, &blk, HASH_FIND, &found); if (!found) { - RecordFreeIndexPage(indexrel, blk); + /* + * Release the SHARE lock before returning the page to + * the FSM: tp_record_free_index_page re-locks + * EXCLUSIVE to write the recyclable free stamp so a + * later allocator can tell this deliberately-freed + * page from a live one. The page is DEAD (unlinked) + * and unreachable, and is not yet in the FSM, so no + * concurrent backend can allocate or resurrect it + * between the release and the stamp. + */ + UnlockReleaseBuffer(buf); + tp_record_free_index_page(indexrel, blk); reclaimed_pages++; + continue; } } diff --git a/src/constants.h b/src/constants.h index 5ecb80ca9..0fecab685 100644 --- a/src/constants.h +++ b/src/constants.h @@ -20,6 +20,10 @@ 0x5450544F /* "TPTO" - Tapir Tombstone: parks displaced segment pages \ * for deferred, standby-safe FSM reclaim (issue #380) */ #define TP_TOMBSTONE_VERSION 1 +#define TP_FREE_PAGE_MAGIC \ + 0x54504650 /* "TPFP" - Tapir Free Page: a page deliberately returned \ + * to the index FSM and safe to recycle (issues #426, #427) \ + */ /* * Page format versions - bump when on-disk format changes. diff --git a/src/debug/dump.c b/src/debug/dump.c index 82f80169a..74606f87b 100644 --- a/src/debug/dump.c +++ b/src/debug/dump.c @@ -7,11 +7,14 @@ #include #include +#include #include #include #include #include #include +#include +#include #include #include #include @@ -829,6 +832,126 @@ tp_pending_free_pages(PG_FUNCTION_ARGS) PG_RETURN_INT64((int64)count); } +/* + * bm25_test_recycle_tombstone_head(index_name) -> block number + * + * INTERNAL-ONLY test scaffold (issues #426, #427). Returns the + * still-live head tombstone page's block to the index FSM, then + * rebuilds the FSM upper levels so GetFreeIndexPage() can hand the + * block back out. This reproduces — without an actual crash — the + * stale-FSM / non-atomic-claim hazard that lets an allocator pick up + * a block that is still referenced by the deferred-free tombstone + * chain (or, symmetrically, the on-disk memtable chain). + * + * Superuser-only; signature and existence are subject to change or + * removal in ANY release without notice. Not part of the public API. + */ +PG_FUNCTION_INFO_V1(tp_test_recycle_tombstone_head); + +Datum +tp_test_recycle_tombstone_head(PG_FUNCTION_ARGS) +{ + text *index_name_text = PG_GETARG_TEXT_PP(0); + char *index_name = text_to_cstring(index_name_text); + Oid index_oid; + Relation index_rel; + BlockNumber head; + + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to recycle tombstone head"))); + + index_oid = tp_resolve_index_name_shared(index_name); + if (!OidIsValid(index_oid)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("index \"%s\" not found", index_name))); + + index_rel = index_open(index_oid, RowExclusiveLock); + + head = tp_tombstone_read_head(index_rel); + if (head == InvalidBlockNumber) + { + index_close(index_rel, RowExclusiveLock); + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("index \"%s\" has no tombstone chain to recycle", + index_name))); + } + + RecordFreeIndexPage(index_rel, head); + IndexFreeSpaceMapVacuum(index_rel); + + index_close(index_rel, RowExclusiveLock); + + PG_RETURN_INT64((int64)head); +} + +/* + * bm25_test_corrupt_tombstone_head(index_name) -> block number + * + * INTERNAL-ONLY test scaffold (issue #427). Overwrites the head + * tombstone page's magic (WAL-logged) so it no longer validates as a + * tombstone page, simulating a chain node that a page-reuse bug + * already clobbered in production. Used to exercise the drain's + * self-healing recovery path on an already-corrupt chain. + * + * Superuser-only; signature and existence are subject to change or + * removal in ANY release without notice. Not part of the public API. + */ +PG_FUNCTION_INFO_V1(tp_test_corrupt_tombstone_head); + +Datum +tp_test_corrupt_tombstone_head(PG_FUNCTION_ARGS) +{ + text *index_name_text = PG_GETARG_TEXT_PP(0); + char *index_name = text_to_cstring(index_name_text); + Oid index_oid; + Relation index_rel; + BlockNumber head; + Buffer buf; + Page page; + GenericXLogState *state; + TpTombstonePage t; + + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to corrupt tombstone head"))); + + index_oid = tp_resolve_index_name_shared(index_name); + if (!OidIsValid(index_oid)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("index \"%s\" not found", index_name))); + + index_rel = index_open(index_oid, RowExclusiveLock); + + head = tp_tombstone_read_head(index_rel); + if (head == InvalidBlockNumber) + { + index_close(index_rel, RowExclusiveLock); + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("index \"%s\" has no tombstone chain to corrupt", + index_name))); + } + + buf = ReadBuffer(index_rel, head); + LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); + state = GenericXLogStart(index_rel); + page = GenericXLogRegisterBuffer(state, buf, GENERIC_XLOG_FULL_IMAGE); + t = tp_tombstone_page(page); + t->magic = 0; /* no longer a valid tombstone page */ + GenericXLogFinish(state); + UnlockReleaseBuffer(buf); + + index_close(index_rel, RowExclusiveLock); + + PG_RETURN_INT64((int64)head); +} + /* * Page visualization support - only available in debug builds. * These functions write to arbitrary file paths, so they are gated diff --git a/src/index/freepage.c b/src/index/freepage.c new file mode 100644 index 000000000..2cc6f6348 --- /dev/null +++ b/src/index/freepage.c @@ -0,0 +1,149 @@ +/* + * Copyright (c) 2025-2026 Tiger Data, Inc. + * Licensed under the PostgreSQL License. See LICENSE for details. + * + * freepage.c - Recyclable free-page stamping for safe FSM reuse. + * + * See freepage.h for the rationale (issues #380, #426, #427). + */ +#include + +#include +#include +#include +#include + +#include "constants.h" +#include "index/freepage.h" + +bool +tp_page_is_recyclable(Page page) +{ + TpFreePageData *f = (TpFreePageData *)PageGetContents(page); + + return f->magic == TP_FREE_PAGE_MAGIC; +} + +void +tp_record_free_index_page(Relation index, BlockNumber blk) +{ + Buffer buf; + Page page; + PageHeader ph; + GenericXLogState *state; + TpFreePageData *f; + + if (blk == TP_METAPAGE_BLKNO) + elog(ERROR, "pg_textsearch: refusing to free metapage (block 0)"); + + buf = ReadBuffer(index, blk); + LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); + + state = GenericXLogStart(index); + page = GenericXLogRegisterBuffer(state, buf, 0); + + f = (TpFreePageData *)PageGetContents(page); + f->magic = TP_FREE_PAGE_MAGIC; + f->flags = 0; + f->freed_fxid = ReadNextFullTransactionId(); + + /* + * Collapse the GenericXLog page hole so the stamp above lands in + * the WAL-logged lower region: computeDelta() diffs only + * [0, pd_lower) and [pd_upper, BLCKSZ), ignoring the hole in + * between. Setting pd_lower = pd_upper = pd_special = BLCKSZ + * records the whole page while leaving the body bytes untouched, + * so the delta is just the header + stamp (a few dozen bytes) even + * though a merge/vacuum may free many pages. Same page-hole + * convention as tp_tombstone_page_init. + */ + ph = (PageHeader)page; + ph->pd_lower = BLCKSZ; + ph->pd_upper = BLCKSZ; + ph->pd_special = BLCKSZ; + + GenericXLogFinish(state); + UnlockReleaseBuffer(buf); + + RecordFreeIndexPage(index, blk); +} + +Buffer +tp_fsm_claim_free_buffer(Relation index) +{ + for (;;) + { + BlockNumber blk; + Buffer buf; + Page page; + + CHECK_FOR_INTERRUPTS(); + + blk = GetFreeIndexPage(index); + if (blk == InvalidBlockNumber) + return InvalidBuffer; /* FSM empty: caller extends */ + + /* + * Drop obviously bogus FSM entries. GetFreeIndexPage() has + * already marked `blk` used, so simply skipping it removes the + * entry from circulation (no infinite loop, no re-offer). + */ + if (blk == TP_METAPAGE_BLKNO || + blk >= RelationGetNumberOfBlocks(index)) + continue; + + buf = ReadBuffer(index, blk); + LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); + page = BufferGetPage(buf); + + if (tp_page_is_recyclable(page)) + return buf; /* caller reinitializes + WAL-logs under the lock */ + + /* + * The FSM offered a block that is NOT a deliberately-freed + * page — a stale (crash) or double-allocated (non-atomic + * GetFreeIndexPage) entry that still points at a live + * structure page. Reusing it would corrupt that structure + * (issues #426, #427). GetFreeIndexPage already cleared the + * FSM slot, so release the page and try the next candidate. + */ + UnlockReleaseBuffer(buf); + } +} + +BlockNumber +tp_fsm_claim_free_block(Relation index) +{ + Buffer buf = tp_fsm_claim_free_buffer(index); + BlockNumber blk; + GenericXLogState *state; + Page page; + TpFreePageData *f; + + if (!BufferIsValid(buf)) + return InvalidBlockNumber; + + blk = BufferGetBlockNumber(buf); + + /* + * This variant releases the buffer lock before returning the block — + * the caller reinitializes the page later, under a fresh lock. To + * keep the claim atomic against a concurrent allocator that the + * non-atomic GetFreeIndexPage() handed the same block, clear the free + * stamp under the lock now (WAL-logged). That concurrent allocator + * blocks on this buffer lock, then observes a page that is no longer + * recyclable and skips it, so it cannot double-allocate the block. + * A crash between here and the caller's reinitialization only leaks + * the block (reclaimed by REINDEX); it is out of the FSM and linked + * into no structure. pd_lower is already BLCKSZ from the stamp, so + * the cleared magic lands in GenericXLog's logged region. + */ + state = GenericXLogStart(index); + page = GenericXLogRegisterBuffer(state, buf, 0); + f = (TpFreePageData *)PageGetContents(page); + f->magic = 0; + GenericXLogFinish(state); + UnlockReleaseBuffer(buf); + + return blk; +} diff --git a/src/index/freepage.h b/src/index/freepage.h new file mode 100644 index 000000000..d1914d7f9 --- /dev/null +++ b/src/index/freepage.h @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2025-2026 Tiger Data, Inc. + * Licensed under the PostgreSQL License. See LICENSE for details. + * + * freepage.h - Recyclable free-page stamping for FSM reuse. + * + * The index FSM (free space map) 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 on-disk memtable chain (issue #426), + * the deferred-free tombstone chain (issue #380 / #427), a segment, + * or a page index. Blindly reinitializing such a block corrupts the + * structure that still owns it. + * + * To make FSM reuse safe, every page returned to the FSM is first + * WAL-stamped with TP_FREE_PAGE_MAGIC (tp_record_free_index_page). + * Allocators then reuse a block ONLY when it still carries that stamp + * (tp_fsm_claim_free_buffer / tp_fsm_claim_free_block). A block the + * FSM offers that is NOT stamped (a stale or double-allocated entry + * pointing at a live page) is skipped rather than overwritten — the + * same recyclability discipline PostgreSQL's B-tree uses via + * _bt_page_recyclable. + */ +#pragma once + +#include + +#include +#include +#include +#include +#include + +/* + * Free-page stamp, written at PageGetContents() of a page that has + * been deliberately returned to the index FSM. The rest of the page + * body is left intact (a reuse fully reinitializes it), keeping the + * WAL delta tiny. + */ +typedef struct TpFreePageData +{ + uint32 magic; /* TP_FREE_PAGE_MAGIC */ + uint32 flags; /* reserved, 0 */ + FullTransactionId freed_fxid; /* xid at free time (diagnostic) */ +} TpFreePageData; + +/* True iff `page` carries the recyclable free-page stamp. */ +extern bool tp_page_is_recyclable(Page page); + +/* + * WAL-stamp `blk` as a recyclable free page, then return it to the + * index FSM. Use in place of a bare RecordFreeIndexPage() so a later + * allocator can distinguish a deliberately-freed page from a live one + * whose block the FSM offered by mistake. Takes the buffer's + * EXCLUSIVE lock; the caller must already have removed `blk` from + * every live structure (it is unreferenced once freed). + */ +extern void tp_record_free_index_page(Relation index, BlockNumber blk); + +/* + * Claim a recyclable free page from the FSM. Returns a pinned, + * EXCLUSIVE-locked buffer whose page the caller must reinitialize and + * WAL-log before releasing the lock, or InvalidBuffer when the FSM + * offers no reusable page (the caller then extends the relation). + * + * Holding the buffer lock from the recyclability check through the + * caller's reinitialization makes the claim atomic: a second backend + * handed the same block by a non-atomic GetFreeIndexPage() blocks on + * the lock, then observes the now-live page and skips it. This is the + * path memtable inserts use (they run under the per-index lock only in + * SHARED mode, so they race each other — issue #426). + */ +extern Buffer tp_fsm_claim_free_buffer(Relation index); + +/* + * Block-returning wrapper over tp_fsm_claim_free_buffer for callers + * that reopen the block later under their own lock (the segment + * writer buffers pages and flushes them afterward). Because the lock + * is dropped before the block is returned, this variant clears the + * free stamp under the claim lock so the claim stays atomic even when + * the caller holds the per-index lock only in SHARED mode and races a + * concurrent allocator (e.g. VACUUM's pre-V5 segment rebuild vs. a + * memtable insert). Returns InvalidBlockNumber when the FSM offers no + * reusable page. + */ +extern BlockNumber tp_fsm_claim_free_block(Relation index); diff --git a/src/memtable/log.c b/src/memtable/log.c index af3f53d1c..96afae813 100644 --- a/src/memtable/log.c +++ b/src/memtable/log.c @@ -35,7 +35,6 @@ #include #include #include -#include #include #include #include @@ -45,6 +44,7 @@ #include #include "constants.h" +#include "index/freepage.h" #include "index/metapage.h" #include "index/resolve.h" #include "index/state.h" @@ -73,32 +73,27 @@ memtable_read_tail_blkno(Relation rel) } /* - * Allocate a memtable chain page: try the index FSM first, else + * Allocate a memtable chain page: reuse a recyclable FSM page, else * extend the relation. Returns a pinned buffer with * BUFFER_LOCK_EXCLUSIVE (same as ExtendBufferedRel EB_LOCK_FIRST). * Caller must UnlockReleaseBuffer when done. + * + * tp_fsm_claim_free_buffer holds the returned buffer's lock from the + * recyclability check onward, so the caller's page_init (under that + * same lock) atomically claims the block: a concurrent insert handed + * the same block by the non-atomic FSM blocks on the lock, then sees + * the now-live memtable page and skips it (issue #426). It also + * refuses any block the FSM offers that is still a live structure + * page, so a stale/crash FSM entry can't clobber the tombstone chain + * or another memtable page (issues #380, #427). */ static Buffer tp_memtable_alloc_page(Relation rel) { - BlockNumber block; - - block = GetFreeIndexPage(rel); - if (BlockNumberIsValid(block)) - { - if (block == TP_METAPAGE_BLKNO) - elog(ERROR, "pg_textsearch: FSM returned metapage for memtable"); + Buffer buf = tp_fsm_claim_free_buffer(rel); - if (block >= RelationGetNumberOfBlocks(rel)) - elog(ERROR, - "pg_textsearch: FSM returned block %u beyond relation " - "size for index \"%s\"", - block, - RelationGetRelationName(rel)); - - return ReadBufferExtended( - rel, MAIN_FORKNUM, block, RBM_ZERO_AND_LOCK, NULL); - } + if (BufferIsValid(buf)) + return buf; return ExtendBufferedRel(BMR_REL(rel), MAIN_FORKNUM, NULL, EB_LOCK_FIRST); } diff --git a/src/segment/segment.c b/src/segment/segment.c index f108bc4a9..ae6ba6ac3 100644 --- a/src/segment/segment.c +++ b/src/segment/segment.c @@ -28,6 +28,7 @@ #include #include "debug/dump.h" +#include "index/freepage.h" #include "index/metapage.h" #include "index/state.h" #include "segment/alive_bitset.h" @@ -763,8 +764,14 @@ allocate_segment_page(Relation index) Buffer buffer; BlockNumber block; - /* Try to get a free page from FSM (recycled from compaction) */ - block = GetFreeIndexPage(index); + /* + * Reuse a recyclable free page from the FSM (recycled from a + * compaction/vacuum drain). tp_fsm_claim_free_block skips any + * block the non-crash-safe FSM offers that is still a live + * structure page, so we never reinitialize a page another + * structure still owns (issues #426, #427). + */ + block = tp_fsm_claim_free_block(index); if (block != InvalidBlockNumber) return block; @@ -1586,7 +1593,7 @@ tp_segment_free_pages(Relation index, BlockNumber *pages, uint32 num_pages) if (pages[i] == 0) elog(ERROR, "attempted to free metapage (block 0)"); - RecordFreeIndexPage(index, pages[i]); + tp_record_free_index_page(index, pages[i]); } } diff --git a/src/segment/tombstone.c b/src/segment/tombstone.c index 147be2d10..aaca8771a 100644 --- a/src/segment/tombstone.c +++ b/src/segment/tombstone.c @@ -9,10 +9,9 @@ #include #include #include -#include -#include #include "constants.h" +#include "index/freepage.h" #include "index/metapage.h" #include "index/state.h" #include "segment/io.h" @@ -53,10 +52,11 @@ tp_tombstone_page_is_valid(Page page) } /* - * Allocate one zero/extend index page via the FSM-or-extend path. - * Mirrors allocate_segment_page() but is local to this module; the - * page is fully overwritten by the GenericXLog image below, so its - * pre-existing contents are irrelevant. + * Allocate one index page for a tombstone page. With use_fsm, reuse + * a recyclable free page from the FSM (skipping any live-structure + * block the non-crash-safe FSM offers); otherwise extend the + * relation. The page is fully overwritten by the GenericXLog image + * below, so its prior contents are irrelevant. */ static BlockNumber tombstone_alloc_page(Relation index, bool use_fsm) @@ -66,7 +66,7 @@ tombstone_alloc_page(Relation index, bool use_fsm) if (use_fsm) { - block = GetFreeIndexPage(index); + block = tp_fsm_claim_free_block(index); if (block != InvalidBlockNumber) return block; } @@ -250,6 +250,9 @@ tp_tombstone_drain( BlockNumber victim_next = InvalidBlockNumber; BlockNumber *victim_blocks = NULL; uint32 victim_count = 0; + bool corrupt = false; + BlockNumber corrupt_at = InvalidBlockNumber; + BlockNumber corrupt_prev = InvalidBlockNumber; CHECK_FOR_INTERRUPTS(); @@ -271,12 +274,10 @@ tp_tombstone_drain( if (!tp_tombstone_page_is_valid(page)) { UnlockReleaseBuffer(buf); - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg("pg_textsearch: corrupt tombstone page %u " - "in index \"%s\"", - cur, - RelationGetRelationName(index)))); + corrupt = true; + corrupt_at = cur; + corrupt_prev = prev; + break; } t = tp_tombstone_page(page); @@ -296,19 +297,20 @@ tp_tombstone_drain( if (b == 0 || b >= nblocks || b == cur) { - UnlockReleaseBuffer(buf); - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg("pg_textsearch: tombstone page %u " - "lists invalid block %u in index " - "\"%s\"", - cur, - b, - RelationGetRelationName(index)))); + corrupt = true; + corrupt_at = cur; + corrupt_prev = prev; + break; } victim_blocks[k] = b; } UnlockReleaseBuffer(buf); + if (corrupt) + { + pfree(victim_blocks); + victim_blocks = NULL; + victim = InvalidBlockNumber; + } break; } @@ -317,6 +319,42 @@ tp_tombstone_drain( UnlockReleaseBuffer(buf); } + /* + * Self-heal an already-corrupt chain instead of wedging every + * writer (issue #427). An older page-reuse bug (or a crash + * with a stale FSM) can leave a chain node that no longer + * validates, or a valid node that lists an impossible block. + * We cannot trust such a node's next_page, so drop it and the + * unverifiable remainder by pointing its predecessor — a + * still-valid tombstone page, or the metapage head when the + * corruption is at the head — at InvalidBlockNumber. The + * still-drainable prefix ahead of the corruption is preserved; + * the dropped tail leaks pages that a REINDEX reclaims. We + * never free the corrupt node's listed blocks, so if the block + * was double-owned by a live structure, healing cannot corrupt + * that structure. + */ + if (corrupt) + { + tombstone_unlink( + index, corrupt_prev, corrupt_at, InvalidBlockNumber); + + if (own_lock) + tp_release_index_lock(state); + + ereport(WARNING, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("pg_textsearch: recovered from corrupt tombstone " + "page %u in index \"%s\"", + corrupt_at, + RelationGetRelationName(index)), + errdetail( + "Dropped the deferred-free chain from that " + "page onward; leaked index pages are " + "reclaimed by REINDEX."))); + break; + } + if (victim == InvalidBlockNumber) { if (own_lock) @@ -335,8 +373,8 @@ tp_tombstone_drain( uint32 k; for (k = 0; k < victim_count; k++) - RecordFreeIndexPage(index, victim_blocks[k]); - RecordFreeIndexPage(index, victim); + tp_record_free_index_page(index, victim_blocks[k]); + tp_record_free_index_page(index, victim); freed += victim_count + 1; } if (victim_blocks) diff --git a/test/expected/tombstone_recover.out b/test/expected/tombstone_recover.out new file mode 100644 index 000000000..6d4e5b6db --- /dev/null +++ b/test/expected/tombstone_recover.out @@ -0,0 +1,78 @@ +-- Self-healing recovery of a corrupt deferred-free tombstone chain +-- (issue #427). +-- +-- Prevention (the allocator refusing to reuse a live page) stops NEW +-- corruption, but an index already corrupted by an older binary stays +-- wedged: every write path triggers a merge/vacuum tombstone drain that +-- raises "corrupt tombstone page ..." and aborts. The drain must +-- instead recover — drop the unverifiable tail of the chain, warn, and +-- let writes proceed — so operators are not forced to REINDEX. +CREATE EXTENSION IF NOT EXISTS pg_textsearch; +SET pg_textsearch.memtable_pages_threshold = 0; +SET pg_textsearch.bulk_load_threshold = 0; +-- Keep the recovery WARNING out of the golden output (it names a +-- block number); the test asserts the post-recovery state instead. +SET client_min_messages = error; +CREATE TABLE recover_docs (id int, body text) + WITH (autovacuum_enabled = false); +INSERT INTO recover_docs +SELECT g, 'alpha beta gamma delta term' || (g % 50) +FROM generate_series(1, 2000) g; +CREATE INDEX recover_idx ON recover_docs + USING bm25 (body) WITH (text_config = 'english'); +INSERT INTO recover_docs +SELECT g, 'alpha beta term' || (g % 50) +FROM generate_series(2001, 4000) g; +SELECT bm25_spill_index('recover_idx') > 0 AS spilled; + spilled +--------- + t +(1 row) + +SELECT bm25_force_merge('recover_idx'); + bm25_force_merge +------------------ + +(1 row) + +SELECT bm25_pending_free_pages('recover_idx') > 0 AS parked; + parked +-------- + t +(1 row) + +-- Simulate an already-corrupt chain node, as an older page-reuse bug +-- would have left behind. +SELECT bm25_test_corrupt_tombstone_head('recover_idx') > 0 AS corrupted; + corrupted +----------- + t +(1 row) + +-- A VACUUM drives the tombstone drain. It must self-heal (drop the +-- corrupt tail with a WARNING) instead of raising and wedging the +-- index for every future write. +VACUUM recover_docs; +-- The chain was reset past the corruption, so the drain no longer +-- errors and reports a clean (empty) pending set. +SELECT bm25_pending_free_pages('recover_idx') AS pending_after_recovery; + pending_after_recovery +------------------------ + 0 +(1 row) + +-- Writes and queries proceed normally after recovery. +INSERT INTO recover_docs VALUES (999001, 'alpha beta gamma probe'); +SELECT count(*) > 0 AS has_hits +FROM ( + SELECT 1 FROM recover_docs + ORDER BY body <@> to_bm25query('probe', 'recover_idx') + LIMIT 10 +) s; + has_hits +---------- + t +(1 row) + +DROP TABLE recover_docs; +DROP EXTENSION pg_textsearch CASCADE; diff --git a/test/expected/tombstone_reuse.out b/test/expected/tombstone_reuse.out new file mode 100644 index 000000000..32dbdd6b2 --- /dev/null +++ b/test/expected/tombstone_reuse.out @@ -0,0 +1,114 @@ +-- Recycled-page corruption of the deferred-free tombstone chain +-- (issues #426, #427). +-- +-- The deferred-free tombstone chain (metap.pending_free_head) parks +-- displaced segment pages until a later VACUUM returns them to the FSM. +-- The chain links are WAL-logged, but the index FSM that guards those +-- pages is NOT crash-safe and its GetFreeIndexPage() claim is not +-- atomic. Either can offer a block that is still referenced by the +-- tombstone chain. If an allocator then reuses that block, it +-- overwrites a live tombstone page, and the next tombstone drain +-- fails with "corrupt tombstone page ..." — permanently wedging every +-- write path for the index (issue #427). +-- +-- This test reproduces that hazard deterministically with an +-- internal-only scaffold that returns the live head tombstone page to +-- the FSM, then drives a normal allocation. A correct allocator must +-- refuse to reuse a block that is still a live pg_textsearch structure +-- page. +-- +-- autovacuum + auto-spill are disabled so the only allocations and +-- drains are the explicit ones below, keeping FSM state deterministic. +CREATE EXTENSION IF NOT EXISTS pg_textsearch; +SET pg_textsearch.memtable_pages_threshold = 0; +SET pg_textsearch.bulk_load_threshold = 0; +CREATE TABLE reuse_docs (id int, body text) + WITH (autovacuum_enabled = false); +INSERT INTO reuse_docs +SELECT g, 'alpha beta gamma delta term' || (g % 50) +FROM generate_series(1, 2000) g; +CREATE INDEX reuse_idx ON reuse_docs + USING bm25 (body) WITH (text_config = 'english'); +NOTICE: BM25 index build started for relation reuse_idx +NOTICE: Using text search configuration: english +NOTICE: Using index options: k1=1.20, b=0.75 +NOTICE: BM25 index build completed: 2000 documents, avg_length=5.00 +-- Build wrote a segment directly, so the memtable is empty here; the +-- next batch fills it so the following spill produces a second segment. +INSERT INTO reuse_docs +SELECT g, 'alpha beta term' || (g % 50) +FROM generate_series(2001, 4000) g; +SELECT bm25_spill_index('reuse_idx') > 0 AS spilled; + spilled +--------- + t +(1 row) + +-- Merge the L0 segments into one L1 segment; this displaces the source +-- segments' pages, which are parked in the tombstone chain (not freed). +SELECT bm25_force_merge('reuse_idx'); + bm25_force_merge +------------------ + +(1 row) + +SELECT bm25_pending_free_pages('reuse_idx') > 0 AS parked_after_merge; + parked_after_merge +-------------------- + t +(1 row) + +-- Drain any incidental FSM free pages left by build/merge by allocating +-- a fresh segment, so the block recycled below is the single +-- deterministic allocation target. Parked pages are not in the FSM, so +-- the tombstone chain is untouched by this step. +INSERT INTO reuse_docs +SELECT g, 'alpha beta term' || (g % 50) +FROM generate_series(4001, 6000) g; +SELECT bm25_spill_index('reuse_idx') > 0 AS drain_spilled; + drain_spilled +--------------- + t +(1 row) + +SELECT bm25_pending_free_pages('reuse_idx') > 0 AS still_parked; + still_parked +-------------- + t +(1 row) + +-- Hand the still-live head tombstone page back to the (now empty) FSM. +SELECT bm25_test_recycle_tombstone_head('reuse_idx') > 0 AS recycled; + recycled +---------- + t +(1 row) + +-- The memtable is empty (last op was a spill), so this single insert +-- bootstraps a fresh memtable page and allocates it from the FSM — +-- picking up the recycled block. A correct allocator must NOT reuse +-- that block, because it is still a live tombstone page. +INSERT INTO reuse_docs VALUES (999001, 'alpha beta gamma probe'); +-- Walking the tombstone chain must still succeed. With a broken +-- allocator the insert above overwrote the live tombstone page, so +-- this raises: ERROR: pg_textsearch: corrupt tombstone page ... +SELECT bm25_pending_free_pages('reuse_idx') >= 0 AS chain_intact; + chain_intact +-------------- + t +(1 row) + +-- Sanity: the index still answers queries and the probe row is found. +SELECT count(*) > 0 AS has_hits +FROM ( + SELECT 1 FROM reuse_docs + ORDER BY body <@> to_bm25query('probe', 'reuse_idx') + LIMIT 10 +) s; + has_hits +---------- + t +(1 row) + +DROP TABLE reuse_docs; +DROP EXTENSION pg_textsearch CASCADE; diff --git a/test/sql/tombstone_recover.sql b/test/sql/tombstone_recover.sql new file mode 100644 index 000000000..3bf7bee6d --- /dev/null +++ b/test/sql/tombstone_recover.sql @@ -0,0 +1,55 @@ +-- Self-healing recovery of a corrupt deferred-free tombstone chain +-- (issue #427). +-- +-- Prevention (the allocator refusing to reuse a live page) stops NEW +-- corruption, but an index already corrupted by an older binary stays +-- wedged: every write path triggers a merge/vacuum tombstone drain that +-- raises "corrupt tombstone page ..." and aborts. The drain must +-- instead recover — drop the unverifiable tail of the chain, warn, and +-- let writes proceed — so operators are not forced to REINDEX. +CREATE EXTENSION IF NOT EXISTS pg_textsearch; +SET pg_textsearch.memtable_pages_threshold = 0; +SET pg_textsearch.bulk_load_threshold = 0; +-- Keep the recovery WARNING out of the golden output (it names a +-- block number); the test asserts the post-recovery state instead. +SET client_min_messages = error; + +CREATE TABLE recover_docs (id int, body text) + WITH (autovacuum_enabled = false); +INSERT INTO recover_docs +SELECT g, 'alpha beta gamma delta term' || (g % 50) +FROM generate_series(1, 2000) g; +CREATE INDEX recover_idx ON recover_docs + USING bm25 (body) WITH (text_config = 'english'); + +INSERT INTO recover_docs +SELECT g, 'alpha beta term' || (g % 50) +FROM generate_series(2001, 4000) g; +SELECT bm25_spill_index('recover_idx') > 0 AS spilled; +SELECT bm25_force_merge('recover_idx'); +SELECT bm25_pending_free_pages('recover_idx') > 0 AS parked; + +-- Simulate an already-corrupt chain node, as an older page-reuse bug +-- would have left behind. +SELECT bm25_test_corrupt_tombstone_head('recover_idx') > 0 AS corrupted; + +-- A VACUUM drives the tombstone drain. It must self-heal (drop the +-- corrupt tail with a WARNING) instead of raising and wedging the +-- index for every future write. +VACUUM recover_docs; + +-- The chain was reset past the corruption, so the drain no longer +-- errors and reports a clean (empty) pending set. +SELECT bm25_pending_free_pages('recover_idx') AS pending_after_recovery; + +-- Writes and queries proceed normally after recovery. +INSERT INTO recover_docs VALUES (999001, 'alpha beta gamma probe'); +SELECT count(*) > 0 AS has_hits +FROM ( + SELECT 1 FROM recover_docs + ORDER BY body <@> to_bm25query('probe', 'recover_idx') + LIMIT 10 +) s; + +DROP TABLE recover_docs; +DROP EXTENSION pg_textsearch CASCADE; diff --git a/test/sql/tombstone_reuse.sql b/test/sql/tombstone_reuse.sql new file mode 100644 index 000000000..2c70e5701 --- /dev/null +++ b/test/sql/tombstone_reuse.sql @@ -0,0 +1,80 @@ +-- Recycled-page corruption of the deferred-free tombstone chain +-- (issues #426, #427). +-- +-- The deferred-free tombstone chain (metap.pending_free_head) parks +-- displaced segment pages until a later VACUUM returns them to the FSM. +-- The chain links are WAL-logged, but the index FSM that guards those +-- pages is NOT crash-safe and its GetFreeIndexPage() claim is not +-- atomic. Either can offer a block that is still referenced by the +-- tombstone chain. If an allocator then reuses that block, it +-- overwrites a live tombstone page, and the next tombstone drain +-- fails with "corrupt tombstone page ..." — permanently wedging every +-- write path for the index (issue #427). +-- +-- This test reproduces that hazard deterministically with an +-- internal-only scaffold that returns the live head tombstone page to +-- the FSM, then drives a normal allocation. A correct allocator must +-- refuse to reuse a block that is still a live pg_textsearch structure +-- page. +-- +-- autovacuum + auto-spill are disabled so the only allocations and +-- drains are the explicit ones below, keeping FSM state deterministic. +CREATE EXTENSION IF NOT EXISTS pg_textsearch; +SET pg_textsearch.memtable_pages_threshold = 0; +SET pg_textsearch.bulk_load_threshold = 0; + +CREATE TABLE reuse_docs (id int, body text) + WITH (autovacuum_enabled = false); +INSERT INTO reuse_docs +SELECT g, 'alpha beta gamma delta term' || (g % 50) +FROM generate_series(1, 2000) g; + +CREATE INDEX reuse_idx ON reuse_docs + USING bm25 (body) WITH (text_config = 'english'); + +-- Build wrote a segment directly, so the memtable is empty here; the +-- next batch fills it so the following spill produces a second segment. +INSERT INTO reuse_docs +SELECT g, 'alpha beta term' || (g % 50) +FROM generate_series(2001, 4000) g; +SELECT bm25_spill_index('reuse_idx') > 0 AS spilled; + +-- Merge the L0 segments into one L1 segment; this displaces the source +-- segments' pages, which are parked in the tombstone chain (not freed). +SELECT bm25_force_merge('reuse_idx'); +SELECT bm25_pending_free_pages('reuse_idx') > 0 AS parked_after_merge; + +-- Drain any incidental FSM free pages left by build/merge by allocating +-- a fresh segment, so the block recycled below is the single +-- deterministic allocation target. Parked pages are not in the FSM, so +-- the tombstone chain is untouched by this step. +INSERT INTO reuse_docs +SELECT g, 'alpha beta term' || (g % 50) +FROM generate_series(4001, 6000) g; +SELECT bm25_spill_index('reuse_idx') > 0 AS drain_spilled; +SELECT bm25_pending_free_pages('reuse_idx') > 0 AS still_parked; + +-- Hand the still-live head tombstone page back to the (now empty) FSM. +SELECT bm25_test_recycle_tombstone_head('reuse_idx') > 0 AS recycled; + +-- The memtable is empty (last op was a spill), so this single insert +-- bootstraps a fresh memtable page and allocates it from the FSM — +-- picking up the recycled block. A correct allocator must NOT reuse +-- that block, because it is still a live tombstone page. +INSERT INTO reuse_docs VALUES (999001, 'alpha beta gamma probe'); + +-- Walking the tombstone chain must still succeed. With a broken +-- allocator the insert above overwrote the live tombstone page, so +-- this raises: ERROR: pg_textsearch: corrupt tombstone page ... +SELECT bm25_pending_free_pages('reuse_idx') >= 0 AS chain_intact; + +-- Sanity: the index still answers queries and the probe row is found. +SELECT count(*) > 0 AS has_hits +FROM ( + SELECT 1 FROM reuse_docs + ORDER BY body <@> to_bm25query('probe', 'reuse_idx') + LIMIT 10 +) s; + +DROP TABLE reuse_docs; +DROP EXTENSION pg_textsearch CASCADE;