From b568e7512e10d49742509315ea18dde4185445ad Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Sat, 6 Apr 2024 14:51:35 -0400
Subject: [PATCH v19 15/21] Add BitmapHeapScanDesc and scan functions

Bitmap Heap Scans are specialized enough that it makes sense to create a
separate "sub-class" of the HeapScanDescData with Bitmap Heap
Scan-specific members.

This creates a convenient location for other Bitmap Heap Scan-specific
members to be added in the future. For example, the VM buffer used for
avoiding prefetching heap table blocks when no data is needed for
the query (the skip fetch optimization). This is a table AM layering
violation and should be moved out of the BitmapHeapScanState.

Along with the BitmapHeapScanDesc structure, add heap implementations of
the table AM functions for beginning, restarting, and ending a Bitmap
Table Scan.

Author: Melanie Plageman
Discussion: https://postgr.es/m/CAAKRu_ZwCwWFeL_H3ia26bP2e7HiKLWt0ZmGXPVwPO6uXq0vaA%40mail.gmail.com
---
 src/backend/access/heap/heapam.c          | 101 ++++++++++++++++++----
 src/backend/access/heap/heapam_handler.c  |  48 +++++-----
 src/backend/executor/nodeBitmapHeapscan.c |  46 +++++-----
 src/include/access/heapam.h               |  33 +++++--
 src/include/access/tableam.h              |  28 ++++--
 src/tools/pgindent/typedefs.list          |   1 +
 6 files changed, 180 insertions(+), 77 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 9be3a4b1afb..bcb9e4390d4 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -13,8 +13,11 @@
  *
  * INTERFACE ROUTINES
  *		heap_beginscan	- begin relation scan
+ *		heap_beginscan_bm - begin bitmap heap scan
  *		heap_rescan		- restart a relation scan
+ *		heap_rescan_bm  - restart a bitmap heap scan
  *		heap_endscan	- end relation scan
+ *		heap_endscan_bm - end a bitmap heap scan
  *		heap_getnext	- retrieve next tuple in scan
  *		heap_fetch		- retrieve tuple with given tid
  *		heap_insert		- insert tuple into a relation
@@ -967,8 +970,6 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 	scan->rs_base.rs_flags = flags;
 	scan->rs_base.rs_parallel = parallel_scan;
 	scan->rs_strategy = NULL;	/* set in initscan */
-	scan->rs_vmbuffer = InvalidBuffer;
-	scan->rs_empty_tuples_pending = 0;
 
 	/*
 	 * Disable page-at-a-time mode if it's not a MVCC-safe snapshot.
@@ -1026,6 +1027,45 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 	return (TableScanDesc) scan;
 }
 
+TableScanDesc
+heap_beginscan_bm(Relation relation, Snapshot snapshot, uint32 flags)
+{
+	BitmapHeapScanDesc scan;
+
+	/*
+	 * increment relation ref count while scanning relation
+	 *
+	 * This is just to make really sure the relcache entry won't go away while
+	 * the scan has a pointer to it.  Caller should be holding the rel open
+	 * anyway, so this is redundant in all normal scenarios...
+	 */
+	RelationIncrementReferenceCount(relation);
+	scan = (BitmapHeapScanDesc) palloc(sizeof(BitmapHeapScanDescData));
+
+	scan->heap_common.rs_base.rs_rd = relation;
+	scan->heap_common.rs_base.rs_snapshot = snapshot;
+	scan->heap_common.rs_base.rs_nkeys = 0;
+	scan->heap_common.rs_base.rs_flags = flags;
+	scan->heap_common.rs_base.rs_parallel = NULL;
+	scan->heap_common.rs_strategy = NULL;
+
+	Assert(snapshot && IsMVCCSnapshot(snapshot));
+
+	/* we only need to set this up once */
+	scan->heap_common.rs_ctup.t_tableOid = RelationGetRelid(relation);
+
+	scan->heap_common.rs_parallelworkerdata = NULL;
+	scan->heap_common.rs_base.rs_key = NULL;
+
+	initscan(&scan->heap_common, NULL, false);
+
+	scan->vmbuffer = InvalidBuffer;
+	scan->empty_tuples_pending = 0;
+
+	return (TableScanDesc) scan;
+}
+
+
 void
 heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
 			bool allow_strat, bool allow_sync, bool allow_pagemode)
@@ -1057,20 +1097,33 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (BufferIsValid(scan->rs_vmbuffer))
-	{
-		ReleaseBuffer(scan->rs_vmbuffer);
-		scan->rs_vmbuffer = InvalidBuffer;
-	}
-
-	scan->rs_empty_tuples_pending = 0;
-
 	/*
 	 * reinitialize scan descriptor
 	 */
 	initscan(scan, key, true);
 }
 
+void
+heap_rescan_bm(TableScanDesc sscan)
+{
+	BitmapHeapScanDesc scan = (BitmapHeapScanDesc) sscan;
+
+	if (BufferIsValid(scan->heap_common.rs_cbuf))
+		ReleaseBuffer(scan->heap_common.rs_cbuf);
+
+	if (BufferIsValid(scan->vmbuffer))
+		ReleaseBuffer(scan->vmbuffer);
+	scan->vmbuffer = InvalidBuffer;
+
+	scan->empty_tuples_pending = 0;
+
+	/*
+	 * reinitialize heap scan descriptor
+	 */
+	initscan(&scan->heap_common, NULL, true);
+}
+
+
 void
 heap_endscan(TableScanDesc sscan)
 {
@@ -1084,11 +1137,6 @@ heap_endscan(TableScanDesc sscan)
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (BufferIsValid(scan->rs_vmbuffer))
-		ReleaseBuffer(scan->rs_vmbuffer);
-
-	scan->rs_empty_tuples_pending = 0;
-
 	/*
 	 * decrement relation reference count and free scan descriptor storage
 	 */
@@ -1109,6 +1157,29 @@ heap_endscan(TableScanDesc sscan)
 	pfree(scan);
 }
 
+/*
+ * Cleanup BitmapHeapScan table state
+ */
+void
+heap_endscan_bm(TableScanDesc sscan)
+{
+	BitmapHeapScanDesc scan = (BitmapHeapScanDesc) sscan;
+
+	if (BufferIsValid(scan->heap_common.rs_cbuf))
+		ReleaseBuffer(scan->heap_common.rs_cbuf);
+
+	if (BufferIsValid(scan->vmbuffer))
+		ReleaseBuffer(scan->vmbuffer);
+
+	/*
+	 * decrement relation reference count and free scan descriptor storage
+	 */
+	RelationDecrementReferenceCount(scan->heap_common.rs_base.rs_rd);
+
+	pfree(scan);
+}
+
+
 HeapTuple
 heap_getnext(TableScanDesc sscan, ScanDirection direction)
 {
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index ec6128e1d65..a2cbd7a24d6 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2187,11 +2187,12 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
  */
 
 static bool
-heapam_scan_bitmap_next_block(TableScanDesc scan,
+heapam_scan_bitmap_next_block(TableScanDesc sscan,
 							  BlockNumber *blockno, bool *recheck,
 							  long *lossy_pages, long *exact_pages)
 {
-	HeapScanDesc hscan = (HeapScanDesc) scan;
+	BitmapHeapScanDesc scan = (BitmapHeapScanDesc) sscan;
+	HeapScanDesc hscan = &scan->heap_common;
 	BlockNumber block;
 	Buffer		buffer;
 	Snapshot	snapshot;
@@ -2208,12 +2209,12 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 	{
 		CHECK_FOR_INTERRUPTS();
 
-		tbmres = unified_tbm_iterate(&scan->rs_bhs_iterator);
+		tbmres = unified_tbm_iterate(&sscan->rs_bhs_iterator);
 
 		if (tbmres == NULL)
 		{
 			/* no more entries in the bitmap */
-			Assert(hscan->rs_empty_tuples_pending == 0);
+			Assert(scan->empty_tuples_pending == 0);
 			return false;
 		}
 
@@ -2236,15 +2237,15 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 	 * heap, the bitmap entries don't need rechecking, and all tuples on the
 	 * page are visible to our transaction.
 	 */
-	if (!(scan->rs_flags & SO_NEED_TUPLES) &&
+	if (!(sscan->rs_flags & SO_NEED_TUPLES) &&
 		!tbmres->recheck &&
-		VM_ALL_VISIBLE(scan->rs_rd, tbmres->blockno, &hscan->rs_vmbuffer))
+		VM_ALL_VISIBLE(sscan->rs_rd, tbmres->blockno, &scan->vmbuffer))
 	{
 		/* can't be lossy in the skip_fetch case */
 		Assert(tbmres->ntuples >= 0);
-		Assert(hscan->rs_empty_tuples_pending >= 0);
+		Assert(scan->empty_tuples_pending >= 0);
 
-		hscan->rs_empty_tuples_pending += tbmres->ntuples;
+		scan->empty_tuples_pending += tbmres->ntuples;
 
 		return true;
 	}
@@ -2255,18 +2256,18 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 	 * Acquire pin on the target heap page, trading in any pin we held before.
 	 */
 	hscan->rs_cbuf = ReleaseAndReadBuffer(hscan->rs_cbuf,
-										  scan->rs_rd,
+										  sscan->rs_rd,
 										  block);
 	hscan->rs_cblock = block;
 	buffer = hscan->rs_cbuf;
-	snapshot = scan->rs_snapshot;
+	snapshot = sscan->rs_snapshot;
 
 	ntup = 0;
 
 	/*
 	 * Prune and repair fragmentation for the whole page, if possible.
 	 */
-	heap_page_prune_opt(scan->rs_rd, buffer);
+	heap_page_prune_opt(sscan->rs_rd, buffer);
 
 	/*
 	 * We must hold share lock on the buffer content while examining tuple
@@ -2294,7 +2295,7 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 			HeapTupleData heapTuple;
 
 			ItemPointerSet(&tid, block, offnum);
-			if (heap_hot_search_buffer(&tid, scan->rs_rd, buffer, snapshot,
+			if (heap_hot_search_buffer(&tid, sscan->rs_rd, buffer, snapshot,
 									   &heapTuple, NULL, true))
 				hscan->rs_vistuples[ntup++] = ItemPointerGetOffsetNumber(&tid);
 		}
@@ -2320,16 +2321,16 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 				continue;
 			loctup.t_data = (HeapTupleHeader) PageGetItem(page, lp);
 			loctup.t_len = ItemIdGetLength(lp);
-			loctup.t_tableOid = scan->rs_rd->rd_id;
+			loctup.t_tableOid = sscan->rs_rd->rd_id;
 			ItemPointerSet(&loctup.t_self, block, offnum);
 			valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
 			if (valid)
 			{
 				hscan->rs_vistuples[ntup++] = offnum;
-				PredicateLockTID(scan->rs_rd, &loctup.t_self, snapshot,
+				PredicateLockTID(sscan->rs_rd, &loctup.t_self, snapshot,
 								 HeapTupleHeaderGetXmin(loctup.t_data));
 			}
-			HeapCheckForSerializableConflictOut(valid, scan->rs_rd, &loctup,
+			HeapCheckForSerializableConflictOut(valid, sscan->rs_rd, &loctup,
 												buffer, snapshot);
 		}
 	}
@@ -2355,21 +2356,22 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 }
 
 static bool
-heapam_scan_bitmap_next_tuple(TableScanDesc scan,
+heapam_scan_bitmap_next_tuple(TableScanDesc sscan,
 							  TupleTableSlot *slot)
 {
-	HeapScanDesc hscan = (HeapScanDesc) scan;
+	BitmapHeapScanDesc scan = (BitmapHeapScanDesc) sscan;
+	HeapScanDesc hscan = &scan->heap_common;
 	OffsetNumber targoffset;
 	Page		page;
 	ItemId		lp;
 
-	if (hscan->rs_empty_tuples_pending > 0)
+	if (scan->empty_tuples_pending > 0)
 	{
 		/*
 		 * If we don't have to fetch the tuple, just return nulls.
 		 */
 		ExecStoreAllNullTuple(slot);
-		hscan->rs_empty_tuples_pending--;
+		scan->empty_tuples_pending--;
 		return true;
 	}
 
@@ -2386,10 +2388,10 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
 
 	hscan->rs_ctup.t_data = (HeapTupleHeader) PageGetItem(page, lp);
 	hscan->rs_ctup.t_len = ItemIdGetLength(lp);
-	hscan->rs_ctup.t_tableOid = scan->rs_rd->rd_id;
+	hscan->rs_ctup.t_tableOid = sscan->rs_rd->rd_id;
 	ItemPointerSet(&hscan->rs_ctup.t_self, hscan->rs_cblock, targoffset);
 
-	pgstat_count_heap_fetch(scan->rs_rd);
+	pgstat_count_heap_fetch(sscan->rs_rd);
 
 	/*
 	 * Set up the result slot to point to this tuple.  Note that the slot
@@ -2701,6 +2703,10 @@ static const TableAmRoutine heapam_methods = {
 	.scan_rescan = heap_rescan,
 	.scan_getnextslot = heap_getnextslot,
 
+	.scan_rescan_bm = heap_rescan_bm,
+	.scan_begin_bm = heap_beginscan_bm,
+	.scan_end_bm = heap_endscan_bm,
+
 	.scan_set_tidrange = heap_set_tidrange,
 	.scan_getnextslot_tidrange = heap_getnextslot_tidrange,
 
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 1fe50348c6f..bb9a11aa51d 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -96,6 +96,8 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	 */
 	if (!node->initialized)
 	{
+		bool		need_tuples = false;
+
 		/*
 		 * The leader will immediately come out of the function, but others
 		 * will be blocked until leader populates the TBM and wakes them up.
@@ -136,36 +138,28 @@ BitmapHeapNext(BitmapHeapScanState *node)
 			}
 		}
 
+
+		/*
+		 * We can potentially skip fetching heap pages if we do not need any
+		 * columns of the table, either for checking non-indexable quals or
+		 * for returning data.  This test is a bit simplistic, as it checks
+		 * the stronger condition that there's no qual or return tlist at all.
+		 * But in most cases it's probably not worth working harder than that.
+		 */
+		need_tuples = (node->ss.ps.plan->qual != NIL ||
+					   node->ss.ps.plan->targetlist != NIL);
+
 		/*
 		 * If this is the first scan of the underlying table, create the table
 		 * scan descriptor and begin the scan.
 		 */
-		if (!scan)
-		{
-			bool		need_tuples = false;
+		scan = table_beginscan_bm(node->ss.ss_currentScanDesc,
+								  node->ss.ss_currentRelation,
+								  node->ss.ps.state->es_snapshot,
+								  need_tuples);
 
-			/*
-			 * We can potentially skip fetching heap pages if we do not need
-			 * any columns of the table, either for checking non-indexable
-			 * quals or for returning data.  This test is a bit simplistic, as
-			 * it checks the stronger condition that there's no qual or return
-			 * tlist at all. But in most cases it's probably not worth working
-			 * harder than that.
-			 */
-			need_tuples = (node->ss.ps.plan->qual != NIL ||
-						   node->ss.ps.plan->targetlist != NIL);
-
-			scan = table_beginscan_bm(node->ss.ss_currentRelation,
-									  node->ss.ps.state->es_snapshot,
-									  0,
-									  NULL,
-									  need_tuples);
-
-			node->ss.ss_currentScanDesc = scan;
-		}
+		node->ss.ss_currentScanDesc = scan;
 
-		/* rescan to release any page pin */
-		table_rescan(scan, NULL);
 		unified_tbm_begin_iterate(&scan->rs_bhs_iterator, tbm, dsa,
 								  pstate ?
 								  pstate->tbmiterator :
@@ -591,10 +585,10 @@ ExecEndBitmapHeapScan(BitmapHeapScanState *node)
 
 
 	/*
-	 * close heap scan
+	 * close bitmap heap scan
 	 */
 	if (scanDesc)
-		table_endscan(scanDesc);
+		table_endscan_bm(scanDesc);
 
 	/*
 	 * release bitmaps and buffers if any
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 750ea30852e..1ee5195a8d5 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -76,22 +76,34 @@ typedef struct HeapScanDescData
 	 */
 	ParallelBlockTableScanWorkerData *rs_parallelworkerdata;
 
+	/* these fields only used in page-at-a-time mode and for bitmap scans */
+	int			rs_cindex;		/* current tuple's index in vistuples */
+	int			rs_ntuples;		/* number of visible tuples on page */
+	OffsetNumber rs_vistuples[MaxHeapTuplesPerPage];	/* their offsets */
+}			HeapScanDescData;
+typedef struct HeapScanDescData *HeapScanDesc;
+
+typedef struct BitmapHeapScanDescData
+{
+	/* All the non-BitmapHeapScan specific members */
+	HeapScanDescData heap_common;
+
+	/*
+	 * Members common to Parallel and Serial BitmapHeapScan
+	 */
+
 	/*
 	 * These fields are only used for bitmap scans for the "skip fetch"
 	 * optimization. Bitmap scans needing no fields from the heap may skip
 	 * fetching an all visible block, instead using the number of tuples per
 	 * block reported by the bitmap to determine how many NULL-filled tuples
-	 * to return.
+	 * to return. They are common to parallel and serial BitmapHeapScans
 	 */
-	Buffer		rs_vmbuffer;
-	int			rs_empty_tuples_pending;
+	Buffer		vmbuffer;
+	int			empty_tuples_pending;
+}			BitmapHeapScanDescData;
+typedef struct BitmapHeapScanDescData *BitmapHeapScanDesc;
 
-	/* these fields only used in page-at-a-time mode and for bitmap scans */
-	int			rs_cindex;		/* current tuple's index in vistuples */
-	int			rs_ntuples;		/* number of visible tuples on page */
-	OffsetNumber rs_vistuples[MaxHeapTuplesPerPage];	/* their offsets */
-}			HeapScanDescData;
-typedef struct HeapScanDescData *HeapScanDesc;
 
 /*
  * Descriptor for fetches from heap via an index.
@@ -289,6 +301,9 @@ extern void heap_set_tidrange(TableScanDesc sscan, ItemPointer mintid,
 extern bool heap_getnextslot_tidrange(TableScanDesc sscan,
 									  ScanDirection direction,
 									  TupleTableSlot *slot);
+extern TableScanDesc heap_beginscan_bm(Relation relation, Snapshot snapshot, uint32 flags);
+extern void heap_endscan_bm(TableScanDesc scan);
+extern void heap_rescan_bm(TableScanDesc sscan);
 extern bool heap_fetch(Relation relation, Snapshot snapshot,
 					   HeapTuple tuple, Buffer *userbuf, bool keep_buf);
 extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 15f3db4113a..0763ab9847c 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -948,20 +948,27 @@ table_beginscan_strat(Relation rel, Snapshot snapshot,
 
 /*
  * table_beginscan_bm is an alternative entry point for setting up a
- * TableScanDesc for a bitmap heap scan.  Although that scan technology is
- * really quite unlike a standard seqscan, there is just enough commonality to
- * make it worth using the same data structure.
+ * TableScanDesc for a bitmap table scan.
  */
 static inline TableScanDesc
-table_beginscan_bm(Relation rel, Snapshot snapshot,
-				   int nkeys, struct ScanKeyData *key, bool need_tuple)
+table_beginscan_bm(TableScanDesc scan, Relation rel, Snapshot snapshot,
+				   bool need_tuple)
 {
 	uint32		flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE;
 
 	if (need_tuple)
 		flags |= SO_NEED_TUPLES;
 
-	return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags);
+	/*
+	 * If this is the first scan of the underlying table, create the table
+	 * scan descriptor and begin the scan.
+	 */
+	if (!scan)
+		scan = rel->rd_tableam->scan_begin_bm(rel, snapshot, flags);
+
+	scan->rs_rd->rd_tableam->scan_rescan_bm(scan);
+
+	return scan;
 }
 
 /*
@@ -1015,6 +1022,15 @@ table_endscan(TableScanDesc scan)
 	scan->rs_rd->rd_tableam->scan_end(scan);
 }
 
+/*
+* End Bitmap Table Scan
+ */
+static inline void
+table_endscan_bm(TableScanDesc scan)
+{
+	scan->rs_rd->rd_tableam->scan_end_bm(scan);
+}
+
 /*
  * Restart a relation scan.
  */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 71686e79abb..b74338fea83 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -263,6 +263,7 @@ BitmapAndPath
 BitmapAndState
 BitmapHeapPath
 BitmapHeapScan
+BitmapHeapScanDesc
 BitmapHeapScanState
 BitmapIndexScan
 BitmapIndexScanState
-- 
2.40.1

