From 991c5a7ed46cc5dee36352194058ffb06a4e8670 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Sat, 30 Dec 2023 16:59:27 -0500
Subject: [PATCH v7 3/7] Confine vacuum skip logic to lazy_scan_skip

In preparation for vacuum to use the streaming read interface [1] (and
eventually AIO), refactor vacuum's logic for skipping blocks such that
it is entirely confined to lazy_scan_skip(). This turns lazy_scan_skip()
and its next block state in LVRelState into an iterator which yields
blocks to lazy_scan_heap(). Such a structure is conducive to an async
interface. While we are at it, rename lazy_scan_skip() to
heap_vac_scan_next_block(), which now more accurately describes it.

By always calling heap_vac_scan_next_block(), instead of only when we
have reached the next unskippable block, we no longer need the
skipping_current_range variable. Furthermore, lazy_scan_heap() no longer
needs to manage the skipped range by checking if we reached the end in
order to then call heap_vac_scan_next_block(). And
heap_vac_scan_next_block() can derive the visibility status of a block
from whether or not we are in a skippable range; that is, if the next
block is equal to the next unskippable block, then the block isn't all
visible, otherwise it is.

[1] https://postgr.es/m/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2gkvA%40mail.gmail.com

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

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index accc6303fa2..8d715caccc1 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -206,19 +206,20 @@ typedef struct LVRelState
 	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.
+	 * Parameters maintained by heap_vac_scan_next_block() to manage getting
+	 * the next block for vacuum to process.
 	 */
 	struct
 	{
-		/* The last block lazy_scan_skip() returned and vacuum processed */
+		/*
+		 * The last block heap_vac_scan_next_block() 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;
 
@@ -232,7 +233,9 @@ typedef struct LVSavedErrInfo
 
 /* non-export function prototypes */
 static void lazy_scan_heap(LVRelState *vacrel);
-static void lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer);
+static bool heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer,
+									 BlockNumber *blkno,
+									 bool *all_visible_according_to_vm);
 static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf,
 								   BlockNumber blkno, Page page,
 								   bool sharelock, Buffer vmbuffer);
@@ -816,6 +819,8 @@ lazy_scan_heap(LVRelState *vacrel)
 	BlockNumber rel_pages = vacrel->rel_pages,
 				blkno,
 				next_fsm_block_to_vacuum = 0;
+	bool		all_visible_according_to_vm;
+
 	VacDeadItems *dead_items = vacrel->dead_items;
 	Buffer		vmbuffer = InvalidBuffer;
 	const int	initprog_index[] = {
@@ -831,41 +836,18 @@ 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 */
+	/* Initialize for first heap_vac_scan_next_block() 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 */
-	lazy_scan_skip(vacrel, &vmbuffer);
-	for (blkno = 0; blkno < rel_pages; blkno++)
+	while (heap_vac_scan_next_block(vacrel, &vmbuffer,
+									&blkno, &all_visible_according_to_vm))
 	{
 		Buffer		buf;
 		Page		page;
-		bool		all_visible_according_to_vm;
 		bool		has_lpdead_items;
 		bool		got_cleanup_lock = false;
 
-		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 = 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 (vacrel->next_block_state.skipping_current_range)
-				continue;
-
-			/* Current range is too small to skip -- just scan the page */
-			all_visible_according_to_vm = true;
-		}
-
 		vacrel->scanned_pages++;
 
 		/* Report as block scanned, update error traceback information */
@@ -1086,10 +1068,16 @@ lazy_scan_heap(LVRelState *vacrel)
 }
 
 /*
- *	lazy_scan_skip() -- set up range of skippable blocks using visibility map.
+ *	heap_vac_scan_next_block() -- get next block for vacuum to process
+ *
+ * lazy_scan_heap() calls here every time it needs to get the next block to
+ * prune and vacuum, using the visibility map, vacuum options, and various
+ * thresholds to skip blocks which do not need to be processed and set blkno to
+ * the next block that actually needs to be processed.
  *
- * lazy_scan_heap() calls here every time it needs to set up a new range of
- * blocks to skip via the visibility map.
+ * The block number and visibility status of the next block to process are set
+ * in blkno and all_visible_according_to_vm. heap_vac_scan_next_block()
+ * returns false if there are no further blocks to process.
  *
  * 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
@@ -1112,19 +1100,14 @@ 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 void
-lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer)
+static bool
+heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer,
+						 BlockNumber *blkno, bool *all_visible_according_to_vm)
 {
-	/* 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;
 
-	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
@@ -1144,88 +1127,107 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer)
 	 * indicates to the caller whether or not it is processing a skippable
 	 * (and thus all-visible) block.
 	 */
-	while (next_unskippable_block < rel_pages)
+	if (next_block >= vacrel->rel_pages)
 	{
-		uint8		mapbits = visibilitymap_get_status(vacrel->rel,
-													   next_unskippable_block,
-													   vmbuffer);
+		vacrel->next_block_state.current_block = *blkno = InvalidBlockNumber;
+		return false;
+	}
+
+	if (vacrel->next_block_state.next_unskippable_block == InvalidBlockNumber ||
+		next_block > vacrel->next_block_state.next_unskippable_block)
+	{
+		/* Use local variables for better optimized loop code */
+		BlockNumber rel_pages = vacrel->rel_pages;
+		BlockNumber next_unskippable_block = vacrel->next_block_state.next_unskippable_block;
 
-		if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
+		while (++next_unskippable_block < rel_pages)
 		{
-			Assert((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0);
-			vacrel->next_block_state.next_unskippable_allvis = false;
-			break;
-		}
+			uint8		mapbits = visibilitymap_get_status(vacrel->rel,
+														   next_unskippable_block,
+														   vmbuffer);
 
-		/*
-		 * Caller must scan the last page to determine whether it has tuples
-		 * (caller must have the opportunity to set vacrel->nonempty_pages).
-		 * This rule avoids having lazy_truncate_heap() take access-exclusive
-		 * lock on rel to attempt a truncation that fails anyway, just because
-		 * there are tuples on the last page (it is likely that there will be
-		 * tuples on other nearby pages as well, but those can be skipped).
-		 *
-		 * Implement this by always treating the last block as unsafe to skip.
-		 */
-		if (next_unskippable_block == rel_pages - 1)
-			break;
+			vacrel->next_block_state.next_unskippable_allvis = mapbits & VISIBILITYMAP_ALL_VISIBLE;
 
-		/* DISABLE_PAGE_SKIPPING makes all skipping unsafe */
-		if (!vacrel->skipwithvm)
-		{
-			/* Caller shouldn't rely on all_visible_according_to_vm */
-			vacrel->next_block_state.next_unskippable_allvis = false;
-			break;
-		}
+			if (!vacrel->next_block_state.next_unskippable_allvis)
+			{
+				Assert((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0);
+				break;
+			}
 
-		/*
-		 * Aggressive VACUUM caller can't skip pages just because they are
-		 * all-visible.  They may still skip all-frozen pages, which can't
-		 * contain XIDs < OldestXmin (XIDs that aren't already frozen by now).
-		 */
-		if ((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0)
-		{
-			if (vacrel->aggressive)
+			/*
+			 * Caller must scan the last page to determine whether it has
+			 * tuples (caller must have the opportunity to set
+			 * vacrel->nonempty_pages). This rule avoids having
+			 * lazy_truncate_heap() take access-exclusive lock on rel to
+			 * attempt a truncation that fails anyway, just because there are
+			 * tuples on the last page (it is likely that there will be tuples
+			 * on other nearby pages as well, but those can be skipped).
+			 *
+			 * Implement this by always treating the last block as unsafe to
+			 * skip.
+			 */
+			if (next_unskippable_block == rel_pages - 1)
 				break;
 
+			/* DISABLE_PAGE_SKIPPING makes all skipping unsafe */
+			if (!vacrel->skipwithvm)
+			{
+				/* Caller shouldn't rely on all_visible_according_to_vm */
+				vacrel->next_block_state.next_unskippable_allvis = false;
+				break;
+			}
+
 			/*
-			 * All-visible block is safe to skip in non-aggressive case.  But
-			 * remember that the final range contains such a block for later.
+			 * Aggressive VACUUM caller can't skip pages just because they are
+			 * all-visible.  They may still skip all-frozen pages, which can't
+			 * contain XIDs < OldestXmin (XIDs that aren't already frozen by
+			 * now).
 			 */
-			skipsallvis = true;
-		}
+			if ((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0)
+			{
+				if (vacrel->aggressive)
+					break;
 
-		vacuum_delay_point();
-		next_unskippable_block++;
-	}
+				/*
+				 * All-visible block is safe to skip in non-aggressive case.
+				 * But remember that the final range contains such a block for
+				 * later.
+				 */
+				skipsallvis = true;
+			}
 
-	Assert(vacrel->next_block_state.next_unskippable_block >=
-		   vacrel->next_block_state.current_block);
-	vacrel->next_block_state.next_unskippable_block = next_unskippable_block;
+			vacuum_delay_point();
+		}
 
-	/*
-	 * We only skip a range with at least SKIP_PAGES_THRESHOLD consecutive
-	 * pages.  Since we're reading sequentially, the OS should be doing
-	 * readahead for us, so there's no gain in skipping a page now and then.
-	 * Skipping such a range might even discourage sequential detection.
-	 *
-	 * This test also enables more frequent relfrozenxid advancement during
-	 * non-aggressive VACUUMs.  If the range has any all-visible pages then
-	 * skipping makes updating relfrozenxid unsafe, which is a real downside.
-	 */
-	if (vacrel->next_block_state.next_unskippable_block - next_block < SKIP_PAGES_THRESHOLD)
-		vacrel->next_block_state.skipping_current_range = false;
-	else
-	{
-		vacrel->next_block_state.skipping_current_range = true;
-		if (skipsallvis)
-			vacrel->skippedallvis = true;
+		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
+		 * readahead for us, so there's no gain in skipping a page now and
+		 * then. Skipping such a range might even discourage sequential
+		 * detection.
+		 *
+		 * This test also enables more frequent relfrozenxid advancement
+		 * during non-aggressive VACUUMs.  If the range has any all-visible
+		 * pages then skipping makes updating relfrozenxid unsafe, which is a
+		 * real downside.
+		 */
+		if (vacrel->next_block_state.next_unskippable_block - next_block >= SKIP_PAGES_THRESHOLD)
+		{
+			next_block = vacrel->next_block_state.next_unskippable_block;
+			if (skipsallvis)
+				vacrel->skippedallvis = true;
+		}
 	}
 
-	if (next_unskippable_block >= rel_pages)
-		next_block = InvalidBlockNumber;
+	if (next_block == vacrel->next_block_state.next_unskippable_block)
+		*all_visible_according_to_vm = vacrel->next_block_state.next_unskippable_allvis;
+	else
+		*all_visible_according_to_vm = true;
 
-	vacrel->next_block_state.current_block = next_block;
+	vacrel->next_block_state.current_block = *blkno = next_block;
+	return true;
 }
 
 /*
@@ -1798,8 +1800,8 @@ lazy_scan_prune(LVRelState *vacrel,
 
 	/*
 	 * Handle setting visibility map bit based on information from the VM (as
-	 * of last lazy_scan_skip() call), and from all_visible and all_frozen
-	 * variables
+	 * of last heap_vac_scan_next_block() call), and from all_visible and
+	 * all_frozen variables
 	 */
 	if (!all_visible_according_to_vm && all_visible)
 	{
@@ -1834,8 +1836,8 @@ lazy_scan_prune(LVRelState *vacrel,
 	/*
 	 * 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 lazy_scan_skip() was called, so we must recheck with
-	 * buffer lock before concluding that the VM is corrupt.
+	 * 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(page) &&
 			 visibilitymap_get_status(vacrel->rel, blkno, &vmbuffer) != 0)
-- 
2.40.1

