v24-0004-Refactor-lazy_scan_prune-VM-set-logic-into-helpe.patch

From 842dd8da0c38440315de4e01bda026970b42d7eb Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 2 Dec 2025 13:36:39 -0500
Subject: [PATCH v24 04/16] Refactor lazy_scan_prune() VM set logic into helper

While this may not be an improvement on its own, encapsulating the logic
for determining what to set the VM bits to in a helper is one step
toward setting the VM in heap_page_prune_and_freeze().
---
 src/backend/access/heap/vacuumlazy.c | 209 ++++++++++++++++-----------
 1 file changed, 126 insertions(+), 83 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 14040552e48..577950c2f77 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1934,6 +1934,104 @@ cmpOffsetNumbers(const void *a, const void *b)
 	return pg_cmp_u16(*(const OffsetNumber *) a, *(const OffsetNumber *) b);
 }
 
+
+/*
+ * Decide whether to set the visibility map bits (all-visible and all-frozen)
+ * for heap_blk using information from PruneFreezeResult and
+ * all_visible_according_to_vm. This function does not actually set the VM
+ * bits or page-level visibility hint, PD_ALL_VISIBLE.
+ *
+ * If it finds that the page-level visibility hint or VM is corrupted, it will
+ * fix them by clearing the VM bits and visibility page hint. This does not
+ * need to be done in a critical section.
+ *
+ * Returns true if one or both VM bits should be set, along with returning
+ * what bits should be set in the VM in *new_vmbits.
+ */
+static bool
+heap_page_will_set_vm(Relation relation,
+					  BlockNumber heap_blk,
+					  Buffer heap_buf,
+					  Buffer vmbuffer,
+					  bool all_visible_according_to_vm,
+					  const PruneFreezeResult *presult,
+					  uint8 *new_vmbits)
+{
+	Page		heap_page = BufferGetPage(heap_buf);
+
+	*new_vmbits = 0;
+
+	/*
+	 * Determine what to set the visibility map bits to based on information
+	 * from the VM (as of last heap_vac_scan_next_block() call), and from
+	 * all_visible and all_frozen variables.
+	 */
+	if ((presult->all_visible && !all_visible_according_to_vm) ||
+		(presult->all_frozen && !VM_ALL_FROZEN(relation, heap_blk, &vmbuffer)))
+	{
+		*new_vmbits = VISIBILITYMAP_ALL_VISIBLE;
+		if (presult->all_frozen)
+		{
+			Assert(!TransactionIdIsValid(presult->vm_conflict_horizon));
+			*new_vmbits |= VISIBILITYMAP_ALL_FROZEN;
+		}
+
+		return true;
+	}
+
+	/*
+	 * Now handle two potential corruption cases:
+	 *
+	 * These do not need to happen in a critical section and are not
+	 * WAL-logged.
+	 *
+	 * As of PostgreSQL 9.2, the visibility map bit should never be set if the
+	 * page-level bit is clear.  However, it's possible that the bit got
+	 * cleared after heap_vac_scan_next_block() was called, so we must recheck
+	 * with buffer lock before concluding that the VM is corrupt.
+	 */
+	else if (all_visible_according_to_vm && !PageIsAllVisible(heap_page) &&
+			 visibilitymap_get_status(relation, heap_blk, &vmbuffer) != 0)
+	{
+		ereport(WARNING,
+				(errcode(ERRCODE_DATA_CORRUPTED),
+				 errmsg("page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u",
+						RelationGetRelationName(relation), heap_blk)));
+
+		visibilitymap_clear(relation, heap_blk, vmbuffer,
+							VISIBILITYMAP_VALID_BITS);
+	}
+
+	/*
+	 * It's possible for the value returned by
+	 * GetOldestNonRemovableTransactionId() to move backwards, so it's not
+	 * wrong for us to see tuples that appear to not be visible to everyone
+	 * yet, while PD_ALL_VISIBLE is already set. The real safe xmin value
+	 * never moves backwards, but GetOldestNonRemovableTransactionId() is
+	 * conservative and sometimes returns a value that's unnecessarily small,
+	 * so if we see that contradiction it just means that the tuples that we
+	 * think are not visible to everyone yet actually are, and the
+	 * PD_ALL_VISIBLE flag is correct.
+	 *
+	 * There should never be LP_DEAD items on a page with PD_ALL_VISIBLE set,
+	 * however.
+	 */
+	else if (presult->lpdead_items > 0 && PageIsAllVisible(heap_page))
+	{
+		ereport(WARNING,
+				(errcode(ERRCODE_DATA_CORRUPTED),
+				 errmsg("page containing LP_DEAD items is marked as all-visible in relation \"%s\" page %u",
+						RelationGetRelationName(relation), heap_blk)));
+
+		PageClearAllVisible(heap_page);
+		MarkBufferDirty(heap_buf);
+		visibilitymap_clear(relation, heap_blk, vmbuffer,
+							VISIBILITYMAP_VALID_BITS);
+	}
+
+	return false;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1964,6 +2062,9 @@ lazy_scan_prune(LVRelState *vacrel,
 				bool *vm_page_frozen)
 {
 	Relation	rel = vacrel->rel;
+	bool		do_set_vm = false;
+	uint8		new_vmbits = 0;
+	uint8		old_vmbits = 0;
 	PruneFreezeResult presult;
 	PruneFreezeParams params = {
 		.relation = rel,
@@ -2075,33 +2176,20 @@ lazy_scan_prune(LVRelState *vacrel,
 	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
-	 * of last heap_vac_scan_next_block() call), and from all_visible and
-	 * all_frozen variables
-	 */
-	if ((presult.all_visible && !all_visible_according_to_vm) ||
-		(presult.all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer)))
-	{
-		uint8		old_vmbits;
-		uint8		new_vmbits = VISIBILITYMAP_ALL_VISIBLE;
-
-		if (presult.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.
-			 */
-			Assert(!TransactionIdIsValid(presult.vm_conflict_horizon));
-			new_vmbits |= VISIBILITYMAP_ALL_FROZEN;
-		}
+	do_set_vm = heap_page_will_set_vm(rel,
+									  blkno,
+									  buf,
+									  vmbuffer,
+									  all_visible_according_to_vm,
+									  &presult,
+									  &new_vmbits);
 
+	if (do_set_vm)
+	{
 		/*
 		 * 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.
+		 * checksums are not enabled).
 		 *
 		 * The heap page is added to the WAL chain even if it wasn't modified,
 		 * so we still need to mark it dirty. The only scenario where it isn't
@@ -2114,73 +2202,28 @@ lazy_scan_prune(LVRelState *vacrel,
 									   InvalidXLogRecPtr,
 									   vmbuffer, presult.vm_conflict_horizon,
 									   new_vmbits);
-
-		/*
-		 * For the purposes of logging, count whether or not the page was
-		 * newly set all-visible and, potentially, all-frozen.
-		 */
-		if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 &&
-			(new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
-		{
-			vacrel->vm_new_visible_pages++;
-			if ((new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0)
-			{
-				vacrel->vm_new_visible_frozen_pages++;
-				*vm_page_frozen = true;
-			}
-		}
-		else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 &&
-				 (new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0)
-		{
-			Assert((new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0);
-			vacrel->vm_new_frozen_pages++;
-			*vm_page_frozen = true;
-		}
 	}
 
 	/*
-	 * As of PostgreSQL 9.2, the visibility map bit should never be set if the
-	 * page-level bit is clear.  However, it's possible that the bit got
-	 * cleared after heap_vac_scan_next_block() was called, so we must recheck
-	 * with buffer lock before concluding that the VM is corrupt.
+	 * For the purposes of logging, count whether or not the page was newly
+	 * set all-visible and, potentially, all-frozen.
 	 */
-	else if (all_visible_according_to_vm && !PageIsAllVisible(page) &&
-			 visibilitymap_get_status(vacrel->rel, blkno, &vmbuffer) != 0)
+	if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 &&
+		(new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
 	{
-		ereport(WARNING,
-				(errcode(ERRCODE_DATA_CORRUPTED),
-				 errmsg("page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u",
-						vacrel->relname, blkno)));
-
-		visibilitymap_clear(vacrel->rel, blkno, vmbuffer,
-							VISIBILITYMAP_VALID_BITS);
+		vacrel->vm_new_visible_pages++;
+		if ((new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0)
+		{
+			vacrel->vm_new_visible_frozen_pages++;
+			*vm_page_frozen = true;
+		}
 	}
-
-	/*
-	 * It's possible for the value returned by
-	 * GetOldestNonRemovableTransactionId() to move backwards, so it's not
-	 * wrong for us to see tuples that appear to not be visible to everyone
-	 * yet, while PD_ALL_VISIBLE is already set. The real safe xmin value
-	 * never moves backwards, but GetOldestNonRemovableTransactionId() is
-	 * conservative and sometimes returns a value that's unnecessarily small,
-	 * so if we see that contradiction it just means that the tuples that we
-	 * think are not visible to everyone yet actually are, and the
-	 * PD_ALL_VISIBLE flag is correct.
-	 *
-	 * There should never be LP_DEAD items on a page with PD_ALL_VISIBLE set,
-	 * however.
-	 */
-	else if (presult.lpdead_items > 0 && PageIsAllVisible(page))
+	else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 &&
+			 (new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0)
 	{
-		ereport(WARNING,
-				(errcode(ERRCODE_DATA_CORRUPTED),
-				 errmsg("page containing LP_DEAD items is marked as all-visible in relation \"%s\" page %u",
-						vacrel->relname, blkno)));
-
-		PageClearAllVisible(page);
-		MarkBufferDirty(buf);
-		visibilitymap_clear(vacrel->rel, blkno, vmbuffer,
-							VISIBILITYMAP_VALID_BITS);
+		Assert((new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0);
+		vacrel->vm_new_frozen_pages++;
+		*vm_page_frozen = true;
 	}
 
 	return presult.ndeleted;
-- 
2.43.0