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 <melanieplageman@gmail.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/flat/CAAKRu_ZMw6Npd_qm2KM%2BFwQ3cMOMx1Dh3VMhp8-V7SOLxdK9-g%40mail.gmail.com

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index e9e14cb42b7a695345b308c6f1770aebee627947..7cd51c7be3343f742cd4d5a471d06bd9e092a488 100644 (file)
--- 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 c3fc1098e232962b33b55a0a828536adeb6b47ae..7a6d6f4263467d5c35fd74426e2520a770bd9240 100644 (file)
--- 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;