From 9aa0ec2b5fae04762128fbec329a23139fb5b4a4 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 2 Dec 2025 15:57:34 -0500 Subject: [PATCH v23 04/14] Move VM assert into prune/freeze code For review only, this commit moves the check of the heap page into prune/freeze code before setting the VM. This allows us to remove some fields of the PruneFreezeResult. This will get squashed into a larger commit to set the VM in the same record where we prune and freeze. --- src/backend/access/heap/pruneheap.c | 142 +++++++++++++++++++-------- src/backend/access/heap/vacuumlazy.c | 68 +------------ src/include/access/heapam.h | 25 ++--- 3 files changed, 111 insertions(+), 124 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 0daf3abf717..2512b5d83e3 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -199,7 +199,7 @@ static bool heap_page_will_set_vis(Relation relation, Buffer heap_buf, Buffer vmbuffer, bool blk_known_av, - const PruneFreezeResult *presult, + const PruneState *prstate, uint8 *new_vmbits, bool *do_set_pd_vis); @@ -785,8 +785,8 @@ heap_page_will_freeze(Relation relation, Buffer buffer, /* * Decide whether to set the visibility map bits for heap_blk, using - * information from PruneFreezeResult and blk_known_av. Some callers may - * already have examined this page’s VM bits (e.g., VACUUM in the previous + * information from PruneState and blk_known_av. Some callers may already have + * examined this page’s VM bits (e.g., VACUUM in the previous * heap_vac_scan_next_block() call) and can pass that along as blk_known_av. * Callers that have not previously checked the page's status in the VM should * pass false for blk_known_av. @@ -808,13 +808,20 @@ heap_page_will_set_vis(Relation relation, Buffer heap_buf, Buffer vmbuffer, bool blk_known_av, - const PruneFreezeResult *presult, + const PruneState *prstate, uint8 *new_vmbits, bool *do_set_pd_vis) { Page heap_page = BufferGetPage(heap_buf); *new_vmbits = 0; + *do_set_pd_vis = false; + + if (!prstate->attempt_update_vm) + { + Assert(!prstate->all_visible && !prstate->all_frozen); + return false; + } /* * It should never be the case that the visibility map page is set while @@ -825,22 +832,19 @@ heap_page_will_set_vis(Relation relation, * PD_ALL_VISIBLE bit being set, since it might have become stale and may * not be provided by all callers. */ - *do_set_pd_vis = presult->all_visible & !PageIsAllVisible(heap_page); + *do_set_pd_vis = prstate->all_visible & !PageIsAllVisible(heap_page); /* * Determine what the visibility map bits should be set to using the * values of all_visible and all_frozen determined during * pruning/freezing. */ - if ((presult->all_visible && !blk_known_av) || - (presult->all_frozen && !VM_ALL_FROZEN(relation, heap_blk, &vmbuffer))) + if ((prstate->all_visible && !blk_known_av) || + (prstate->all_frozen && !VM_ALL_FROZEN(relation, heap_blk, &vmbuffer))) { *new_vmbits = VISIBILITYMAP_ALL_VISIBLE; - if (presult->all_frozen) - { - Assert(!TransactionIdIsValid(presult->vm_conflict_horizon)); + if (prstate->all_frozen) *new_vmbits |= VISIBILITYMAP_ALL_FROZEN; - } return true; } @@ -887,7 +891,7 @@ heap_page_will_set_vis(Relation relation, * There should never be LP_DEAD items on a page with PD_ALL_VISIBLE set, * however. */ - else if (presult->lpdead_items > 0 && PageIsAllVisible(heap_page)) + else if (prstate->lpdead_items > 0 && PageIsAllVisible(heap_page)) { ereport(WARNING, (errcode(ERRCODE_DATA_CORRUPTED), @@ -903,6 +907,30 @@ heap_page_will_set_vis(Relation relation, return false; } +#ifdef USE_ASSERT_CHECKING + +/* + * Wrapper for heap_page_would_be_all_visible() which can be used for callers + * 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 +heap_page_is_all_visible(Relation rel, Buffer buf, + TransactionId OldestXmin, + bool *all_frozen, + TransactionId *visibility_cutoff_xid, + OffsetNumber *logging_offnum) +{ + + return heap_page_would_be_all_visible(rel, buf, + OldestXmin, + NULL, 0, + all_frozen, + visibility_cutoff_xid, + logging_offnum); +} +#endif + /* * Prune and repair fragmentation and potentially freeze tuples on the @@ -956,6 +984,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; @@ -1116,23 +1145,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 */ @@ -1150,20 +1164,68 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, } } - /* Now update the visibility map and PD_ALL_VISIBLE hint */ - Assert(!prstate.all_visible || (prstate.lpdead_items == 0)); + /* + * 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)); - do_set_vm = false; - if (prstate.attempt_update_vm) - do_set_vm = heap_page_will_set_vis(params->relation, - blockno, - buffer, - vmbuffer, - params->blk_known_av, - presult, - &presult->new_vmbits, - &do_set_pd_vis); + Assert(prstate.all_frozen == debug_all_frozen); + Assert(!TransactionIdIsValid(debug_cutoff) || + debug_cutoff == vm_conflict_horizon); + } +#endif + + Assert(!prstate.all_frozen || prstate.all_visible); + Assert(!prstate.all_visible || (prstate.lpdead_items == 0)); + + /* + * Decide whether to set the page-level PD_ALL_VISIBLE bit and the VM bits + * based on information from the VM and the all_visible/all_frozen flags. + * + * While it is valid for PD_ALL_VISIBLE to be set when the corresponding + * VM bit is clear, we strongly prefer to keep them in sync. + * + * Accordingly, we also allow updating only the VM when PD_ALL_VISIBLE has + * already been set. Setting only the VM is most common when setting an + * already all-visible page all-frozen. + */ + do_set_vm = heap_page_will_set_vis(params->relation, + blockno, + buffer, + vmbuffer, + params->blk_known_av, + &prstate, + &presult->new_vmbits, + &do_set_pd_vis); /* We should only set the VM if PD_ALL_VISIBLE is set or will be */ Assert(!do_set_vm || do_set_pd_vis || PageIsAllVisible(page)); @@ -1192,7 +1254,7 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, if (do_set_vm) presult->old_vmbits = visibilitymap_set(params->relation, blockno, buffer, InvalidXLogRecPtr, - vmbuffer, presult->vm_conflict_horizon, + vmbuffer, vm_conflict_horizon, presult->new_vmbits); } diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index f5617335cb2..4aa425ec945 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -464,20 +464,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, - int ndeadoffsets, - bool *all_frozen, - TransactionId *visibility_cutoff_xid, - OffsetNumber *logging_offnum); static void update_relstats_all_indexes(LVRelState *vacrel); static void vacuum_error_callback(void *arg); static void update_vacuum_error_info(LVRelState *vacrel, @@ -2016,32 +2002,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 */ @@ -3496,29 +3456,6 @@ dead_items_cleanup(LVRelState *vacrel) vacrel->pvs = NULL; } -#ifdef USE_ASSERT_CHECKING - -/* - * Wrapper for heap_page_would_be_all_visible() which can be used for callers - * 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 -heap_page_is_all_visible(Relation rel, Buffer buf, - TransactionId OldestXmin, - bool *all_frozen, - TransactionId *visibility_cutoff_xid, - OffsetNumber *logging_offnum) -{ - - return heap_page_would_be_all_visible(rel, buf, - OldestXmin, - NULL, 0, - all_frozen, - visibility_cutoff_xid, - logging_offnum); -} -#endif /* * Check whether the heap page in buf is all-visible except for the dead @@ -3542,15 +3479,12 @@ heap_page_is_all_visible(Relation rel, Buffer buf, * - *logging_offnum: OffsetNumber of current tuple being processed; * used by vacuum's error callback system. * - * Callers looking to verify that the page is already all-visible can call - * heap_page_is_all_visible(). - * * This logic is closely related to heap_prune_record_unchanged_lp_normal(). * If you modify this function, ensure consistency with that code. An * assertion cross-checks that both remain in agreement. Do not introduce new * side-effects. */ -static bool +bool heap_page_would_be_all_visible(Relation rel, Buffer buf, TransactionId OldestXmin, OffsetNumber *deadoffsets, diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index ce9cfbdc767..b20096b6ca1 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -263,8 +263,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_VIS indicates that we will set the page's status * in the VM. @@ -300,21 +299,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_PRUNE_FREEZE option is set. - */ - bool all_visible; - bool all_frozen; - TransactionId vm_conflict_horizon; - /* * old_vmbits are the state of the all-visible and all-frozen bits in the * visibility map before updating it during phase I of vacuuming. @@ -460,6 +444,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); +extern bool heap_page_would_be_all_visible(Relation rel, Buffer buf, + TransactionId OldestXmin, + OffsetNumber *deadoffsets, + int ndeadoffsets, + bool *all_frozen, + TransactionId *visibility_cutoff_xid, + OffsetNumber *logging_offnum); /* in heap/heapam_visibility.c */ extern bool HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, -- 2.43.0