From 51729486db735989377d18bfc855d0d3d7f32114 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Wed, 23 Jul 2025 16:01:24 -0400 Subject: [PATCH v14 04/24] Keep all_frozen updated in heap_page_prune_and_freeze We previously relied on only using all-visible and all-frozen together but it's best to keep them both updated. Future commits will separate usage of these fields, so it is best not to rely on all_visible for all_frozen's validity. --- src/backend/access/heap/pruneheap.c | 21 ++++++++++----------- src/backend/access/heap/vacuumlazy.c | 9 ++++----- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 28bd6a56749..ea8216e0632 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -142,10 +142,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; @@ -696,8 +692,10 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, * used 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? @@ -750,6 +748,7 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, */ } + Assert(!prstate.all_frozen || prstate.all_visible); /* Any error while applying the changes is critical */ START_CRIT_SECTION(); @@ -819,7 +818,7 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, */ if (do_freeze) { - if (prstate.all_visible && prstate.all_frozen) + if (prstate.all_frozen) frz_conflict_horizon = prstate.visibility_cutoff_xid; else { @@ -1382,7 +1381,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; } @@ -1404,7 +1403,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; } @@ -1417,7 +1416,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 @@ -1436,7 +1435,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 @@ -1454,7 +1453,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 ddc9677694c..50cc898087f 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2003,7 +2003,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; @@ -2056,6 +2055,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 @@ -2161,11 +2161,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