v25-0001-Combine-visibilitymap_set-cases-in-lazy_scan_pru.patch

From 0863c5db56d8f62cf525e8b98ab71245e27c17a6 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Mon, 8 Dec 2025 15:49:54 -0500
Subject: [PATCH v25 01/14] Combine visibilitymap_set() cases in
 lazy_scan_prune()

The heap buffer is unconditionally added to the WAL chain when setting
the VM, so it must always be marked dirty.

In one of the cases in lazy_scan_prune(), we try to avoid setting
PD_ALL_VISIBLE and marking the buffer dirty again if PD_ALL_VISIBLE is
already set. There is little gain here, and if we eliminate that
condition, we can easily combine the two cases which set the VM in
lazy_scan_prune(). This is more straightforward and makes it clear that
the heap buffer must be marked dirty since it is added to the WAL chain.

In the previously separate second VM set case, the heap buffer would
always be dirty anyway -- either because we just froze a tuple and
marked the buffer dirty or because we modified the buffer between
find_next_unskippable_block() and heap_page_prune_and_freeze() and then
pruned it in heap_page_prune_and_freeze().

This commit also adds a test case for vacuum when it does not need
to modify the heap page. Currently that would ensure the heap buffer is
marked dirty before adding it to the WAL chain, but if we ever remove it
from the VM set WAL chain or pass it with REGBUF_NO_CHANGES, it would
also serve as coverage of that.
---
 .../pg_visibility/expected/pg_visibility.out  | 17 ++++
 contrib/pg_visibility/sql/pg_visibility.sql   | 13 +++
 src/backend/access/heap/vacuumlazy.c          | 91 ++++---------------
 3 files changed, 48 insertions(+), 73 deletions(-)

diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
index 09fa5933a35..3608f801eee 100644
--- a/contrib/pg_visibility/expected/pg_visibility.out
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -204,6 +204,23 @@ select pg_truncate_visibility_map('test_partition');
  
 (1 row)
 
+-- test the case where vacuum phase I does not need to modify the heap buffer
+-- and only needs to set the VM
+create table test_vac_unmodified_heap(a int);
+insert into test_vac_unmodified_heap values (1);
+vacuum (freeze) test_vac_unmodified_heap;
+-- the checkpoint cleans the buffer dirtied by freezing the sole tuple
+checkpoint;
+-- truncating the VM ensures that the next vacuum will need to set it
+select pg_truncate_visibility_map('test_vac_unmodified_heap');
+ pg_truncate_visibility_map 
+----------------------------
+ 
+(1 row)
+
+-- vacuum sets the VM but does not need to set PD_ALL_VISIBLE so no heap page
+-- modification
+vacuum test_vac_unmodified_heap;
 -- test copy freeze
 create table copyfreeze (a int, b char(1500));
 -- load all rows via COPY FREEZE and ensure that all pages are set all-visible
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
index 5af06ec5b76..6af7c179df0 100644
--- a/contrib/pg_visibility/sql/pg_visibility.sql
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -94,6 +94,19 @@ select count(*) > 0 from pg_visibility_map_summary('test_partition');
 select * from pg_check_frozen('test_partition'); -- hopefully none
 select pg_truncate_visibility_map('test_partition');
 
+-- test the case where vacuum phase I does not need to modify the heap buffer
+-- and only needs to set the VM
+create table test_vac_unmodified_heap(a int);
+insert into test_vac_unmodified_heap values (1);
+vacuum (freeze) test_vac_unmodified_heap;
+-- the checkpoint cleans the buffer dirtied by freezing the sole tuple
+checkpoint;
+-- truncating the VM ensures that the next vacuum will need to set it
+select pg_truncate_visibility_map('test_vac_unmodified_heap');
+-- vacuum sets the VM but does not need to set PD_ALL_VISIBLE so no heap page
+-- modification
+vacuum test_vac_unmodified_heap;
+
 -- test copy freeze
 create table copyfreeze (a int, b char(1500));
 
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index e8c99c3773d..38a1268b004 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2094,15 +2094,21 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * of last heap_vac_scan_next_block() call), and from all_visible and
 	 * all_frozen variables
 	 */
-	if (!all_visible_according_to_vm && presult.all_visible)
+	if ((presult.all_visible && !all_visible_according_to_vm) ||
+		(presult.all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer)))
 	{
 		uint8		old_vmbits;
-		uint8		flags = VISIBILITYMAP_ALL_VISIBLE;
+		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));
-			flags |= VISIBILITYMAP_ALL_FROZEN;
+			new_vmbits |= VISIBILITYMAP_ALL_FROZEN;
 		}
 
 		/*
@@ -2111,35 +2117,33 @@ lazy_scan_prune(LVRelState *vacrel,
 		 * checksums are not enabled).  Regardless, set both bits so that we
 		 * get back in sync.
 		 *
-		 * NB: 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.
-		 * Given that this situation should only happen in rare cases after a
-		 * crash, it is not worth optimizing.
+		 * 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
+		 * modified in phase I is when the VM was truncated or removed, which
+		 * isn't worth optimizing for.
 		 */
 		PageSetAllVisible(page);
 		MarkBufferDirty(buf);
 		old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
 									   InvalidXLogRecPtr,
 									   vmbuffer, presult.vm_conflict_horizon,
-									   flags);
+									   new_vmbits);
 
 		/*
-		 * If the page wasn't already set all-visible and/or all-frozen in the
-		 * VM, count it as newly set for logging.
+		 * 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)
 		{
 			vacrel->vm_new_visible_pages++;
-			if (presult.all_frozen)
+			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 &&
-				 presult.all_frozen)
+				 (new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0)
 		{
 			vacrel->vm_new_frozen_pages++;
 			*vm_page_frozen = true;
@@ -2191,65 +2195,6 @@ lazy_scan_prune(LVRelState *vacrel,
 							VISIBILITYMAP_VALID_BITS);
 	}
 
-	/*
-	 * If the all-visible page is all-frozen but not marked as such yet, mark
-	 * it as all-frozen.
-	 */
-	else if (all_visible_according_to_vm && presult.all_frozen &&
-			 !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))
-	{
-		uint8		old_vmbits;
-
-		/*
-		 * Avoid relying 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 -- even when all_visible is set
-		 */
-		if (!PageIsAllVisible(page))
-		{
-			PageSetAllVisible(page);
-			MarkBufferDirty(buf);
-		}
-
-		/*
-		 * Set the page all-frozen (and all-visible) in the VM.
-		 *
-		 * 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));
-		old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
-									   InvalidXLogRecPtr,
-									   vmbuffer, InvalidTransactionId,
-									   VISIBILITYMAP_ALL_VISIBLE |
-									   VISIBILITYMAP_ALL_FROZEN);
-
-		/*
-		 * The page was likely already set all-visible in the VM. However,
-		 * there is a small chance that it was modified sometime between
-		 * setting all_visible_according_to_vm and checking the visibility
-		 * during pruning. Check the return value of old_vmbits anyway to
-		 * ensure the visibility map counters used for logging are accurate.
-		 */
-		if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
-		{
-			vacrel->vm_new_visible_pages++;
-			vacrel->vm_new_visible_frozen_pages++;
-			*vm_page_frozen = true;
-		}
-
-		/*
-		 * We already checked that the page was not set all-frozen in the VM
-		 * above, so we don't need to test the value of old_vmbits.
-		 */
-		else
-		{
-			vacrel->vm_new_frozen_pages++;
-			*vm_page_frozen = true;
-		}
-	}
-
 	return presult.ndeleted;
 }
 
-- 
2.43.0