From 6a2a8b7d147e3c5343512bcf4d1d8389ff71bbe8 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 5 Apr 2024 18:56:45 -0400
Subject: [PATCH v16 18/18] Remove table_scan_bitmap_next_block()

With several of the changes to the control flow of BitmapHeapNext() in
recent commits, table_scan_bitmap_next_tuple() can be responsible for
getting the next block. Do this and remove the table AM API function
table_scan_bitmap_next_block().

Author: Melanie Plageman
Discussion: https://postgr.es/m/CAAKRu_ZwCwWFeL_H3ia26bP2e7HiKLWt0ZmGXPVwPO6uXq0vaA%40mail.gmail.com
---
 src/backend/access/heap/heapam.c          |  2 +
 src/backend/access/heap/heapam_handler.c  | 53 +++++++++-------
 src/backend/access/table/tableamapi.c     |  2 -
 src/backend/executor/nodeBitmapHeapscan.c | 69 +++++++++------------
 src/backend/optimizer/util/plancat.c      |  2 +-
 src/include/access/tableam.h              | 73 +++++------------------
 src/include/nodes/execnodes.h             |  2 -
 7 files changed, 78 insertions(+), 125 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 5728b2ea8a..3ee4d75f17 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -327,6 +327,8 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
 	ItemPointerSetInvalid(&scan->rs_ctup.t_self);
 	scan->rs_cbuf = InvalidBuffer;
 	scan->rs_cblock = InvalidBlockNumber;
+	scan->rs_cindex = 0;
+	scan->rs_ntuples = 0;
 
 	/* page-at-a-time fields are always invalid when not rs_inited */
 
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index fca9c7233e..a6d75cfaa8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2216,8 +2216,8 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanDesc scan)
 
 	/*
 	 * Adjusting the prefetch iterator before invoking
-	 * table_scan_bitmap_next_block() keeps prefetch distance higher across
-	 * the parallel workers.
+	 * heapam_bitmap_next_block() keeps prefetch distance higher across the
+	 * parallel workers.
 	 */
 	if (scan->prefetch_maximum > 0)
 	{
@@ -2397,14 +2397,12 @@ BitmapPrefetch(BitmapHeapScanDesc scan)
 
 
 
-/* ------------------------------------------------------------------------
- * Executor related callbacks for the heap AM
- * ------------------------------------------------------------------------
+/*
+ * Helper for heapam_scan_bitmap_next_tuple()
  */
-
 static bool
 heapam_scan_bitmap_next_block(TableScanDesc sscan,
-							  BlockNumber *blockno, bool *recheck,
+							  bool *recheck,
 							  long *lossy_pages, long *exact_pages)
 {
 	BitmapHeapScanDesc scan = (BitmapHeapScanDesc) sscan;
@@ -2418,7 +2416,6 @@ heapam_scan_bitmap_next_block(TableScanDesc sscan,
 	hscan->rs_cindex = 0;
 	hscan->rs_ntuples = 0;
 
-	*blockno = InvalidBlockNumber;
 	*recheck = true;
 
 	BitmapAdjustPrefetchIterator(scan);
@@ -2447,7 +2444,6 @@ heapam_scan_bitmap_next_block(TableScanDesc sscan,
 	} while (!IsolationIsSerializable() && tbmres->blockno >= hscan->rs_nblocks);
 
 	/* Got a valid block */
-	*blockno = tbmres->blockno;
 	*recheck = tbmres->recheck;
 
 	/*
@@ -2585,9 +2581,16 @@ heapam_scan_bitmap_next_block(TableScanDesc sscan,
 	return true;
 }
 
+
+/* ------------------------------------------------------------------------
+ * Executor related callbacks for the heap AM
+ * ------------------------------------------------------------------------
+ */
+
 static bool
 heapam_scan_bitmap_next_tuple(TableScanDesc scan,
-							  TupleTableSlot *slot)
+							  TupleTableSlot *slot, bool *recheck,
+							  long *lossy_pages, long *exact_pages)
 {
 	BitmapHeapScanDesc bscan = (BitmapHeapScanDesc) scan;
 	HeapScanDesc hscan = &bscan->heap_common;
@@ -2595,21 +2598,28 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
 	Page		page;
 	ItemId		lp;
 
-	if (bscan->empty_tuples_pending > 0)
+	/*
+	 * Out of range?  If so, nothing more to look at on this page
+	 */
+	while (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
 	{
 		/*
-		 * If we don't have to fetch the tuple, just return nulls.
+		 * Emit empty tuples before advancing to the next block
 		 */
-		ExecStoreAllNullTuple(slot);
-		bscan->empty_tuples_pending--;
-		return true;
-	}
+		if (bscan->empty_tuples_pending > 0)
+		{
+			/*
+			 * If we don't have to fetch the tuple, just return nulls.
+			 */
+			ExecStoreAllNullTuple(slot);
+			bscan->empty_tuples_pending--;
+			return true;
+		}
 
-	/*
-	 * Out of range?  If so, nothing more to look at on this page
-	 */
-	if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
-		return false;
+		if (!heapam_scan_bitmap_next_block(scan, recheck,
+										   lossy_pages, exact_pages))
+			return false;
+	}
 
 #ifdef USE_PREFETCH
 
@@ -3010,7 +3020,6 @@ static const TableAmRoutine heapam_methods = {
 
 	.relation_estimate_size = heapam_estimate_rel_size,
 
-	.scan_bitmap_next_block = heapam_scan_bitmap_next_block,
 	.scan_bitmap_next_tuple = heapam_scan_bitmap_next_tuple,
 	.scan_sample_next_block = heapam_scan_sample_next_block,
 	.scan_sample_next_tuple = heapam_scan_sample_next_tuple
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 55b8caeadf..0b87530a9c 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -90,8 +90,6 @@ GetTableAmRoutine(Oid amhandler)
 	Assert(routine->relation_estimate_size != NULL);
 
 	/* optional, but one callback implies presence of the other */
-	Assert((routine->scan_bitmap_next_block == NULL) ==
-		   (routine->scan_bitmap_next_tuple == NULL));
 	Assert(routine->scan_sample_next_block != NULL);
 	Assert(routine->scan_sample_next_tuple != NULL);
 
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index ce47cd51bd..3ce24156dd 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,61 +211,52 @@ static TupleTableSlot *
 BitmapHeapNext(BitmapHeapScanState *node)
 {
 	ExprContext *econtext;
-	TableScanDesc scan;
 	TupleTableSlot *slot;
-
-	/*
-	 * extract necessary information from index scan node
-	 */
-	econtext = node->ss.ps.ps_ExprContext;
-	slot = node->ss.ss_ScanTupleSlot;
-	scan = node->ss.ss_currentScanDesc;
+	TableScanDesc scan;
 
 	/*
 	 * If we haven't yet performed the underlying index scan, do it, and begin
 	 * the iteration over the bitmap. This happens on rescan as well.
 	 */
 	if (!node->initialized)
-	{
 		BitmapHeapInitialize(node);
-		/* We may have a new scan descriptor */
-		scan = node->ss.ss_currentScanDesc;
-		goto new_page;
-	}
 
-	for (;;)
+	/*
+	 * BitmapHeapInitialize() may make the scan descriptor so don't get it
+	 * from the node until after calling it.
+	 */
+	econtext = node->ss.ps.ps_ExprContext;
+	slot = node->ss.ss_ScanTupleSlot;
+	scan = node->ss.ss_currentScanDesc;
+
+
+	while (table_scan_bitmap_next_tuple(scan, slot, &node->recheck,
+										&node->lossy_pages, &node->exact_pages))
 	{
-		while (table_scan_bitmap_next_tuple(scan, slot))
-		{
-			CHECK_FOR_INTERRUPTS();
+		CHECK_FOR_INTERRUPTS();
 
-			/*
-			 * If we are using lossy info, we have to recheck the qual
-			 * conditions at every tuple.
-			 */
-			if (node->recheck)
+
+		/*
+		 * If we are using lossy info, we have to recheck the qual conditions
+		 * at every tuple.
+		 */
+		if (node->recheck)
+		{
+			econtext->ecxt_scantuple = slot;
+			if (!ExecQualAndReset(node->bitmapqualorig, econtext))
 			{
-				econtext->ecxt_scantuple = slot;
-				if (!ExecQualAndReset(node->bitmapqualorig, econtext))
-				{
-					/* Fails recheck, so drop it and loop back for another */
-					InstrCountFiltered2(node, 1);
-					ExecClearTuple(slot);
-					continue;
-				}
+				/* Fails recheck, so drop it and loop back for another */
+				InstrCountFiltered2(node, 1);
+				ExecClearTuple(slot);
+				continue;
 			}
-
-			/* OK to return this tuple */
-			return slot;
 		}
 
-new_page:
-
-		if (!table_scan_bitmap_next_block(scan, &node->blockno, &node->recheck,
-										  &node->lossy_pages, &node->exact_pages))
-			break;
+		/* OK to return this tuple */
+		return slot;
 	}
 
+
 	/*
 	 * if we get here it means we are at the end of the scan..
 	 */
@@ -334,7 +325,6 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 	node->tbm = NULL;
 	node->initialized = false;
 	node->recheck = true;
-	node->blockno = InvalidBlockNumber;
 
 	ExecScanReScan(&node->ss);
 
@@ -414,7 +404,6 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	scanstate->initialized = false;
 	scanstate->pstate = NULL;
 	scanstate->recheck = true;
-	scanstate->blockno = InvalidBlockNumber;
 
 	/*
 	 * Miscellaneous initialization
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 6bb53e4346..cf56cc572f 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -313,7 +313,7 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
 				info->amcanparallel = amroutine->amcanparallel;
 				info->amhasgettuple = (amroutine->amgettuple != NULL);
 				info->amhasgetbitmap = amroutine->amgetbitmap != NULL &&
-					relation->rd_tableam->scan_bitmap_next_block != NULL;
+					relation->rd_tableam->scan_bitmap_next_tuple != NULL;
 				info->amcanmarkpos = (amroutine->ammarkpos != NULL &&
 									  amroutine->amrestrpos != NULL);
 				info->amcostestimate = amroutine->amcostestimate;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index eea7b7ff20..b2f0a61751 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -790,34 +790,21 @@ typedef struct TableAmRoutine
 	 * ------------------------------------------------------------------------
 	 */
 
-	/*
-	 * Prepare to fetch / check / return tuples as part of a bitmap table
-	 * scan. `scan` was started via table_beginscan_bm(). Return false if the
-	 * bitmap is exhausted and true otherwise.
-	 *
-	 * This will typically read and pin the target block, and do the necessary
-	 * work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
-	 * make sense to perform tuple visibility checks at this time).
-	 *
-	 * lossy_pages is incremented if the bitmap is lossy for the selected
-	 * block; otherwise, exact_pages is incremented.
-	 *
-	 * Optional callback, but either both scan_bitmap_next_block and
-	 * scan_bitmap_next_tuple need to exist, or neither.
-	 */
-	bool		(*scan_bitmap_next_block) (TableScanDesc scan,
-										   BlockNumber *blockno, bool *recheck,
-										   long *lossy_pages, long *exact_pages);
 
 	/*
 	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
 	 * if a visible tuple was found, false otherwise.
 	 *
-	 * Optional callback, but either both scan_bitmap_next_block and
-	 * scan_bitmap_next_tuple need to exist, or neither.
+	 * recheck is set if recheck is required.
+	 *
+	 * The table AM is responsible for reading in blocks and counting (for
+	 * EXPLAIN) which of those blocks were represented lossily in the bitmap
+	 * using the lossy_pages and exact_pages counters.
 	 */
 	bool		(*scan_bitmap_next_tuple) (TableScanDesc scan,
-										   TupleTableSlot *slot);
+										   TupleTableSlot *slot,
+										   bool *recheck,
+										   long *lossy_pages, long *exact_pages);
 
 	/*
 	 * Prepare to fetch tuples from the next block in a sample scan. Return
@@ -1983,45 +1970,13 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
  */
 
 /*
- * Prepare to fetch / check / return tuples as part of a bitmap table scan.
- * `scan` needs to have been started via table_beginscan_bm(). Returns false if
- * there are no more blocks in the bitmap, true otherwise. lossy_pages is
- * incremented is the block's representation in the bitmap is lossy; otherwise,
- * exact_pages is incremented.
- *
- * Note, this is an optionally implemented function, therefore should only be
- * used after verifying the presence (at plan time or such).
- */
-static inline bool
-table_scan_bitmap_next_block(TableScanDesc scan,
-							 BlockNumber *blockno, bool *recheck,
-							 long *lossy_pages,
-							 long *exact_pages)
-{
-	/*
-	 * We don't expect direct calls to table_scan_bitmap_next_block with valid
-	 * CheckXidAlive for catalog or regular tables.  See detailed comments in
-	 * xact.c where these variables are declared.
-	 */
-	if (unlikely(TransactionIdIsValid(CheckXidAlive) && !bsysscan))
-		elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
-
-	return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
-														   blockno, recheck,
-														   lossy_pages, exact_pages);
-}
-
-/*
- * Fetch the next tuple of a bitmap table scan into `slot` and return true if
- * a visible tuple was found, false otherwise.
- * table_scan_bitmap_next_block() needs to previously have selected a
- * block (i.e. returned true), and no previous
- * table_scan_bitmap_next_tuple() for the same block may have
- * returned false.
+ * Fetch the next tuple of a bitmap table scan into `slot` and return true if a
+ * visible tuple was found, false otherwise.
  */
 static inline bool
 table_scan_bitmap_next_tuple(TableScanDesc scan,
-							 TupleTableSlot *slot)
+							 TupleTableSlot *slot, bool *recheck,
+							 long *lossy_pages, long *exact_pages)
 {
 	/*
 	 * We don't expect direct calls to table_scan_bitmap_next_tuple with valid
@@ -2032,7 +1987,9 @@ table_scan_bitmap_next_tuple(TableScanDesc scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rs_rd->rd_tableam->scan_bitmap_next_tuple(scan,
-														   slot);
+														   slot, recheck,
+														   lossy_pages,
+														   exact_pages);
 }
 
 /*
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 713de7d50f..49ce4ff9a7 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1797,7 +1797,6 @@ typedef struct ParallelBitmapHeapState
  *		initialized		   is node is ready to iterate
  *		pstate			   shared state for parallel bitmap scan
  *		recheck			   do current page's tuples need recheck
- *		blockno			   used to validate pf and current block in sync
  * ----------------
  */
 typedef struct BitmapHeapScanState
@@ -1810,7 +1809,6 @@ typedef struct BitmapHeapScanState
 	bool		initialized;
 	ParallelBitmapHeapState *pstate;
 	bool		recheck;
-	BlockNumber blockno;
 } BitmapHeapScanState;
 
 /* ----------------
-- 
2.40.1

