From 93292c7104856f069b8f94d6ec31832e6f4f977c Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sun, 7 Feb 2021 19:24:03 -0800
Subject: [PATCH v1 2/2] Add pages_newly_deleted to VACUUM VERBOSE.

pages_newly_deleted reports on the number of pages deleted by the
current VACUUM operation.  The pages_deleted field continues to report
on the total number of deleted pages in the index (as well as pages that
are recyclable due to being zeroed in rare cases), without regard to
whether or not this VACUUM operation deleted them.
---
 src/include/access/genam.h            | 12 ++++---
 src/include/access/nbtree.h           |  3 +-
 src/backend/access/gin/ginvacuum.c    |  1 +
 src/backend/access/gist/gistvacuum.c  |  2 ++
 src/backend/access/heap/vacuumlazy.c  |  6 ++--
 src/backend/access/nbtree/nbtpage.c   | 46 +++++++++++++++++++--------
 src/backend/access/nbtree/nbtree.c    | 27 ++++++++++++----
 src/backend/access/spgist/spgvacuum.c |  1 +
 8 files changed, 70 insertions(+), 28 deletions(-)

diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 0eab1508d3..09a9aa3c29 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -64,10 +64,11 @@ typedef struct IndexVacuumInfo
  * to communicate additional private data to amvacuumcleanup.
  *
  * Note: pages_removed is the amount by which the index physically shrank,
- * if any (ie the change in its total size on disk).  pages_deleted and
- * pages_free refer to free space within the index file.  Some index AMs
- * may compute num_index_tuples by reference to num_heap_tuples, in which
- * case they should copy the estimated_count field from IndexVacuumInfo.
+ * if any (ie the change in its total size on disk).  pages_deleted,
+ * pages_newly_deleted, and pages_free refer to free space within the index
+ * file.  Some index AMs may compute num_index_tuples by reference to
+ * num_heap_tuples, in which case they should copy the estimated_count field
+ * from IndexVacuumInfo.
  */
 typedef struct IndexBulkDeleteResult
 {
@@ -76,7 +77,8 @@ typedef struct IndexBulkDeleteResult
 	bool		estimated_count;	/* num_index_tuples is an estimate */
 	double		num_index_tuples;	/* tuples remaining */
 	double		tuples_removed; /* # removed during vacuum operation */
-	BlockNumber pages_deleted;	/* # unused pages in index */
+	BlockNumber pages_deleted;	/* # pages marked deleted (could be by us) */
+	BlockNumber pages_newly_deleted; /* # pages marked deleted by us  */
 	BlockNumber pages_free;		/* # pages available for reuse */
 } IndexBulkDeleteResult;
 
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 17083e9d76..897368b4d3 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1149,7 +1149,8 @@ extern void _bt_delitems_delete_check(Relation rel, Buffer buf,
 									  Relation heapRel,
 									  TM_IndexDeleteOp *delstate);
 extern uint32 _bt_pagedel(Relation rel, Buffer leafbuf,
-						  FullTransactionId *oldestSafeXid);
+						  FullTransactionId *oldestSafeXid,
+						  BlockNumber *ndeletedcount);
 
 /*
  * prototypes for functions in nbtsearch.c
diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
index 35b85a9bff..7504f57a03 100644
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -232,6 +232,7 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
 	END_CRIT_SECTION();
 
 	gvs->result->pages_deleted++;
+	gvs->result->pages_newly_deleted++;
 }
 
 
diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c
index 94a7e12763..2935491ec9 100644
--- a/src/backend/access/gist/gistvacuum.c
+++ b/src/backend/access/gist/gistvacuum.c
@@ -139,6 +139,7 @@ gistvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
 	stats->estimated_count = false;
 	stats->num_index_tuples = 0;
 	stats->pages_deleted = 0;
+	stats->pages_newly_deleted = 0;
 	stats->pages_free = 0;
 
 	/*
@@ -640,6 +641,7 @@ gistdeletepage(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
 	MarkBufferDirty(leafBuffer);
 	GistPageSetDeleted(leafPage, txid);
 	stats->pages_deleted++;
+	stats->pages_newly_deleted++;
 
 	/* remove the downlink from the parent */
 	MarkBufferDirty(parentBuffer);
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index f3d2265fad..addf243e40 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2521,10 +2521,12 @@ lazy_cleanup_index(Relation indrel,
 						(*stats)->num_index_tuples,
 						(*stats)->num_pages),
 				 errdetail("%.0f index row versions were removed.\n"
-						   "%u index pages have been deleted, %u are currently reusable.\n"
+						   "%u index pages have been deleted, %u are newly deleted, %u are currently reusable.\n"
 						   "%s.",
 						   (*stats)->tuples_removed,
-						   (*stats)->pages_deleted, (*stats)->pages_free,
+						   (*stats)->pages_deleted,
+						   (*stats)->pages_newly_deleted,
+						   (*stats)->pages_free,
 						   pg_rusage_show(&ru0))));
 	}
 
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 86652fff29..746033612e 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -51,7 +51,7 @@ static bool _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf,
 									 BlockNumber scanblkno,
 									 bool *rightsib_empty,
 									 FullTransactionId *oldestSafeXid,
-									 uint32 *ndeleted);
+									 BlockNumber *ndeletedcount);
 static bool _bt_lock_subtree_parent(Relation rel, BlockNumber child,
 									BTStack stack,
 									Buffer *subtreeparent,
@@ -1787,11 +1787,24 @@ _bt_rightsib_halfdeadflag(Relation rel, BlockNumber leafrightsib)
  * should never pass a buffer containing an existing deleted page here.  The
  * lock and pin on caller's buffer will be dropped before we return.
  *
- * Returns the number of pages successfully deleted (zero if page cannot
- * be deleted now; could be more than one if parent or right sibling pages
- * were deleted too).  Note that this does not include pages that we delete
- * that the btvacuumscan scan has yet to reach; they'll get counted later
- * instead.
+ * Returns the number of pages successfully physically deleted (zero if page
+ * cannot be deleted now; could be more than one if parent or right sibling
+ * pages were deleted too).  Caller uses return value to maintain bulk stats'
+ * pages_newly_deleted value.
+ *
+ * Maintains *ndeletedcount for caller, which is ultimately used as the
+ * pages_deleted value in bulk delete stats for entire VACUUM.  When we're
+ * called *ndeletedcount is the current total count of pages deleted in the
+ * index prior to current scanblkno block/position in btvacuumscan.  This
+ * includes existing deleted pages (pages deleted by a previous VACUUM), and
+ * pages that we delete during current VACUUM.  We need to cooperate closely
+ * with caller here so that whole VACUUM operation reliably avoids any double
+ * counting of subsidiary-to-leafbuf pages that we delete in passing.  If such
+ * pages happen to be from a block number that is ahead of the current
+ * scanblkno position, then caller is expected to count them directly later
+ * on.  It's simpler for us to understand caller's requirements than it would
+ * be for caller to understand when or how a deleted page became deleted after
+ * the fact.
  *
  * Maintains *oldestSafeXid for any pages that get deleted.  Caller is
  * responsible for maintaining *oldestSafeXid in the case of pages that were
@@ -1803,7 +1816,8 @@ _bt_rightsib_halfdeadflag(Relation rel, BlockNumber leafrightsib)
  * frequently.
  */
 uint32
-_bt_pagedel(Relation rel, Buffer leafbuf, FullTransactionId *oldestSafeXid)
+_bt_pagedel(Relation rel, Buffer leafbuf, FullTransactionId *oldestSafeXid,
+			BlockNumber *ndeletedcount)
 {
 	uint32		ndeleted = 0;
 	BlockNumber rightsib;
@@ -1813,7 +1827,7 @@ _bt_pagedel(Relation rel, Buffer leafbuf, FullTransactionId *oldestSafeXid)
 
 	/*
 	 * Save original leafbuf block number from caller.  Only deleted blocks
-	 * that are <= scanblkno get counted in ndeleted return value.
+	 * that are <= scanblkno are accounted for by *ndeletedcount.
 	 */
 	BlockNumber scanblkno = BufferGetBlockNumber(leafbuf);
 
@@ -2012,7 +2026,7 @@ _bt_pagedel(Relation rel, Buffer leafbuf, FullTransactionId *oldestSafeXid)
 			/* Check for interrupts in _bt_unlink_halfdead_page */
 			if (!_bt_unlink_halfdead_page(rel, leafbuf, scanblkno,
 										  &rightsib_empty, oldestSafeXid,
-										  &ndeleted))
+										  ndeletedcount))
 			{
 				/*
 				 * _bt_unlink_halfdead_page should never fail, since we
@@ -2025,6 +2039,8 @@ _bt_pagedel(Relation rel, Buffer leafbuf, FullTransactionId *oldestSafeXid)
 				Assert(false);
 				return ndeleted;
 			}
+			ndeleted++;
+			/* _bt_unlink_halfdead_page probably incremented ndeletedcount */
 		}
 
 		Assert(P_ISLEAF(opaque) && P_ISDELETED(opaque) &&
@@ -2304,7 +2320,8 @@ _bt_mark_page_halfdead(Relation rel, Buffer leafbuf, BTStack stack)
 static bool
 _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf, BlockNumber scanblkno,
 						 bool *rightsib_empty,
-						 FullTransactionId *oldestSafeXid, uint32 *ndeleted)
+						 FullTransactionId *oldestSafeXid,
+						 BlockNumber *ndeletedcount)
 {
 	BlockNumber leafblkno = BufferGetBlockNumber(leafbuf);
 	BlockNumber leafleftsib;
@@ -2719,12 +2736,15 @@ _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf, BlockNumber scanblkno,
 		*oldestSafeXid = safexid;
 
 	/*
+	 * Maintain ndeletedcount for entire call to _bt_pagedel.  Used to
+	 * maintain pages_deleted bulk delete stats for entire VACUUM operation.
+	 *
 	 * If btvacuumscan won't revisit this page in a future btvacuumpage call
-	 * and count it as deleted then, we count it as deleted by current
-	 * btvacuumpage call
+	 * and count it as deleted then, we count it as deleted in pages_deleted
+	 * by current btvacuumpage call.
 	 */
 	if (target <= scanblkno)
-		(*ndeleted)++;
+		(*ndeletedcount)++;
 
 	return true;
 }
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 27b41a4979..e5d6c5768d 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -981,6 +981,7 @@ btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
 	stats->estimated_count = false;
 	stats->num_index_tuples = 0;
 	stats->pages_deleted = 0;
+	stats->pages_newly_deleted = 0;
 
 	/* Set up info to pass down to btvacuumpage */
 	vstate.info = info;
@@ -1076,7 +1077,11 @@ btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
 	 *
 	 * Note that pages placed in the FSM are reported as deleted pages in the
 	 * bulk delete statistics, despite not counting as deleted pages for the
-	 * purposes of determining the oldest safexid.
+	 * purposes of determining the oldest safexid.  We generally expect that
+	 * the oldest safexid will come from one of the deleted pages that gets
+	 * counted in pages_newly_deleted.  In rare cases it will come from a page
+	 * that was marked deleted by a previous VACUUM operation which was not
+	 * safe to place in the FSM, even during this VACUUM operation.
 	 */
 	_bt_update_meta_cleanup_info(rel, vstate.oldestSafeXid,
 								 info->num_heap_tuples);
@@ -1178,8 +1183,8 @@ backtrack:
 		 * since _bt_pagedel() sometimes deletes the right sibling page of
 		 * scanblkno in passing (it does so after we decided where to
 		 * backtrack to).  We don't need to process this page as a deleted
-		 * page a second time now (in fact, it would be wrong to count it as a
-		 * deleted page in the bulk delete statistics a second time).
+		 * page a second time now (in fact, it would be wrong to double count
+		 * it in the pages_deleted field from bulk delete statistics).
 		 */
 		if (opaque->btpo_cycleid != vstate->cycleid || P_ISDELETED(opaque))
 		{
@@ -1436,12 +1441,20 @@ backtrack:
 		oldcontext = MemoryContextSwitchTo(vstate->pagedelcontext);
 
 		/*
-		 * We trust the _bt_pagedel return value because it does not include
-		 * any page that a future call here from btvacuumscan is expected to
-		 * count.  There will be no double-counting.
+		 * _bt_pagedel return value is simply the number of pages directly
+		 * deleted on each call.  This is just added to pages_newly_deleted,
+		 * which counts the number of pages marked deleted in current VACUUM.
+		 *
+		 * We need to maintain pages_deleted more carefully here, though.  We
+		 * cannot just add the same _bt_pagedel return value to pages_deleted
+		 * because that double-counts pages that are deleted within
+		 * _bt_pagedel that will become scanblkno in a later call here from
+		 * btvacuumscan.
 		 */
 		Assert(blkno == scanblkno);
-		stats->pages_deleted += _bt_pagedel(rel, buf, &vstate->oldestSafeXid);
+		stats->pages_newly_deleted += _bt_pagedel(rel, buf,
+												  &vstate->oldestSafeXid,
+												  &stats->pages_deleted);
 
 		MemoryContextSwitchTo(oldcontext);
 		/* pagedel released buffer, so we shouldn't */
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c
index 0d02a02222..a9ffca5183 100644
--- a/src/backend/access/spgist/spgvacuum.c
+++ b/src/backend/access/spgist/spgvacuum.c
@@ -891,6 +891,7 @@ spgvacuumscan(spgBulkDeleteState *bds)
 
 	/* Report final stats */
 	bds->stats->num_pages = num_pages;
+	bds->stats->pages_newly_deleted = bds->stats->pages_deleted;
 	bds->stats->pages_free = bds->stats->pages_deleted;
 }
 
-- 
2.27.0

