v6-0009-Update-VM-in-pruneheap.c.patch

From 952f9aa12924868d98951ca621d09e7aefc23b81 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Mon, 2 Jun 2025 11:04:14 -0400
Subject: [PATCH v6 09/20] Update VM in pruneheap.c

As a step toward updating the VM in the same critical section and WAL
record as pruning and freezing (during phase I of vacuuming), first move
the VM update (still in its own critical section and WAL record) into
heap_page_prune_and_freeze(). This makes review easier.
---
 src/backend/access/heap/pruneheap.c  | 99 +++++++++++++++++++++++-----
 src/backend/access/heap/vacuumlazy.c | 99 +++++-----------------------
 src/include/access/heapam.h          | 15 +++--
 3 files changed, 106 insertions(+), 107 deletions(-)

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 6c3653e776c..05227ce0339 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -360,7 +360,8 @@ identify_and_fix_vm_corruption(Relation relation,
 
 /*
  * Prune and repair fragmentation and potentially freeze tuples on the
- * specified page.
+ * specified page. If the page's visibility status has changed, update it in
+ * the VM.
  *
  * Caller must have pin and buffer cleanup lock on the page.  Note that we
  * don't update the FSM information for page on caller's behalf.  Caller might
@@ -436,6 +437,8 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 	bool		do_freeze;
 	bool		do_prune;
 	bool		do_hint;
+	uint8		vmflags = 0;
+	uint8		old_vmbits = 0;
 	bool		hint_bit_fpi;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 
@@ -936,7 +939,7 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 	 *
 	 * 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.
+	 * of the page, as expected for updating the visibility map.
 	 */
 	if (prstate.all_visible && prstate.lpdead_items == 0)
 	{
@@ -952,31 +955,91 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
 	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 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 the VM update record 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 */
-
 	/*
-	 * Clear any VM corruption. This does not need to be done in a critical
-	 * section.
+	 * Handle setting visibility map bit based on information from the VM (as
+	 * of last heap_vac_scan_next_block() call), and from all_visible and
+	 * all_frozen variables.
 	 */
-	presult->vm_corruption = false;
 	if (options & HEAP_PAGE_PRUNE_UPDATE_VM)
-		presult->vm_corruption = identify_and_fix_vm_corruption(relation,
-																blockno, buffer, page,
-																blk_known_av,
-																prstate.lpdead_items, vmbuffer);
+	{
+		if (identify_and_fix_vm_corruption(relation,
+										   blockno, buffer, page,
+										   blk_known_av,
+										   prstate.lpdead_items, vmbuffer))
+		{
+			/* If we fix corruption, don't update the VM further */
+		}
+
+		/*
+		 * If the page isn't yet marked all-visible in the VM or it is and
+		 * needs to me marked all-frozen, update the VM Note that all_frozen
+		 * is only valid if all_visible is true, so we must check both
+		 * all_visible and all_frozen.
+		 */
+		else if (presult->all_visible &&
+				 (!blk_known_av ||
+				  (presult->all_frozen && !VM_ALL_FROZEN(relation, blockno, &vmbuffer))))
+		{
+			Assert(prstate.lpdead_items == 0);
+			vmflags = VISIBILITYMAP_ALL_VISIBLE;
+
+			/*
+			 * If the page is all-frozen, we can pass InvalidTransactionId as
+			 * our cutoff_xid, since a snapshotConflictHorizon sufficient to
+			 * make everything safe for REDO was logged when the page's tuples
+			 * were frozen.
+			 */
+			if (presult->all_frozen)
+			{
+				Assert(!TransactionIdIsValid(presult->vm_conflict_horizon));
+				vmflags |= VISIBILITYMAP_ALL_FROZEN;
+			}
+
+			/*
+			 * It's possible for the VM bit to be clear and the page-level bit
+			 * to be set if checksums are not enabled.
+			 *
+			 * And even if we are just planning to update the frozen bit in
+			 * the VM, we shouldn't rely on all_visible_according_to_vm as a
+			 * proxy for the page-level PD_ALL_VISIBLE bit being set, since it
+			 * might have become stale.
+			 *
+			 * If the heap page is all-visible but the VM bit is not set, we
+			 * don't need to dirty the heap page.  However, if checksums are
+			 * enabled, we do need to make sure that the heap page is dirtied
+			 * before passing it to visibilitymap_set(), because it may be
+			 * logged.
+			 */
+			if (!PageIsAllVisible(page) || XLogHintBitIsNeeded())
+			{
+				PageSetAllVisible(page);
+				MarkBufferDirty(buffer);
+			}
+
+			old_vmbits = visibilitymap_set(relation, blockno, buffer, InvalidXLogRecPtr,
+										   vmbuffer, presult->vm_conflict_horizon,
+										   vmflags);
+		}
+	}
+
+	presult->lpdead_items = prstate.lpdead_items;
+	/* the presult->deadoffsets array was already filled in */
+
+	presult->old_vmbits = old_vmbits;
+	presult->new_vmbits = vmflags;
+
 	if (prstate.freeze)
 	{
 		if (presult->nfrozen > 0)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 0c121fdf4e6..c49e81bc5dd 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1933,7 +1933,6 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
-
 /* qsort comparator for sorting OffsetNumbers */
 static int
 cmpOffsetNumbers(const void *a, const void *b)
@@ -1949,7 +1948,8 @@ cmpOffsetNumbers(const void *a, const void *b)
  * vmbuffer is the buffer containing the VM block with visibility information
  * for the heap block, blkno. all_visible_according_to_vm is the saved
  * visibility status of the heap block looked up earlier by the caller. We
- * won't rely entirely on this status, as it may be out of date.
+ * won't rely entirely on this status, as it may be out of date. These will be
+ * passed on to heap_page_prune_and_freeze() to use while setting the VM.
  *
  * *has_lpdead_items is set to true or false depending on whether, upon return
  * from this function, any LP_DEAD items are still present on the page.
@@ -1978,6 +1978,7 @@ lazy_scan_prune(LVRelState *vacrel,
 
 	/*
 	 * Prune all HOT-update chains and potentially freeze tuples on this page.
+	 * Then, if the page's visibility status has changed, update the VM.
 	 *
 	 * If the relation has no indexes, we can immediately mark would-be dead
 	 * items LP_UNUSED.
@@ -1986,10 +1987,6 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * presult.ndeleted.  It should not be confused with presult.lpdead_items;
 	 * presult.lpdead_items's final value can be thought of as the number of
 	 * tuples that were deleted from indexes.
-	 *
-	 * We will update the VM after collecting LP_DEAD items and freezing
-	 * tuples. Pruning will have determined whether or not the page is
-	 * all-visible.
 	 */
 	prune_options = HEAP_PAGE_PRUNE_FREEZE | HEAP_PAGE_PRUNE_UPDATE_VM;
 	if (vacrel->nindexes == 0)
@@ -2081,88 +2078,26 @@ lazy_scan_prune(LVRelState *vacrel,
 	Assert(!presult.all_visible || !(*has_lpdead_items));
 
 	/*
-	 * Handle setting visibility map bit based on information from the VM (as
-	 * of last heap_vac_scan_next_block() call), and from all_visible and
-	 * all_frozen variables.
+	 * For the purposes of logging, count whether or not the page was newly
+	 * set all-visible and, potentially, all-frozen.
 	 */
-	if (presult.vm_corruption)
+	if ((presult.old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 &&
+		(presult.new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
 	{
-		/* Don't update the VM if we just cleared corruption in it */
-	}
-
-	/*
-	 * If the page isn't yet marked all-visible in the VM or it is and needs
-	 * to me marked all-frozen, update the VM Note that all_frozen is only
-	 * valid if all_visible is true, so we must check both all_visible and
-	 * all_frozen.
-	 */
-	else if (presult.all_visible &&
-			 (!all_visible_according_to_vm ||
-			  (presult.all_frozen && !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))))
-	{
-		uint8		old_vmbits;
-		uint8		flags = VISIBILITYMAP_ALL_VISIBLE;
-
-		/*
-		 * If the page is all-frozen, we can pass InvalidTransactionId as our
-		 * cutoff_xid, since a snapshotConflictHorizon sufficient to make
-		 * everything safe for REDO was logged when the page's tuples were
-		 * frozen.
-		 */
-		if (presult.all_frozen)
-		{
-			Assert(!TransactionIdIsValid(presult.vm_conflict_horizon));
-			flags |= VISIBILITYMAP_ALL_FROZEN;
-		}
-
-		/*
-		 * It should never be the case that the visibility map page is set
-		 * while the page-level bit is clear, but the reverse is allowed (if
-		 * checksums are not enabled).  Regardless, set both bits so that we
-		 * get back in sync.
-		 *
-		 * If the heap page is all-visible but the VM bit is not set, we don't
-		 * need to dirty the heap page.  However, if checksums are enabled, we
-		 * do need to make sure that the heap page is dirtied before passing
-		 * it to visibilitymap_set(), because it may be logged.
-		 */
-		if (!PageIsAllVisible(page) || XLogHintBitIsNeeded())
-		{
-			PageSetAllVisible(page);
-			MarkBufferDirty(buf);
-		}
-
-		old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
-									   InvalidXLogRecPtr,
-									   vmbuffer, presult.vm_conflict_horizon,
-									   flags);
-
-		/*
-		 * Even if we are only setting the all-frozen bit, there is a small
-		 * chance that the VM was modified sometime between setting
-		 * all_visible_according_to_vm and checking the visibility during
-		 * pruning. Check the return value of old_vmbits to ensure the
-		 * visibility map counters used for logging are accurate.
-		 *
-		 * If the page wasn't already set all-visible and/or all-frozen in the
-		 * VM, count it as newly set for logging.
-		 */
-		if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
-		{
-			vacrel->vm_new_visible_pages++;
-			if (presult.all_frozen)
-			{
-				vacrel->vm_new_visible_frozen_pages++;
-				*vm_page_frozen = true;
-			}
-		}
-		else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 &&
-				 presult.all_frozen)
+		vacrel->vm_new_visible_pages++;
+		if ((presult.new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0)
 		{
-			vacrel->vm_new_frozen_pages++;
+			vacrel->vm_new_visible_frozen_pages++;
 			*vm_page_frozen = true;
 		}
 	}
+	else if ((presult.old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 &&
+			 (presult.new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0)
+	{
+		Assert((presult.new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0);
+		vacrel->vm_new_frozen_pages++;
+		*vm_page_frozen = true;
+	}
 
 	return presult.ndeleted;
 }
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 0c7eb5e46f4..b85648456e9 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -235,20 +235,21 @@ typedef struct PruneFreezeResult
 	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.
+	 * all_visible and all_frozen indicate the status of the page as reflected
+	 * in the visibility map after pruning, freezing, and setting any pages
+	 * all-visible in the visibility map.
 	 *
-	 * 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.
+	 * vm_conflict_horizon is the newest xmin of live tuples on the page
+	 * (older than OldestXmin).  It will only be valid if we did not set the
+	 * page all-frozen in the VM.
 	 *
 	 * These are only set if the HEAP_PRUNE_FREEZE option is set.
 	 */
 	bool		all_visible;
 	bool		all_frozen;
 	TransactionId vm_conflict_horizon;
-	bool		vm_corruption;
+	uint8		old_vmbits;
+	uint8		new_vmbits;
 
 	/*
 	 * Whether or not the page makes rel truncation unsafe.  This is set to
-- 
2.43.0