v24-0006-Move-VM-assert-into-prune-freeze-code.patch

From 30e6b5420669389b0b0e6169905d344442d17266 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 2 Dec 2025 15:57:34 -0500
Subject: [PATCH v24 06/16] Move VM assert into prune/freeze code

This is a step toward setting the VM in the same WAL record as pruning
and freezing. It moves the check of the heap page into prune/freeze code
before setting the VM. This allows us to remove some fields of the
PruneFreezeResult.
---
 src/backend/access/heap/pruneheap.c  | 138 +++++++++++++++++++--------
 src/backend/access/heap/vacuumlazy.c |  68 +------------
 src/include/access/heapam.h          |  25 ++---
 3 files changed, 109 insertions(+), 122 deletions(-)

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index d7f36e2764f..96dc902ec12 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -199,7 +199,7 @@ static bool heap_page_will_set_vm(Relation relation,
 								  Buffer heap_buf,
 								  Buffer vmbuffer,
 								  bool blk_known_av,
-								  const PruneFreezeResult *presult,
+								  const PruneState *prstate,
 								  uint8 *new_vmbits);
 
 
@@ -784,9 +784,9 @@ heap_page_will_freeze(Relation relation, Buffer buffer,
 
 /*
  * Decide whether to set the visibility map bits (all-visible and all-frozen)
- * for heap_blk using information from PruneFreezeResult and blk_known_av.
- * Some callers may already have examined this page’s VM bits (e.g., VACUUM in
- * the previous heap_vac_scan_next_block() call) and can pass that along as
+ * for heap_blk using information from PruneState and blk_known_av. Some
+ * callers may already have examined this page’s VM bits (e.g., VACUUM in the
+ * previous heap_vac_scan_next_block() call) and can pass that along as
  * blk_known_av. Callers that have not previously checked the page's status in
  * the VM should pass false for blk_known_av.
  *
@@ -806,27 +806,30 @@ heap_page_will_set_vm(Relation relation,
 					  Buffer heap_buf,
 					  Buffer vmbuffer,
 					  bool blk_known_av,
-					  const PruneFreezeResult *presult,
+					  const PruneState *prstate,
 					  uint8 *new_vmbits)
 {
 	Page		heap_page = BufferGetPage(heap_buf);
 
 	*new_vmbits = 0;
 
+	if (!prstate->attempt_update_vm)
+	{
+		Assert(!prstate->all_visible && !prstate->all_frozen);
+		return false;
+	}
+
 	/*
 	 * Determine what the visibility map bits should be set to using the
 	 * values of all_visible and all_frozen determined during
 	 * pruning/freezing.
 	 */
-	if ((presult->all_visible && !blk_known_av) ||
-		(presult->all_frozen && !VM_ALL_FROZEN(relation, heap_blk, &vmbuffer)))
+	if ((prstate->all_visible && !blk_known_av) ||
+		(prstate->all_frozen && !VM_ALL_FROZEN(relation, heap_blk, &vmbuffer)))
 	{
 		*new_vmbits = VISIBILITYMAP_ALL_VISIBLE;
-		if (presult->all_frozen)
-		{
-			Assert(!TransactionIdIsValid(presult->vm_conflict_horizon));
+		if (prstate->all_frozen)
 			*new_vmbits |= VISIBILITYMAP_ALL_FROZEN;
-		}
 
 		return true;
 	}
@@ -873,7 +876,7 @@ heap_page_will_set_vm(Relation relation,
 	 * 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))
+	else if (prstate->lpdead_items > 0 && PageIsAllVisible(heap_page))
 	{
 		ereport(WARNING,
 				(errcode(ERRCODE_DATA_CORRUPTED),
@@ -889,6 +892,30 @@ heap_page_will_set_vm(Relation relation,
 	return false;
 }
 
+#ifdef USE_ASSERT_CHECKING
+
+/*
+ * Wrapper for heap_page_would_be_all_visible() which can be used for callers
+ * that expect no LP_DEAD on the page. Currently assert-only, but there is no
+ * reason not to use it outside of asserts.
+ */
+static bool
+heap_page_is_all_visible(Relation rel, Buffer buf,
+						 TransactionId OldestXmin,
+						 bool *all_frozen,
+						 TransactionId *visibility_cutoff_xid,
+						 OffsetNumber *logging_offnum)
+{
+
+	return heap_page_would_be_all_visible(rel, buf,
+										  OldestXmin,
+										  NULL, 0,
+										  all_frozen,
+										  visibility_cutoff_xid,
+										  logging_offnum);
+}
+#endif
+
 
 /*
  * Prune and repair fragmentation and potentially freeze tuples on the
@@ -942,6 +969,7 @@ heap_page_prune_and_freeze(PruneFreezeParams *params,
 	Buffer		vmbuffer = params->vmbuffer;
 	Page		page = BufferGetPage(buffer);
 	BlockNumber blockno = BufferGetBlockNumber(buffer);
+	TransactionId vm_conflict_horizon = InvalidTransactionId;
 	PruneState	prstate;
 	bool		do_freeze;
 	bool		do_prune;
@@ -1097,23 +1125,8 @@ heap_page_prune_and_freeze(PruneFreezeParams *params,
 	presult->nfrozen = prstate.nfrozen;
 	presult->live_tuples = prstate.live_tuples;
 	presult->recently_dead_tuples = prstate.recently_dead_tuples;
-	presult->all_visible = prstate.all_visible;
-	presult->all_frozen = prstate.all_frozen;
 	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 (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 */
 
@@ -1134,18 +1147,67 @@ heap_page_prune_and_freeze(PruneFreezeParams *params,
 	presult->new_vmbits = 0;
 	presult->old_vmbits = 0;
 
-	/* Now update the visibility map and PD_ALL_VISIBLE hint */
+	/*
+	 * 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 we don't need to again.
+	 */
+	if (prstate.all_frozen)
+		vm_conflict_horizon = InvalidTransactionId;
+	else
+		vm_conflict_horizon = prstate.visibility_cutoff_xid;
+
+	/*
+	 * During its second pass over the heap, VACUUM calls
+	 * heap_page_would_be_all_visible() to determine whether a page is
+	 * all-visible and all-frozen. The logic here is similar. After completing
+	 * pruning and freezing, use an assertion to verify that our results
+	 * remain consistent with heap_page_would_be_all_visible().
+	 */
+#ifdef USE_ASSERT_CHECKING
+	if (prstate.all_visible)
+	{
+		TransactionId debug_cutoff;
+		bool		debug_all_frozen;
+
+		Assert(presult->lpdead_items == 0);
+
+		Assert(heap_page_is_all_visible(params->relation, buffer,
+										prstate.cutoffs->OldestXmin,
+										&debug_all_frozen,
+										&debug_cutoff, off_loc));
+
+		Assert(prstate.all_frozen == debug_all_frozen);
+
+		Assert(!TransactionIdIsValid(debug_cutoff) ||
+			   debug_cutoff == vm_conflict_horizon);
+	}
+#endif
+
+	Assert(!prstate.all_frozen || prstate.all_visible);
 	Assert(!prstate.all_visible || (prstate.lpdead_items == 0));
 
-	do_set_vm = false;
-	if (prstate.attempt_update_vm)
-		do_set_vm = heap_page_will_set_vm(params->relation,
-										  blockno,
-										  buffer,
-										  vmbuffer,
-										  params->blk_known_av,
-										  presult,
-										  &presult->new_vmbits);
+	/*
+	 * Decide whether to set the page-level PD_ALL_VISIBLE bit and the VM bits
+	 * based on information from the VM and the all_visible/all_frozen flags.
+	 *
+	 * While it is valid for PD_ALL_VISIBLE to be set when the corresponding
+	 * VM bit is clear, we strongly prefer to keep them in sync.
+	 *
+	 * Accordingly, we also allow updating only the VM when PD_ALL_VISIBLE has
+	 * already been set. Setting only the VM is most common when setting an
+	 * already all-visible page all-frozen.
+	 */
+	do_set_vm = heap_page_will_set_vm(params->relation,
+									  blockno,
+									  buffer,
+									  vmbuffer,
+									  params->blk_known_av,
+									  &prstate,
+									  &presult->new_vmbits);
 
 	/*
 	 * new_vmbits should be 0 regardless of whether or not the page is
@@ -1169,7 +1231,7 @@ heap_page_prune_and_freeze(PruneFreezeParams *params,
 		MarkBufferDirty(buffer);
 		presult->old_vmbits = visibilitymap_set(params->relation, blockno, buffer,
 												InvalidXLogRecPtr,
-												vmbuffer, presult->vm_conflict_horizon,
+												vmbuffer, vm_conflict_horizon,
 												presult->new_vmbits);
 	}
 }
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 86822778abc..9f404e03869 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -464,20 +464,6 @@ static void dead_items_add(LVRelState *vacrel, BlockNumber blkno, OffsetNumber *
 static void dead_items_reset(LVRelState *vacrel);
 static void dead_items_cleanup(LVRelState *vacrel);
 
-#ifdef USE_ASSERT_CHECKING
-static bool heap_page_is_all_visible(Relation rel, Buffer buf,
-									 TransactionId OldestXmin,
-									 bool *all_frozen,
-									 TransactionId *visibility_cutoff_xid,
-									 OffsetNumber *logging_offnum);
-#endif
-static bool heap_page_would_be_all_visible(Relation rel, Buffer buf,
-										   TransactionId OldestXmin,
-										   OffsetNumber *deadoffsets,
-										   int ndeadoffsets,
-										   bool *all_frozen,
-										   TransactionId *visibility_cutoff_xid,
-										   OffsetNumber *logging_offnum);
 static void update_relstats_all_indexes(LVRelState *vacrel);
 static void vacuum_error_callback(void *arg);
 static void update_vacuum_error_info(LVRelState *vacrel,
@@ -2016,32 +2002,6 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->new_frozen_tuple_pages++;
 	}
 
-	/*
-	 * VACUUM will call heap_page_is_all_visible() during the second pass over
-	 * the heap to determine all_visible and all_frozen for the page -- this
-	 * is a specialized version of the logic from this function.  Now that
-	 * we've finished pruning and freezing, make sure that we're in total
-	 * agreement with heap_page_is_all_visible() using an assertion.
-	 */
-#ifdef USE_ASSERT_CHECKING
-	if (presult.all_visible)
-	{
-		TransactionId debug_cutoff;
-		bool		debug_all_frozen;
-
-		Assert(presult.lpdead_items == 0);
-
-		Assert(heap_page_is_all_visible(vacrel->rel, buf,
-										vacrel->cutoffs.OldestXmin, &debug_all_frozen,
-										&debug_cutoff, &vacrel->offnum));
-
-		Assert(presult.all_frozen == debug_all_frozen);
-
-		Assert(!TransactionIdIsValid(debug_cutoff) ||
-			   debug_cutoff == presult.vm_conflict_horizon);
-	}
-#endif
-
 	/*
 	 * Now save details of the LP_DEAD items from the page in vacrel
 	 */
@@ -3496,29 +3456,6 @@ dead_items_cleanup(LVRelState *vacrel)
 	vacrel->pvs = NULL;
 }
 
-#ifdef USE_ASSERT_CHECKING
-
-/*
- * Wrapper for heap_page_would_be_all_visible() which can be used for callers
- * that expect no LP_DEAD on the page. Currently assert-only, but there is no
- * reason not to use it outside of asserts.
- */
-static bool
-heap_page_is_all_visible(Relation rel, Buffer buf,
-						 TransactionId OldestXmin,
-						 bool *all_frozen,
-						 TransactionId *visibility_cutoff_xid,
-						 OffsetNumber *logging_offnum)
-{
-
-	return heap_page_would_be_all_visible(rel, buf,
-										  OldestXmin,
-										  NULL, 0,
-										  all_frozen,
-										  visibility_cutoff_xid,
-										  logging_offnum);
-}
-#endif
 
 /*
  * Check whether the heap page in buf is all-visible except for the dead
@@ -3542,15 +3479,12 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
  *  - *logging_offnum: OffsetNumber of current tuple being processed;
  *     used by vacuum's error callback system.
  *
- * Callers looking to verify that the page is already all-visible can call
- * heap_page_is_all_visible().
- *
  * This logic is closely related to heap_prune_record_unchanged_lp_normal().
  * If you modify this function, ensure consistency with that code. An
  * assertion cross-checks that both remain in agreement. Do not introduce new
  * side-effects.
  */
-static bool
+bool
 heap_page_would_be_all_visible(Relation rel, Buffer buf,
 							   TransactionId OldestXmin,
 							   OffsetNumber *deadoffsets,
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index bb712c5b29f..392af6503da 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -263,8 +263,7 @@ typedef struct PruneFreezeParams
 	 * HEAP_PAGE_PRUNE_MARK_UNUSED_NOW indicates that dead items can be set
 	 * LP_UNUSED during pruning.
 	 *
-	 * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples, and
-	 * will return 'all_visible', 'all_frozen' flags to the caller.
+	 * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples
 	 *
 	 * HEAP_PAGE_PRUNE_UPDATE_VM indicates that we will set the page's status
 	 * in the VM.
@@ -300,21 +299,6 @@ typedef struct PruneFreezeResult
 	int			live_tuples;
 	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.
-	 *
-	 * 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.
-	 *
-	 * These are only set if the HEAP_PRUNE_FREEZE option is set.
-	 */
-	bool		all_visible;
-	bool		all_frozen;
-	TransactionId vm_conflict_horizon;
-
 	/*
 	 * old_vmbits are the state of the all-visible and all-frozen bits in the
 	 * visibility map before updating it during phase I of vacuuming.
@@ -460,6 +444,13 @@ extern void log_heap_prune_and_freeze(Relation relation, Buffer buffer,
 /* in heap/vacuumlazy.c */
 extern void heap_vacuum_rel(Relation rel,
 							const VacuumParams params, BufferAccessStrategy bstrategy);
+extern bool heap_page_would_be_all_visible(Relation rel, Buffer buf,
+										   TransactionId OldestXmin,
+										   OffsetNumber *deadoffsets,
+										   int ndeadoffsets,
+										   bool *all_frozen,
+										   TransactionId *visibility_cutoff_xid,
+										   OffsetNumber *logging_offnum);
 
 /* in heap/heapam_visibility.c */
 extern bool HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot,
-- 
2.43.0