From e591bf061ee673f3750d1180673e1ab48be43bb8 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 27 Jan 2026 16:53:11 -0500 Subject: [PATCH v34 03/14] Move VM assert into prune/freeze code and simplify returned values After pruning and freezing, we do an assert-only validatation that the page's visibility status matches what we found during the pruning and freezing pass over the page. There's no reason to wait until lazy_scan_prune() to do this validation, as all of the VM setting logic has already been moved to heap_page_prune_and_freeze(). Doing so also allows us to remove some fields of PruneFreezeResult, narrowing the scope of values the caller has to think about. Author: Melanie Plageman Reviewed-by: Andres Freund Reviewed-by: Chao Li Discussion: https://postgr.es/m/7ib3sa55sapwjlaz4sijbiq7iezna27kjvvvar4dpgkmadml6t%40gfpkkwmdnepx --- src/backend/access/heap/pruneheap.c | 65 +++++++++++++++++++--------- src/backend/access/heap/vacuumlazy.c | 35 +-------------- src/include/access/heapam.h | 26 ++++------- 3 files changed, 54 insertions(+), 72 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 014c3c92d6c..192df9a2218 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -991,6 +991,7 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, Buffer vmbuffer = params->vmbuffer; Page page = BufferGetPage(buffer); BlockNumber blockno = BufferGetBlockNumber(buffer); + TransactionId vm_conflict_horizon = InvalidTransactionId; PruneState prstate; bool do_freeze; bool do_prune; @@ -1149,23 +1150,8 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, presult->nfrozen = prstate.nfrozen; presult->live_tuples = prstate.live_tuples; presult->recently_dead_tuples = prstate.recently_dead_tuples; - presult->all_visible = prstate.all_visible; - presult->all_frozen = prstate.all_frozen; presult->hastup = prstate.hastup; - /* - * For callers planning to update the visibility map, the conflict horizon - * for that record must be the newest xmin on the page. However, if the - * page is completely frozen, there can be no conflict and the - * vm_conflict_horizon should remain InvalidTransactionId. This includes - * the case that we just froze all the tuples; the prune-freeze record - * included the conflict XID already so the caller doesn't need it. - */ - if (presult->all_frozen) - presult->vm_conflict_horizon = InvalidTransactionId; - else - presult->vm_conflict_horizon = prstate.visibility_cutoff_xid; - presult->lpdead_items = prstate.lpdead_items; /* the presult->deadoffsets array was already filled in */ @@ -1183,6 +1169,46 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, } } + /* + * If updating the visibility map, the conflict horizon for that record + * must be the newest xmin on the page. However, if the page is + * completely frozen, there can be no conflict and the vm_conflict_horizon + * should remain InvalidTransactionId. This includes the case that we + * just froze all the tuples; the prune-freeze record included the + * conflict XID already so we don't need to again. + */ + if (prstate.all_frozen) + vm_conflict_horizon = InvalidTransactionId; + else + vm_conflict_horizon = prstate.visibility_cutoff_xid; + + /* + * During its second pass over the heap, VACUUM calls + * heap_page_would_be_all_visible() to determine whether a page is + * all-visible and all-frozen. The logic here is similar. After completing + * pruning and freezing, use an assertion to verify that our results + * remain consistent with heap_page_would_be_all_visible(). + */ +#ifdef USE_ASSERT_CHECKING + if (prstate.all_visible) + { + TransactionId debug_cutoff; + bool debug_all_frozen; + + Assert(presult->lpdead_items == 0); + + Assert(heap_page_is_all_visible(params->relation, buffer, + prstate.cutoffs->OldestXmin, + &debug_all_frozen, + &debug_cutoff, off_loc)); + + Assert(prstate.all_frozen == debug_all_frozen); + + Assert(!TransactionIdIsValid(debug_cutoff) || + debug_cutoff == vm_conflict_horizon); + } +#endif + /* Now update the visibility map and PD_ALL_VISIBLE hint */ Assert(!prstate.all_visible || (prstate.lpdead_items == 0)); @@ -1229,22 +1255,21 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, * make everything safe for REDO was logged when the page's tuples * were frozen. */ - Assert(!prstate.all_frozen || - !TransactionIdIsValid(presult->vm_conflict_horizon)); + Assert(!prstate.all_frozen || !TransactionIdIsValid(vm_conflict_horizon)); visibilitymap_set(params->relation, blockno, buffer, InvalidXLogRecPtr, - vmbuffer, presult->vm_conflict_horizon, + vmbuffer, vm_conflict_horizon, new_vmbits); if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0) { presult->new_all_visible_pages = 1; - if (presult->all_frozen) + if (prstate.all_frozen) presult->new_all_visible_frozen_pages = 1; } else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 && - presult->all_frozen) + prstate.all_frozen) presult->new_all_frozen_pages = 1; } } diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 323b8e3dde3..90e94b2ac3f 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -456,13 +456,6 @@ static void dead_items_add(LVRelState *vacrel, BlockNumber blkno, OffsetNumber * static void dead_items_reset(LVRelState *vacrel); static void dead_items_cleanup(LVRelState *vacrel); -#ifdef USE_ASSERT_CHECKING -static bool heap_page_is_all_visible(Relation rel, Buffer buf, - TransactionId OldestXmin, - bool *all_frozen, - TransactionId *visibility_cutoff_xid, - OffsetNumber *logging_offnum); -#endif static bool heap_page_would_be_all_visible(Relation rel, Buffer buf, TransactionId OldestXmin, OffsetNumber *deadoffsets, @@ -2032,32 +2025,6 @@ lazy_scan_prune(LVRelState *vacrel, vacrel->new_frozen_tuple_pages++; } - /* - * VACUUM will call heap_page_is_all_visible() during the second pass over - * the heap to determine all_visible and all_frozen for the page -- this - * is a specialized version of the logic from this function. Now that - * we've finished pruning and freezing, make sure that we're in total - * agreement with heap_page_is_all_visible() using an assertion. - */ -#ifdef USE_ASSERT_CHECKING - if (presult.all_visible) - { - TransactionId debug_cutoff; - bool debug_all_frozen; - - Assert(presult.lpdead_items == 0); - - Assert(heap_page_is_all_visible(vacrel->rel, buf, - vacrel->cutoffs.OldestXmin, &debug_all_frozen, - &debug_cutoff, &vacrel->offnum)); - - Assert(presult.all_frozen == debug_all_frozen); - - Assert(!TransactionIdIsValid(debug_cutoff) || - debug_cutoff == presult.vm_conflict_horizon); - } -#endif - /* * Now save details of the LP_DEAD items from the page in vacrel */ @@ -3511,7 +3478,7 @@ dead_items_cleanup(LVRelState *vacrel) * that expect no LP_DEAD on the page. Currently assert-only, but there is no * reason not to use it outside of asserts. */ -static bool +bool heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId OldestXmin, bool *all_frozen, diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index df9fa17a6f9..a0f7974942e 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -257,8 +257,7 @@ typedef struct PruneFreezeParams * HEAP_PAGE_PRUNE_MARK_UNUSED_NOW indicates that dead items can be set * LP_UNUSED during pruning. * - * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples, and - * will return 'all_visible', 'all_frozen' flags to the caller. + * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples. * * HEAP_PAGE_PRUNE_UPDATE_VM indicates that we will set the page's status * in the VM. @@ -294,21 +293,6 @@ typedef struct PruneFreezeResult int live_tuples; int recently_dead_tuples; - /* - * all_visible and all_frozen indicate if the all-visible and all-frozen - * bits in the visibility map can be set for this page, after pruning. - * - * vm_conflict_horizon is the newest xmin of live tuples on the page. The - * caller can use it as the conflict horizon when setting the VM bits. It - * is only valid if we froze some tuples (nfrozen > 0), and all_frozen is - * true. - * - * These are only set if the HEAP_PAGE_PRUNE_FREEZE option is set. - */ - bool all_visible; - bool all_frozen; - TransactionId vm_conflict_horizon; - /* * Whether or not the page was newly set all-visible and all-frozen during * phase I of vacuuming. @@ -453,7 +437,13 @@ extern void log_heap_prune_and_freeze(Relation relation, Buffer buffer, /* in heap/vacuumlazy.c */ extern void heap_vacuum_rel(Relation rel, const VacuumParams params, BufferAccessStrategy bstrategy); - +#ifdef USE_ASSERT_CHECKING +extern bool heap_page_is_all_visible(Relation rel, Buffer buf, + TransactionId OldestXmin, + bool *all_frozen, + TransactionId *visibility_cutoff_xid, + OffsetNumber *logging_offnum); +#endif /* in heap/heapam_visibility.c */ extern bool HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer); -- 2.43.0