v22b-0024-Remove-table-AM-callback-scan_bitmap_next_block.patch

From afba4ebb7113716d9cd4d34321f2a634d44c28b1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 21 May 2024 18:02:38 -0400
Subject: [PATCH v22b 24/29] Remove table AM callback scan_bitmap_next_block

Non block-based table AMs might find implementing bitmap table scan
somewhat easier by removing table_scan_bitmap_next_block(). Now bitmap
table scan code only calls table_scan_bitmap_next_tuple().

This commit turns the heap AM implementation of scan_bitmap_next_block()
into a local helper in heapam_handler.c.
---
 src/backend/access/heap/heapam_handler.c  | 58 +++++++++------
 src/backend/access/table/tableamapi.c     |  3 -
 src/backend/executor/nodeBitmapHeapscan.c | 77 ++++++++------------
 src/backend/optimizer/util/plancat.c      |  2 +-
 src/include/access/tableam.h              | 88 ++++++-----------------
 5 files changed, 88 insertions(+), 140 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 166a84b909e..71c74fa12c6 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2334,18 +2334,22 @@ BitmapAdjustPrefetchTarget(BitmapHeapScanDesc *scan)
 #endif							/* USE_PREFETCH */
 }
 
-
-/* ------------------------------------------------------------------------
- * Executor related callbacks for the heap AM
- * ------------------------------------------------------------------------
+/*
+ * Prepare to fetch / check / return tuples as part of a bitmap heap scan.
+ * `scan` needs to have been started via heap_beginscan_bm(). Returns false if
+ * there are no more blocks in the bitmap, true otherwise. `lossy_pages` is
+ * incremented if the block's representation in the bitmap is lossy; otherwise,
+ * `exact_pages` is incremented.
+ *
+ * `recheck` is set to indicate whether or not the tuples from this block
+ * should be rechecked. Tuples from lossy pages will always need to be
+ * rechecked, but some non-lossy pages' tuples may also require recheck.
  */
-
 static bool
-heapam_scan_bitmap_next_block(BitmapTableScanDesc *sscan,
+heapam_scan_bitmap_next_block(BitmapHeapScanDesc *scan,
 							  bool *recheck,
 							  long *lossy_pages, long *exact_pages)
 {
-	BitmapHeapScanDesc *scan = (BitmapHeapScanDesc *) sscan;
 	BlockNumber block;
 	Buffer		buffer;
 	Snapshot	snapshot;
@@ -2516,9 +2520,16 @@ heapam_scan_bitmap_next_block(BitmapTableScanDesc *sscan,
 	return true;
 }
 
+
+/* ------------------------------------------------------------------------
+ * Executor related callbacks for the heap AM
+ * ------------------------------------------------------------------------
+ */
+
 static bool
 heapam_scan_bitmap_next_tuple(BitmapTableScanDesc *sscan,
-							  TupleTableSlot *slot)
+							  TupleTableSlot *slot, bool *recheck,
+							  long *lossy_pages, long *exact_pages)
 {
 	BitmapHeapScanDesc *scan = (BitmapHeapScanDesc *) sscan;
 	ParallelBitmapHeapState *pstate = sscan->pstate;
@@ -2526,22 +2537,28 @@ heapam_scan_bitmap_next_tuple(BitmapTableScanDesc *sscan,
 	Page		page;
 	ItemId		lp;
 
-	if (scan->empty_tuples_pending > 0)
+	/*
+	 * Out of range?  If so, nothing more to look at on this page
+	 */
+	while (scan->vis_idx < 0 || scan->vis_idx >= scan->vis_ntuples)
 	{
 		/*
-		 * If we don't have to fetch the tuple, just return nulls.
+		 * Emit empty tuples before advancing to the next block
 		 */
-		ExecStoreAllNullTuple(slot);
-		scan->empty_tuples_pending--;
-		BitmapPrefetch(scan);
-		return true;
-	}
+		if (scan->empty_tuples_pending > 0)
+		{
+			/*
+			 * If we don't have to fetch the tuple, just return nulls.
+			 */
+			ExecStoreAllNullTuple(slot);
+			scan->empty_tuples_pending--;
+			BitmapPrefetch(scan);
+			return true;
+		}
 
-	/*
-	 * Out of range?  If so, nothing more to look at on this page
-	 */
-	if (scan->vis_idx < 0 || scan->vis_idx >= scan->vis_ntuples)
-		return false;
+		if (!heapam_scan_bitmap_next_block(scan, recheck, lossy_pages, exact_pages))
+			return false;
+	}
 
 #ifdef USE_PREFETCH
 
@@ -2942,7 +2959,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 e9b598256fb..a47527d490a 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -91,9 +91,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 317cbda857a..badc4b98dc5 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -185,70 +185,49 @@ BitmapTableScanSetup(BitmapHeapScanState *node)
 static TupleTableSlot *
 BitmapHeapNext(BitmapHeapScanState *node)
 {
-	ExprContext *econtext = node->ss.ps.ps_ExprContext;
-	TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
-	BitmapTableScanDesc *scan = node->scandesc;
+	ExprContext *econtext;
+	TupleTableSlot *slot;
+	BitmapTableScanDesc *scan;
 
 	/*
 	 * If we haven't yet performed the underlying index scan, do it, and begin
 	 * the iteration over the bitmap.
 	 */
 	if (!node->initialized)
-	{
 		BitmapTableScanSetup(node);
-		scan = node->scandesc;
-		goto new_page;
-	}
 
-	for (;;)
+	scan = node->scandesc;
+	econtext = node->ss.ps.ps_ExprContext;
+	slot = node->ss.ss_ScanTupleSlot;
+
+	while (table_scan_bitmap_next_tuple(scan, slot,
+										&node->recheck,
+										&node->lossy_pages, &node->exact_pages))
 	{
-		while (table_scan_bitmap_next_tuple(scan, slot))
-		{
-			/*
-			 * Continuing in previously obtained page.
-			 */
+		/*
+		 * Continuing in previously obtained page.
+		 */
 
-			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:
-
-		/*
-		 * XXX maybe add comment explaining what "false" means here?
-		 *
-		 * XXX I'm a bit unsure if this needs to be handled using goto. Wouldn't
-		 * it be simpler / easier to understand to have two nested loops?
-		 *
-		 * while (true)
-		 *		if (!table_scan_bitmap_next_block(...)) { break; }
-		 *		while (table_scan_bitmap_next_tuple(...)) {
-		 *			... process tuples ...
-		 *		}
-		 *
-		 * But I haven't tried implementing this.
-		 */
-		if (!table_scan_bitmap_next_block(scan, &node->recheck,
-										  &node->lossy_pages, &node->exact_pages))
-			break;
+		/* OK to return this tuple */
+		return slot;
 	}
 
 	/*
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 775955363ef..c11ff3a1bae 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -324,7 +324,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 a00b9bd296a..d496d68f1c2 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -808,37 +808,20 @@ typedef struct TableAmRoutine
 	 */
 
 	/*
-	 * Prepare to fetch / check / return tuples from 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 a block, and do the necessary work to
-	 * allow scan_bitmap_next_tuple() to return tuples (e.g. depending on the
-	 * table AM, it might make sense to perform tuple visibility checks at
-	 * this time).
+	 * Fetch the next tuple of a bitmap table scan into `slot` and return true
+	 * if a visible tuple was found, false otherwise.
 	 *
 	 * `lossy_pages` is incremented if the bitmap is lossy for the selected
-	 * block; otherwise, `exact_pages` is incremented.
-	 *
-	 * Prefetching future blocks indicated in the bitmap is left to the table
-	 * AM.
+	 * page; otherwise, `exact_pages` is incremented. This is tracked for
+	 * display in EXPLAIN ANALYZE output.
 	 *
-	 * Optional callback, but either both scan_bitmap_next_block and
-	 * scan_bitmap_next_tuple need to exist, or neither.
-	 */
-	bool		(*scan_bitmap_next_block) (BitmapTableScanDesc *scan,
-										   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.
+	 * Prefetching additional data from the bitmap is left to the table AM.
 	 *
-	 * Optional callback, but either both scan_bitmap_next_block and
-	 * scan_bitmap_next_tuple need to exist, or neither.
+	 * This is an optional callback.
 	 */
 	bool		(*scan_bitmap_next_tuple) (BitmapTableScanDesc *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
@@ -1982,54 +1965,26 @@ 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 if the block's representation in the bitmap is lossy; otherwise,
- * `exact_pages` is incremented.
- *
- * `recheck` is set by the table AM to indicate whether or not the tuples
- * from this block should be rechecked. Tuples from lossy pages will always
- * need to be rechecked, but some non-lossy pages' tuples may also require
- * recheck.
+ * Fetch / check / return tuples as part of a bitmap table scan. `scan` needs
+ * to have been started via table_beginscan_bm(). Fetch the next tuple of a
+ * bitmap table scan into `slot` and return true if a visible tuple was found,
+ * false otherwise.
  *
- * `blockno` is only used in bitmap table scan code to validate that the
- * prefetch block is staying ahead of the current block.
+ * `recheck` is set by the table AM to indicate whether or not the tuple in
+ * `slot` should be rechecked. Tuples from lossy pages will always need to be
+ * rechecked, but some non-lossy pages' tuples may also require recheck.
  *
- * Note, this is an optionally implemented function, therefore should only be
- * used after verifying the presence (at plan time or such).
+ * `lossy_pages` is incremented if the block's representation in the bitmap is
+ * lossy; otherwise, `exact_pages` is incremented.
  */
 static inline bool
-table_scan_bitmap_next_block(BitmapTableScanDesc *scan,
+table_scan_bitmap_next_tuple(BitmapTableScanDesc *scan,
+							 TupleTableSlot *slot,
 							 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->rel->rd_tableam->scan_bitmap_next_block(scan,
-														 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.
- */
-static inline bool
-table_scan_bitmap_next_tuple(BitmapTableScanDesc *scan,
-							 TupleTableSlot *slot)
 {
 	/*
 	 * We don't expect direct calls to table_scan_bitmap_next_tuple with valid
@@ -2040,7 +1995,8 @@ table_scan_bitmap_next_tuple(BitmapTableScanDesc *scan,
 		elog(ERROR, "unexpected table_scan_bitmap_next_tuple call during logical decoding");
 
 	return scan->rel->rd_tableam->scan_bitmap_next_tuple(scan,
-														 slot);
+														 slot, recheck,
+														 lossy_pages, exact_pages);
 }
 
 /*
-- 
2.45.2