From bedba753bb9fcc37b3b5f1a7e38c02828850520d Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 14 Oct 2025 14:55:40 -0400 Subject: [PATCH v21 02/12] Keep all_frozen updated in heap_page_prune_and_freeze Previously, we relied on all_visible and all_frozen being used together to ensure that all_frozen was correct, but it is better to keep both fields updated. Future changes will separate their usage, so we should not depend on all_visible for the validity of all_frozen. Author: Melanie Plageman Reviewed-by: Kirill Reshke Discussion: https://postgr.es/m/flat/CAAKRu_ZMw6Npd_qm2KM%2BFwQ3cMOMx1Dh3VMhp8-V7SOLxdK9-g%40mail.gmail.com --- src/backend/access/heap/pruneheap.c | 60 +++++++++++++++------------- src/backend/access/heap/vacuumlazy.c | 9 ++--- 2 files changed, 37 insertions(+), 32 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index e9e14cb42b7..7cd51c7be33 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -143,10 +143,6 @@ typedef struct * whether to freeze the page or not. The all_visible and all_frozen * values returned to the caller are adjusted to include LP_DEAD items at * the end. - * - * all_frozen should only be considered valid if all_visible is also set; - * we don't bother to clear the all_frozen flag every time we clear the - * all_visible flag. */ bool all_visible; bool all_frozen; @@ -359,8 +355,10 @@ heap_page_will_freeze(Relation relation, Buffer buffer, * anymore. The opportunistic freeze heuristic must be improved; * however, for now, try to approximate the old logic. */ - if (prstate->all_visible && prstate->all_frozen && prstate->nfrozen > 0) + if (prstate->all_frozen && prstate->nfrozen > 0) { + Assert(prstate->all_visible); + /* * Freezing would make the page all-frozen. Have already emitted * an FPI or will do so anyway? @@ -544,9 +542,9 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, * dead tuples which are not yet removable. However, dead tuples which * will be removed by the end of vacuuming should not preclude us from * opportunistically freezing. Because of that, we do not clear - * all_visible when we see LP_DEAD items. We fix that at the end of the - * function, when we return the value to the caller, so that the caller - * doesn't set the VM bit incorrectly. + * all_visible and all_frozen when we see LP_DEAD items. We fix that at + * the end of the function, when we return the value to the caller, so + * that the caller doesn't set the VM bits incorrectly. */ if (prstate.attempt_freeze) { @@ -783,6 +781,8 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, do_hint_prune, &prstate); + Assert(!prstate.all_frozen || prstate.all_visible); + /* Any error while applying the changes is critical */ START_CRIT_SECTION(); @@ -852,7 +852,7 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, */ if (do_freeze) { - if (prstate.all_visible && prstate.all_frozen) + if (prstate.all_frozen) frz_conflict_horizon = prstate.visibility_cutoff_xid; else { @@ -889,16 +889,16 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, presult->recently_dead_tuples = prstate.recently_dead_tuples; /* - * It was convenient to ignore LP_DEAD items in all_visible earlier on to - * make the choice of whether or not to freeze the page unaffected by the - * short-term presence of LP_DEAD items. These LP_DEAD items were - * effectively assumed to be LP_UNUSED items in the making. It doesn't - * matter which vacuum heap pass (initial pass or final pass) ends up - * setting the page all-frozen, as long as the ongoing VACUUM does it. + * It was convenient to ignore LP_DEAD items in all_visible/all_frozen + * earlier on to make the choice of whether or not to freeze the page + * unaffected by the short-term presence of LP_DEAD items. These LP_DEAD + * items were effectively assumed to be LP_UNUSED items in the making. It + * doesn't matter which vacuum heap pass (initial pass or final pass) ends + * up setting the page all-frozen, as long as the ongoing VACUUM does it. * - * Now that freezing has been finalized, unset all_visible if there are - * any LP_DEAD items on the page. It needs to reflect the present state - * of the page, as expected by our caller. + * Now that freezing has been finalized, unset all_visible and all_frozen + * if there are any LP_DEAD items on the page. It needs to reflect the + * present state of the page, as expected by our caller. */ if (prstate.all_visible && prstate.lpdead_items == 0) { @@ -1289,8 +1289,9 @@ heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum, prstate->ndead++; /* - * Deliberately delay unsetting all_visible until later during pruning. - * Removable dead tuples shouldn't preclude freezing the page. + * Deliberately delay unsetting all_visible and all_frozen until later + * during pruning. Removable dead tuples shouldn't preclude freezing the + * page. */ /* Record the dead offset for vacuum */ @@ -1418,6 +1419,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb if (!HeapTupleHeaderXminCommitted(htup)) { prstate->all_visible = false; + prstate->all_frozen = false; break; } @@ -1432,14 +1434,15 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb /* * For now always use prstate->cutoffs for this test, because - * we only update 'all_visible' when freezing is requested. We - * could use GlobalVisTestIsRemovableXid instead, if a - * non-freezing caller wanted to set the VM bit. + * we only update 'all_visible' and 'all_frozen' when freezing + * is requested. We could use GlobalVisTestIsRemovableXid + * instead, if a non-freezing caller wanted to set the VM bit. */ Assert(prstate->cutoffs); if (!TransactionIdPrecedes(xmin, prstate->cutoffs->OldestXmin)) { prstate->all_visible = false; + prstate->all_frozen = false; break; } @@ -1453,6 +1456,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb case HEAPTUPLE_RECENTLY_DEAD: prstate->recently_dead_tuples++; prstate->all_visible = false; + prstate->all_frozen = false; /* * This tuple will soon become DEAD. Update the hint field so @@ -1472,6 +1476,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb * does, so be consistent. */ prstate->all_visible = false; + prstate->all_frozen = false; /* * If we wanted to optimize for aborts, we might consider marking @@ -1490,6 +1495,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb */ prstate->live_tuples++; prstate->all_visible = false; + prstate->all_frozen = false; /* * This tuple may soon become DEAD. Update the hint field so that @@ -1554,10 +1560,10 @@ heap_prune_record_unchanged_lp_dead(Page page, PruneState *prstate, OffsetNumber * hastup/nonempty_pages as provisional no matter how LP_DEAD items are * handled (handled here, or handled later on). * - * Similarly, don't unset all_visible until later, at the end of - * heap_page_prune_and_freeze(). This will allow us to attempt to freeze - * the page after pruning. As long as we unset it before updating the - * visibility map, this will be correct. + * Similarly, don't unset all_visible and all_frozen until later, at the + * end of heap_page_prune_and_freeze(). This will allow us to attempt to + * freeze the page after pruning. As long as we unset it before updating + * the visibility map, this will be correct. */ /* Record the dead offset for vacuum */ diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 2b9e5c7f81b..e1b7456823d 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2017,7 +2017,6 @@ lazy_scan_prune(LVRelState *vacrel, * agreement with heap_page_is_all_visible() using an assertion. */ #ifdef USE_ASSERT_CHECKING - /* Note that all_frozen value does not matter when !all_visible */ if (presult.all_visible) { TransactionId debug_cutoff; @@ -2071,6 +2070,7 @@ lazy_scan_prune(LVRelState *vacrel, *has_lpdead_items = (presult.lpdead_items > 0); Assert(!presult.all_visible || !(*has_lpdead_items)); + Assert(!presult.all_frozen || presult.all_visible); /* * Handle setting visibility map bit based on information from the VM (as @@ -2176,11 +2176,10 @@ lazy_scan_prune(LVRelState *vacrel, /* * If the all-visible page is all-frozen but not marked as such yet, mark - * it as all-frozen. Note that all_frozen is only valid if all_visible is - * true, so we must check both all_visible and all_frozen. + * it as all-frozen. */ - else if (all_visible_according_to_vm && presult.all_visible && - presult.all_frozen && !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer)) + else if (all_visible_according_to_vm && presult.all_frozen && + !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer)) { uint8 old_vmbits; -- 2.43.0