From f179a5493c9671f9c8eca9231292d3a48bf7153c Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 14 Oct 2025 14:55:40 -0400 Subject: [PATCH v19 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. --- src/backend/access/heap/pruneheap.c | 22 +++++++++++----------- src/backend/access/heap/vacuumlazy.c | 9 ++++----- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 450b2eb6494..daa719fc2a1 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; @@ -361,8 +357,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? @@ -784,6 +782,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(); @@ -853,7 +853,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 { @@ -1418,7 +1418,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb if (!HeapTupleHeaderXminCommitted(htup)) { - prstate->all_visible = false; + prstate->all_visible = prstate->all_frozen = false; break; } @@ -1440,7 +1440,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb Assert(prstate->cutoffs); if (!TransactionIdPrecedes(xmin, prstate->cutoffs->OldestXmin)) { - prstate->all_visible = false; + prstate->all_visible = prstate->all_frozen = false; break; } @@ -1453,7 +1453,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_visible = prstate->all_frozen = false; /* * This tuple will soon become DEAD. Update the hint field so @@ -1472,7 +1472,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb * assumption is a bit shaky, but it is what acquire_sample_rows() * does, so be consistent. */ - prstate->all_visible = false; + prstate->all_visible = prstate->all_frozen = false; /* * If we wanted to optimize for aborts, we might consider marking @@ -1490,7 +1490,7 @@ heap_prune_record_unchanged_lp_normal(Page page, PruneState *prstate, OffsetNumb * will commit and update the counters after we report. */ prstate->live_tuples++; - prstate->all_visible = false; + prstate->all_visible = prstate->all_frozen = false; /* * This tuple may soon become DEAD. Update the hint field so that diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index e55be07cae4..670a7424b15 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2021,7 +2021,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; @@ -2075,6 +2074,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 @@ -2180,11 +2180,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