From 4d49028df51550af931f70c21a920a22ff09ba48 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Sat, 30 Dec 2023 16:22:12 -0500
Subject: [PATCH v7 2/7] Add lazy_scan_skip next block state to LVRelState

Future commits will remove all skipping logic from lazy_scan_heap() and
confine it to lazy_scan_skip(). To make those commits more clear, first
introduce a struct to LVRelState containing members tracking the current
block and the information needed to determine whether or not to skip
ranges less than SKIP_PAGES_THRESHOLD.

While we are at it, expand the comments in lazy_scan_skip(), including
descriptions of the role and expectations of its function parameters and
more detail on when skippable blocks are not skipped.

Discussion: https://postgr.es/m/flat/CAAKRu_Yf3gvXGcCnqqfoq0Q8LX8UM-e-qbm_B1LeZh60f8WhWA%40mail.gmail.com
---
 src/backend/access/heap/vacuumlazy.c | 124 ++++++++++++++++++---------
 1 file changed, 84 insertions(+), 40 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 1dc6cc8e4db..accc6303fa2 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -204,6 +204,22 @@ typedef struct LVRelState
 	int64		live_tuples;	/* # live tuples remaining */
 	int64		recently_dead_tuples;	/* # dead, but not yet removable */
 	int64		missed_dead_tuples; /* # removable, but not removed */
+
+	/*
+	 * Parameters maintained by lazy_scan_skip() to manage skipping ranges of
+	 * pages greater than SKIP_PAGES_THRESHOLD.
+	 */
+	struct
+	{
+		/* The last block lazy_scan_skip() returned and vacuum processed */
+		BlockNumber current_block;
+		/* Next unskippable block */
+		BlockNumber next_unskippable_block;
+		/* Next unskippable block's visibility status */
+		bool		next_unskippable_allvis;
+		/* Whether or not skippable blocks should be skipped */
+		bool		skipping_current_range;
+	}			next_block_state;
 } LVRelState;
 
 /* Struct for saving and restoring vacuum error information. */
@@ -214,13 +230,9 @@ typedef struct LVSavedErrInfo
 	VacErrPhase phase;
 } LVSavedErrInfo;
 
-
 /* non-export function prototypes */
 static void lazy_scan_heap(LVRelState *vacrel);
-static BlockNumber lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer,
-								  BlockNumber next_block,
-								  bool *next_unskippable_allvis,
-								  bool *skipping_current_range);
+static void lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer);
 static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf,
 								   BlockNumber blkno, Page page,
 								   bool sharelock, Buffer vmbuffer);
@@ -803,12 +815,9 @@ lazy_scan_heap(LVRelState *vacrel)
 {
 	BlockNumber rel_pages = vacrel->rel_pages,
 				blkno,
-				next_unskippable_block,
 				next_fsm_block_to_vacuum = 0;
 	VacDeadItems *dead_items = vacrel->dead_items;
 	Buffer		vmbuffer = InvalidBuffer;
-	bool		next_unskippable_allvis,
-				skipping_current_range;
 	const int	initprog_index[] = {
 		PROGRESS_VACUUM_PHASE,
 		PROGRESS_VACUUM_TOTAL_HEAP_BLKS,
@@ -822,10 +831,12 @@ lazy_scan_heap(LVRelState *vacrel)
 	initprog_val[2] = dead_items->max_items;
 	pgstat_progress_update_multi_param(3, initprog_index, initprog_val);
 
+	/* Initialize for first lazy_scan_skip() call */
+	vacrel->next_block_state.current_block = InvalidBlockNumber;
+	vacrel->next_block_state.next_unskippable_block = InvalidBlockNumber;
+
 	/* Set up an initial range of skippable blocks using the visibility map */
-	next_unskippable_block = lazy_scan_skip(vacrel, &vmbuffer, 0,
-											&next_unskippable_allvis,
-											&skipping_current_range);
+	lazy_scan_skip(vacrel, &vmbuffer);
 	for (blkno = 0; blkno < rel_pages; blkno++)
 	{
 		Buffer		buf;
@@ -834,26 +845,21 @@ lazy_scan_heap(LVRelState *vacrel)
 		bool		has_lpdead_items;
 		bool		got_cleanup_lock = false;
 
-		if (blkno == next_unskippable_block)
+		if (blkno == vacrel->next_block_state.next_unskippable_block)
 		{
 			/*
 			 * Can't skip this page safely.  Must scan the page.  But
 			 * determine the next skippable range after the page first.
 			 */
-			all_visible_according_to_vm = next_unskippable_allvis;
-			next_unskippable_block = lazy_scan_skip(vacrel, &vmbuffer,
-													blkno + 1,
-													&next_unskippable_allvis,
-													&skipping_current_range);
-
-			Assert(next_unskippable_block >= blkno + 1);
+			all_visible_according_to_vm = vacrel->next_block_state.next_unskippable_allvis;
+			lazy_scan_skip(vacrel, &vmbuffer);
 		}
 		else
 		{
 			/* Last page always scanned (may need to set nonempty_pages) */
 			Assert(blkno < rel_pages - 1);
 
-			if (skipping_current_range)
+			if (vacrel->next_block_state.skipping_current_range)
 				continue;
 
 			/* Current range is too small to skip -- just scan the page */
@@ -1036,7 +1042,10 @@ lazy_scan_heap(LVRelState *vacrel)
 
 	vacrel->blkno = InvalidBlockNumber;
 	if (BufferIsValid(vmbuffer))
+	{
 		ReleaseBuffer(vmbuffer);
+		vmbuffer = InvalidBuffer;
+	}
 
 	/* report that everything is now scanned */
 	pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
@@ -1080,15 +1089,20 @@ lazy_scan_heap(LVRelState *vacrel)
  *	lazy_scan_skip() -- set up range of skippable blocks using visibility map.
  *
  * lazy_scan_heap() calls here every time it needs to set up a new range of
- * blocks to skip via the visibility map.  Caller passes the next block in
- * line.  We return a next_unskippable_block for this range.  When there are
- * no skippable blocks we just return caller's next_block.  The all-visible
- * status of the returned block is set in *next_unskippable_allvis for caller,
- * too.  Block usually won't be all-visible (since it's unskippable), but it
- * can be during aggressive VACUUMs (as well as in certain edge cases).
+ * blocks to skip via the visibility map.
  *
- * Sets *skipping_current_range to indicate if caller should skip this range.
- * Costs and benefits drive our decision.  Very small ranges won't be skipped.
+ * vacrel is an in/out parameter here; vacuum options and information about the
+ * relation are read, members of vacrel->next_block_state are read and set as
+ * bookeeping for this function, and vacrel->skippedallvis is set to ensure we
+ * don't advance relfrozenxid when we have skipped vacuuming all-visible
+ * blocks.
+ *
+ * vmbuffer is an output parameter which, upon return, will contain the block
+ * from the VM containing visibility information for the next unskippable heap
+ * block. If we decide not to skip this heap block, the caller is responsible
+ * for fetching the correct VM block into vmbuffer before using it. This is
+ * okay as providing it as an output parameter is an optimization, not a
+ * requirement.
  *
  * Note: our opinion of which blocks can be skipped can go stale immediately.
  * It's okay if caller "misses" a page whose all-visible or all-frozen marking
@@ -1098,15 +1112,38 @@ lazy_scan_heap(LVRelState *vacrel)
  * older XIDs/MXIDs.  The vacrel->skippedallvis flag will be set here when the
  * choice to skip such a range is actually made, making everything safe.)
  */
-static BlockNumber
-lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
-			   bool *next_unskippable_allvis, bool *skipping_current_range)
+static void
+lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer)
 {
-	BlockNumber rel_pages = vacrel->rel_pages,
-				next_unskippable_block = next_block;
+	/* Use local variables for better optimized loop code */
+	BlockNumber rel_pages = vacrel->rel_pages;
+	/* Relies on InvalidBlockNumber + 1 == 0 */
+	BlockNumber next_block = vacrel->next_block_state.current_block + 1;
+	BlockNumber next_unskippable_block = next_block;
+
 	bool		skipsallvis = false;
 
-	*next_unskippable_allvis = true;
+	vacrel->next_block_state.next_unskippable_allvis = true;
+
+	/*
+	 * A block is unskippable if it is not all visible according to the
+	 * visibility map. It is also unskippable if it is the last block in the
+	 * relation, if the vacuum is an aggressive vacuum, or if
+	 * DISABLE_PAGE_SKIPPING was passed to vacuum.
+	 *
+	 * Even if a block is skippable, we may choose not to skip it if the range
+	 * of skippable blocks is too small (below SKIP_PAGES_THRESHOLD). As a
+	 * consequence, we must keep track of the next truly unskippable block and
+	 * its visibility status along with whether or not we are skipping the
+	 * current range of skippable blocks. This can be used to derive the next
+	 * block lazy_scan_heap() must process and its visibility status.
+	 *
+	 * The block number and visibility status of the next unskippable block
+	 * are set in next_block_state->next_unskippable_block and
+	 * next_unskippable_allvis. next_block_state->skipping_current_range
+	 * indicates to the caller whether or not it is processing a skippable
+	 * (and thus all-visible) block.
+	 */
 	while (next_unskippable_block < rel_pages)
 	{
 		uint8		mapbits = visibilitymap_get_status(vacrel->rel,
@@ -1116,7 +1153,7 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
 		if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
 		{
 			Assert((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0);
-			*next_unskippable_allvis = false;
+			vacrel->next_block_state.next_unskippable_allvis = false;
 			break;
 		}
 
@@ -1137,7 +1174,7 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
 		if (!vacrel->skipwithvm)
 		{
 			/* Caller shouldn't rely on all_visible_according_to_vm */
-			*next_unskippable_allvis = false;
+			vacrel->next_block_state.next_unskippable_allvis = false;
 			break;
 		}
 
@@ -1162,6 +1199,10 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
 		next_unskippable_block++;
 	}
 
+	Assert(vacrel->next_block_state.next_unskippable_block >=
+		   vacrel->next_block_state.current_block);
+	vacrel->next_block_state.next_unskippable_block = next_unskippable_block;
+
 	/*
 	 * We only skip a range with at least SKIP_PAGES_THRESHOLD consecutive
 	 * pages.  Since we're reading sequentially, the OS should be doing
@@ -1172,16 +1213,19 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
 	 * non-aggressive VACUUMs.  If the range has any all-visible pages then
 	 * skipping makes updating relfrozenxid unsafe, which is a real downside.
 	 */
-	if (next_unskippable_block - next_block < SKIP_PAGES_THRESHOLD)
-		*skipping_current_range = false;
+	if (vacrel->next_block_state.next_unskippable_block - next_block < SKIP_PAGES_THRESHOLD)
+		vacrel->next_block_state.skipping_current_range = false;
 	else
 	{
-		*skipping_current_range = true;
+		vacrel->next_block_state.skipping_current_range = true;
 		if (skipsallvis)
 			vacrel->skippedallvis = true;
 	}
 
-	return next_unskippable_block;
+	if (next_unskippable_block >= rel_pages)
+		next_block = InvalidBlockNumber;
+
+	vacrel->next_block_state.current_block = next_block;
 }
 
 /*
-- 
2.40.1

