From 76ef56d01483308c635915f8b43e67741876225c Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Wed, 28 May 2025 16:45:59 -0400 Subject: [PATCH v8 09/22] Find and fix VM corruption in heap_page_prune_and_freeze Future commits will update the VM in the same critical section and WAL record as pruning and freezing. For ease of review, this commit makes one step toward doing this. It moves the VM corruption handling case to heap_page_prune_and_freeze(). --- src/backend/access/heap/pruneheap.c | 87 +++++++++++++++++++++++++++- src/backend/access/heap/vacuumlazy.c | 77 +++--------------------- src/include/access/heapam.h | 4 ++ 3 files changed, 96 insertions(+), 72 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 956caeb69dc..72216126945 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -21,7 +21,7 @@ #include "access/transam.h" #include "access/xlog.h" #include "access/xloginsert.h" -#include "access/visibilitymapdefs.h" +#include "access/visibilitymap.h" #include "commands/vacuum.h" #include "executor/instrument.h" #include "miscadmin.h" @@ -177,6 +177,13 @@ static void heap_prune_record_unchanged_lp_redirect(PruneState *prstate, OffsetN static void page_verify_redirects(Page page); +static bool identify_and_fix_vm_corruption(Relation relation, + BlockNumber heap_blk, + Buffer heap_buffer, Page heap_page, + bool heap_blk_known_av, + int64 nlpdead_items, + Buffer vmbuffer); + /* * Optionally prune and repair fragmentation in the specified page. @@ -261,7 +268,9 @@ heap_page_prune_opt(Relation relation, Buffer buffer) * not the relation has indexes, since we cannot safely determine * that during on-access pruning with the current implementation. */ - heap_page_prune_and_freeze(relation, buffer, vistest, 0, + heap_page_prune_and_freeze(relation, buffer, false, + InvalidBuffer, + vistest, 0, NULL, &presult, PRUNE_ON_ACCESS, &dummy_off_loc, NULL, NULL); /* @@ -294,6 +303,64 @@ heap_page_prune_opt(Relation relation, Buffer buffer) } } +/* + * When updating the visibility map after phase I heap vacuuming, we take the + * opportunity to identify and fix any VM corruption. + * + * heap_blk_known_av is the visibility status of the heap page collected + * while finding the next unskippable block in heap_vac_scan_next_block(). + */ +static bool +identify_and_fix_vm_corruption(Relation relation, + BlockNumber heap_blk, + Buffer heap_buffer, Page heap_page, + bool heap_blk_known_av, + int64 nlpdead_items, + Buffer vmbuffer) +{ + /* + * As of PostgreSQL 9.2, the visibility map bit should never be set if the + * page-level bit is clear. However, it's possible that the bit got + * cleared after heap_vac_scan_next_block() was called, so we must recheck + * with buffer lock before concluding that the VM is corrupt. + */ + if (heap_blk_known_av && !PageIsAllVisible(heap_page) && + visibilitymap_get_status(relation, heap_blk, &vmbuffer) != 0) + { + elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u", + RelationGetRelationName(relation), heap_blk); + visibilitymap_clear(relation, heap_blk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + return true; + } + + /* + * It's possible for the value returned by + * GetOldestNonRemovableTransactionId() to move backwards, so it's not + * wrong for us to see tuples that appear to not be visible to everyone + * yet, while PD_ALL_VISIBLE is already set. The real safe xmin value + * never moves backwards, but GetOldestNonRemovableTransactionId() is + * conservative and sometimes returns a value that's unnecessarily small, + * so if we see that contradiction it just means that the tuples that we + * think are not visible to everyone yet actually are, and the + * PD_ALL_VISIBLE flag is correct. + * + * There should never be LP_DEAD items on a page with PD_ALL_VISIBLE set, + * however. + */ + if (nlpdead_items > 0 && PageIsAllVisible(heap_page)) + { + elog(WARNING, "page containing LP_DEAD items is marked as all-visible in relation \"%s\" page %u", + RelationGetRelationName(relation), heap_blk); + PageClearAllVisible(heap_page); + MarkBufferDirty(heap_buffer); + visibilitymap_clear(relation, heap_blk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + return true; + } + + return false; +} /* * Prune and repair fragmentation and potentially freeze tuples on the @@ -314,6 +381,10 @@ heap_page_prune_opt(Relation relation, Buffer buffer) * HEAP_PRUNE_FREEZE option is not set, because at the moment only callers * that also freeze need that information. * + * blk_known_av is the visibility status of the heap block as of the last call + * to find_next_unskippable_block(). vmbuffer is the buffer that may already + * contain the required block of the visibility map. + * * vistest is used to distinguish whether tuples are DEAD or RECENTLY_DEAD * (see heap_prune_satisfies_vacuum). * @@ -349,6 +420,8 @@ heap_page_prune_opt(Relation relation, Buffer buffer) */ void heap_page_prune_and_freeze(Relation relation, Buffer buffer, + bool blk_known_av, + Buffer vmbuffer, GlobalVisState *vistest, int options, struct VacuumCutoffs *cutoffs, @@ -897,6 +970,16 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, presult->lpdead_items = prstate.lpdead_items; /* the presult->deadoffsets array was already filled in */ + /* + * Clear any VM corruption. This does not need to be done in a critical + * section. + */ + presult->vm_corruption = false; + if (options & HEAP_PAGE_PRUNE_UPDATE_VM) + presult->vm_corruption = identify_and_fix_vm_corruption(relation, + blockno, buffer, page, + blk_known_av, + prstate.lpdead_items, vmbuffer); if (prstate.freeze) { if (presult->nfrozen > 0) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 4d47a6b394a..64ae63dcb12 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -430,12 +430,6 @@ static void find_next_unskippable_block(LVRelState *vacrel, bool *skipsallvis); static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, bool sharelock, Buffer vmbuffer); -static bool identify_and_fix_vm_corruption(Relation relation, - BlockNumber heap_blk, - Buffer heap_buffer, Page heap_page, - bool heap_blk_known_av, - int64 nlpdead_items, - Buffer vmbuffer); static int lazy_scan_prune(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, Buffer vmbuffer, bool all_visible_according_to_vm, @@ -1938,65 +1932,6 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, return false; } -/* - * When updating the visibility map after phase I heap vacuuming, we take the - * opportunity to identify and fix any VM corruption. - * - * heap_blk_known_av is the visibility status of the heap page collected - * while finding the next unskippable block in heap_vac_scan_next_block(). - */ -static bool -identify_and_fix_vm_corruption(Relation relation, - BlockNumber heap_blk, - Buffer heap_buffer, Page heap_page, - bool heap_blk_known_av, - int64 nlpdead_items, - Buffer vmbuffer) -{ - /* - * As of PostgreSQL 9.2, the visibility map bit should never be set if the - * page-level bit is clear. However, it's possible that the bit got - * cleared after heap_vac_scan_next_block() was called, so we must recheck - * with buffer lock before concluding that the VM is corrupt. - */ - if (heap_blk_known_av && !PageIsAllVisible(heap_page) && - visibilitymap_get_status(relation, heap_blk, &vmbuffer) != 0) - { - elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u", - RelationGetRelationName(relation), heap_blk); - visibilitymap_clear(relation, heap_blk, vmbuffer, - VISIBILITYMAP_VALID_BITS); - return true; - } - - /* - * It's possible for the value returned by - * GetOldestNonRemovableTransactionId() to move backwards, so it's not - * wrong for us to see tuples that appear to not be visible to everyone - * yet, while PD_ALL_VISIBLE is already set. The real safe xmin value - * never moves backwards, but GetOldestNonRemovableTransactionId() is - * conservative and sometimes returns a value that's unnecessarily small, - * so if we see that contradiction it just means that the tuples that we - * think are not visible to everyone yet actually are, and the - * PD_ALL_VISIBLE flag is correct. - * - * There should never be LP_DEAD items on a page with PD_ALL_VISIBLE set, - * however. - */ - if (nlpdead_items > 0 && PageIsAllVisible(heap_page)) - { - elog(WARNING, "page containing LP_DEAD items is marked as all-visible in relation \"%s\" page %u", - RelationGetRelationName(relation), heap_blk); - PageClearAllVisible(heap_page); - MarkBufferDirty(heap_buffer); - visibilitymap_clear(relation, heap_blk, vmbuffer, - VISIBILITYMAP_VALID_BITS); - return true; - } - - return false; -} - /* qsort comparator for sorting OffsetNumbers */ static int @@ -2055,11 +1990,14 @@ lazy_scan_prune(LVRelState *vacrel, * tuples. Pruning will have determined whether or not the page is * all-visible. */ - prune_options = HEAP_PAGE_PRUNE_FREEZE; + prune_options = HEAP_PAGE_PRUNE_FREEZE | HEAP_PAGE_PRUNE_UPDATE_VM; if (vacrel->nindexes == 0) prune_options |= HEAP_PAGE_PRUNE_MARK_UNUSED_NOW; - heap_page_prune_and_freeze(rel, buf, vacrel->vistest, prune_options, + heap_page_prune_and_freeze(rel, buf, + all_visible_according_to_vm, + vmbuffer, + vacrel->vistest, prune_options, &vacrel->cutoffs, &presult, PRUNE_VACUUM_SCAN, &vacrel->offnum, &vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid); @@ -2144,10 +2082,9 @@ lazy_scan_prune(LVRelState *vacrel, /* * Handle setting visibility map bit based on information from the VM (as * of last heap_vac_scan_next_block() call), and from all_visible and - * all_frozen variables. Start by looking for any VM corruption. + * all_frozen variables. */ - if (identify_and_fix_vm_corruption(vacrel->rel, blkno, buf, page, - all_visible_according_to_vm, presult.lpdead_items, vmbuffer)) + if (presult.vm_corruption) { /* Don't update the VM if we just cleared corruption in it */ } diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index e7129a644a1..0c7eb5e46f4 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -42,6 +42,7 @@ /* "options" flag bits for heap_page_prune_and_freeze */ #define HEAP_PAGE_PRUNE_MARK_UNUSED_NOW (1 << 0) #define HEAP_PAGE_PRUNE_FREEZE (1 << 1) +#define HEAP_PAGE_PRUNE_UPDATE_VM (1 << 2) typedef struct BulkInsertStateData *BulkInsertState; struct TupleTableSlot; @@ -247,6 +248,7 @@ typedef struct PruneFreezeResult bool all_visible; bool all_frozen; TransactionId vm_conflict_horizon; + bool vm_corruption; /* * Whether or not the page makes rel truncation unsafe. This is set to @@ -380,6 +382,8 @@ extern TransactionId heap_index_delete_tuples(Relation rel, struct GlobalVisState; extern void heap_page_prune_opt(Relation relation, Buffer buffer); extern void heap_page_prune_and_freeze(Relation relation, Buffer buffer, + bool blk_known_av, + Buffer vmbuffer, struct GlobalVisState *vistest, int options, struct VacuumCutoffs *cutoffs, -- 2.43.0