From d2190abf366f148bae5307442e8a6245c6922e78 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Mon, 21 Feb 2022 12:46:44 -0800
Subject: [PATCH v9 3/4] Remove aggressive VACUUM skipping special case.

Since it's simply never okay to miss out on advancing relfrozenxid
during an aggressive VACUUM (that's the whole point), the aggressive
case treated any page from a next_unskippable_block-wise skippable block
range as an all-frozen page (not a merely all-visible page) during
skipping.  Such a page might not be all-visible/all-frozen at the point
that it actually gets skipped, but it could nevertheless be safely
skipped, and then counted in frozenskipped_pages (the page must have
been all-frozen back when we determined the extent of the range of
blocks to skip, since aggressive VACUUMs _must_ scan all-visible pages).
This is necessary to ensure that aggressive VACUUMs are always capable
of advancing relfrozenxid.

The non-aggressive case behaved slightly differently: it rechecked the
visibility map for each page at the point of skipping, and only counted
pages in frozenskipped_pages when they were still all-frozen at that
time.  But it skipped the page either way (since we already committed to
skipping the page at the point of the recheck).  This was correct, but
sometimes resulted in non-aggressive VACUUMs needlessly wasting an
opportunity to advance relfrozenxid (when a page was modified in just
the wrong way, at just the wrong time).  It also resulted in a needless
recheck of the visibility map for each and every page skipped during
non-aggressive VACUUMs.

Avoid these problems by conditioning the "skippable page was definitely
all-frozen when range of skippable pages was first determined" behavior
on what the visibility map _actually said_ about the range as a whole
back when we first determined the extent of the range (don't deduce what
must have happened at that time on the basis of aggressive-ness).  This
allows us to reliably count skipped pages in frozenskipped_pages when
they were initially all-frozen.  In particular, when a page's visibility
map bit is unset after the point where a skippable range of pages is
initially determined, but before the point where the page is actually
skipped, non-aggressive VACUUMs now count it in frozenskipped_pages,
just like aggressive VACUUMs always have [1].  It's not critical for the
non-aggressive case to get this right, but there is no reason not to.

[1] Actually, it might not work that way when there happens to be a mix
of all-visible and all-frozen pages in a range of skippable pages.
There is no chance of VACUUM advancing relfrozenxid in this scenario
either way, though, so it doesn't matter.
---
 src/backend/access/heap/vacuumlazy.c | 59 +++++++++++++++++++---------
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index f14b64dfc..b2d3b039d 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -542,7 +542,14 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	 */
 	if (vacrel->scanned_pages + vacrel->frozenskipped_pages < orig_rel_pages)
 	{
-		/* Cannot advance relfrozenxid/relminmxid */
+		/*
+		 * Skipped some all-visible pages, so definitely cannot advance
+		 * relfrozenxid.  This is generally only expected in pg_upgrade
+		 * scenarios, since VACUUM now avoids setting a page to all-visible
+		 * but not all-frozen.  However, it's also possible (though quite
+		 * unlikely) that we ended up here because somebody else cleared some
+		 * page's all-frozen flag (without clearing its all-visible flag).
+		 */
 		Assert(!aggressive);
 		frozenxid_updated = minmulti_updated = false;
 		vac_update_relstats(rel, new_rel_pages, new_live_tuples,
@@ -810,7 +817,8 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
 				next_failsafe_block,
 				next_fsm_block_to_vacuum;
 	Buffer		vmbuffer = InvalidBuffer;
-	bool		skipping_blocks;
+	bool		skipping_blocks,
+				skipping_allfrozen_blocks;
 	const int	initprog_index[] = {
 		PROGRESS_VACUUM_PHASE,
 		PROGRESS_VACUUM_TOTAL_HEAP_BLKS,
@@ -905,27 +913,31 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
 	 * computed, so they'll have no effect on the value to which we can safely
 	 * set relfrozenxid.  A similar argument applies for MXIDs and relminmxid.
 	 */
+	skipping_allfrozen_blocks = true;	/* iff skipping_blocks */
 	next_unskippable_block = 0;
 	if (vacrel->skipwithvm)
 	{
 		while (next_unskippable_block < nblocks)
 		{
-			uint8		vmstatus;
+			uint8		vmskipflags;
 
-			vmstatus = visibilitymap_get_status(vacrel->rel,
-												next_unskippable_block,
-												&vmbuffer);
+			vmskipflags = visibilitymap_get_status(vacrel->rel,
+												   next_unskippable_block,
+												   &vmbuffer);
 			if (vacrel->aggressive)
 			{
-				if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0)
+				if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
 					break;
 			}
 			else
 			{
-				if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0)
+				if ((vmskipflags & VISIBILITYMAP_ALL_VISIBLE) == 0)
 					break;
 			}
 			vacuum_delay_point();
+
+			if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
+				skipping_allfrozen_blocks = false;
 			next_unskippable_block++;
 		}
 	}
@@ -949,6 +961,8 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
 
 		if (blkno == next_unskippable_block)
 		{
+			skipping_allfrozen_blocks = true;	/* iff skipping_blocks */
+
 			/* Time to advance next_unskippable_block */
 			next_unskippable_block++;
 			if (vacrel->skipwithvm)
@@ -971,6 +985,9 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
 							break;
 					}
 					vacuum_delay_point();
+
+					if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
+						skipping_allfrozen_blocks = false;
 					next_unskippable_block++;
 				}
 			}
@@ -997,8 +1014,11 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
 		{
 			/*
 			 * The current page can be skipped if we've seen a long enough run
-			 * of skippable blocks to justify skipping it -- provided it's not
-			 * the last page in the relation (according to rel_pages/nblocks).
+			 * of skippable blocks to justify skipping it.  An aggressive
+			 * VACUUM can only skip a range of blocks that were determined to
+			 * be all-frozen (not just all-visible) as a group back when the
+			 * next_unskippable_block-wise extent of the range was determined.
+			 * Assert that we got this right in passing.
 			 *
 			 * We always scan the table's last page to determine whether it
 			 * has tuples or not, even if it would otherwise be skipped. This
@@ -1006,19 +1026,20 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
 			 * on the table to attempt a truncation that just fails
 			 * immediately because there are tuples on the last page.
 			 */
+			Assert(!vacrel->aggressive || !skipping_blocks ||
+				   skipping_allfrozen_blocks);
 			if (skipping_blocks && blkno < nblocks - 1)
 			{
 				/*
-				 * Tricky, tricky.  If this is in aggressive vacuum, the page
-				 * must have been all-frozen at the time we checked whether it
-				 * was skippable, but it might not be any more.  We must be
-				 * careful to count it as a skipped all-frozen page in that
-				 * case, or else we'll think we can't update relfrozenxid and
-				 * relminmxid.  If it's not an aggressive vacuum, we don't
-				 * know whether it was initially all-frozen, so we have to
-				 * recheck.
+				 * When skipping a range of blocks with one or more blocks
+				 * that are not all-frozen (expected during a non-aggressive
+				 * VACUUM following pg_upgrade), we need to recheck if this
+				 * block is all-frozen to maintain frozenskipped_pages.  The
+				 * block might not even be all-visible by now, but it's always
+				 * okay to skip (see note above about visibilitymap_get_status
+				 * return value being out-of-date).
 				 */
-				if (vacrel->aggressive ||
+				if (skipping_allfrozen_blocks ||
 					VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))
 					vacrel->frozenskipped_pages++;
 				continue;
-- 
2.30.2

