From afb28afbf45efbdd9fa4992a3dc1f1fe7f07afd3 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Mon, 25 Mar 2024 11:05:51 -0400
Subject: [PATCH v16 10/18] Unify parallel and serial BitmapHeapScan iterator
 interfaces

Introduce a new type, BitmapHeapIterator, which allows unified access to both
TBMIterator and TBMSharedIterators. This encapsulates the parallel and serial
iterators and their access and makes the bitmap heap scan code a bit cleaner.
This naturally lends itself to a bit of reorganization of the
!node->initialized path in BitmapHeapNext().

Author: Melanie Plageman
Discussion: https://postgr.es/m/CAAKRu_ZwCwWFeL_H3ia26bP2e7HiKLWt0ZmGXPVwPO6uXq0vaA%40mail.gmail.com
---
 src/backend/access/heap/heapam_handler.c  |   5 +-
 src/backend/executor/nodeBitmapHeapscan.c | 172 ++++++++++++----------
 src/include/access/relscan.h              |   7 +-
 src/include/access/tableam.h              |  40 +----
 src/include/executor/nodeBitmapHeapscan.h |   7 +
 src/include/nodes/execnodes.h             |  13 +-
 src/tools/pgindent/typedefs.list          |   1 +
 7 files changed, 124 insertions(+), 121 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 9d3e7c7fda..5e57d19d91 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2208,10 +2208,7 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 	{
 		CHECK_FOR_INTERRUPTS();
 
-		if (scan->shared_tbmiterator)
-			tbmres = tbm_shared_iterate(scan->shared_tbmiterator);
-		else
-			tbmres = tbm_iterate(scan->tbmiterator);
+		tbmres = bhs_iterate(&scan->rs_bhs_iterator);
 
 		if (tbmres == NULL)
 		{
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index e1b13ddaa6..70b560b8ee 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -57,6 +57,61 @@ static inline void BitmapPrefetch(BitmapHeapScanState *node,
 								  TableScanDesc scan);
 static bool BitmapShouldInitializeSharedState(ParallelBitmapHeapState *pstate);
 
+/*
+ * Start iteration on a shared or non-shared bitmap iterator. Note that tbm
+ * will only be provided by serial BitmapHeapScan callers. dsa and dsp will
+ * only be provided by parallel BitmapHeapScan callers.
+ */
+void
+bhs_begin_iterate(BitmapHeapIterator *iterator, TIDBitmap *tbm,
+				  dsa_area *dsa, dsa_pointer dsp)
+{
+	Assert(iterator);
+
+	iterator->serial = NULL;
+	iterator->parallel = NULL;
+	iterator->exhausted = false;
+
+	/* Allocate a private iterator and attach the shared state to it */
+	if (DsaPointerIsValid(dsp))
+		iterator->parallel = tbm_attach_shared_iterate(dsa, dsp);
+	else
+		iterator->serial = tbm_begin_iterate(tbm);
+}
+
+/*
+ * Get the next TBMIterateResult from the bitmap iterator.
+ */
+TBMIterateResult *
+bhs_iterate(BitmapHeapIterator *iterator)
+{
+	Assert(iterator);
+	Assert(!iterator->exhausted);
+
+	if (iterator->serial)
+		return tbm_iterate(iterator->serial);
+	else
+		return tbm_shared_iterate(iterator->parallel);
+}
+
+/*
+ * Clean up the bitmap iterator.
+ */
+void
+bhs_end_iterate(BitmapHeapIterator *iterator)
+{
+	Assert(iterator);
+
+	if (iterator->serial)
+		tbm_end_iterate(iterator->serial);
+	else if (iterator->parallel)
+		tbm_end_shared_iterate(iterator->parallel);
+
+	iterator->serial = NULL;
+	iterator->parallel = NULL;
+	iterator->exhausted = true;
+}
+
 
 /* ----------------------------------------------------------------
  *		BitmapHeapNext
@@ -96,43 +151,23 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	 */
 	if (!node->initialized)
 	{
-		TBMIterator *tbmiterator = NULL;
-		TBMSharedIterator *shared_tbmiterator = NULL;
+		/*
+		 * The leader will immediately come out of the function, but others
+		 * will be blocked until leader populates the TBM and wakes them up.
+		 */
+		bool		init_shared_state = node->pstate ?
+			BitmapShouldInitializeSharedState(node->pstate) : false;
 
-		if (!pstate)
+		if (!pstate || init_shared_state)
 		{
 			tbm = (TIDBitmap *) MultiExecProcNode(outerPlanState(node));
 
 			if (!tbm || !IsA(tbm, TIDBitmap))
 				elog(ERROR, "unrecognized result from subplan");
-
 			node->tbm = tbm;
-			tbmiterator = tbm_begin_iterate(tbm);
 
-#ifdef USE_PREFETCH
-			if (node->prefetch_maximum > 0)
+			if (init_shared_state)
 			{
-				node->prefetch_iterator = tbm_begin_iterate(tbm);
-				node->prefetch_pages = 0;
-				node->prefetch_target = -1;
-			}
-#endif							/* USE_PREFETCH */
-		}
-		else
-		{
-			/*
-			 * The leader will immediately come out of the function, but
-			 * others will be blocked until leader populates the TBM and wakes
-			 * them up.
-			 */
-			if (BitmapShouldInitializeSharedState(pstate))
-			{
-				tbm = (TIDBitmap *) MultiExecProcNode(outerPlanState(node));
-				if (!tbm || !IsA(tbm, TIDBitmap))
-					elog(ERROR, "unrecognized result from subplan");
-
-				node->tbm = tbm;
-
 				/*
 				 * Prepare to iterate over the TBM. This will return the
 				 * dsa_pointer of the iterator state which will be used by
@@ -153,21 +188,9 @@ BitmapHeapNext(BitmapHeapScanState *node)
 					pstate->prefetch_target = -1;
 				}
 #endif
-
 				/* We have initialized the shared state so wake up others. */
 				BitmapDoneInitializingSharedState(pstate);
 			}
-
-			/* Allocate a private iterator and attach the shared state to it */
-			shared_tbmiterator = tbm_attach_shared_iterate(dsa, pstate->tbmiterator);
-
-#ifdef USE_PREFETCH
-			if (node->prefetch_maximum > 0)
-			{
-				node->shared_prefetch_iterator =
-					tbm_attach_shared_iterate(dsa, pstate->prefetch_iterator);
-			}
-#endif							/* USE_PREFETCH */
 		}
 
 		/*
@@ -198,8 +221,23 @@ BitmapHeapNext(BitmapHeapScanState *node)
 			node->ss.ss_currentScanDesc = scan;
 		}
 
-		scan->tbmiterator = tbmiterator;
-		scan->shared_tbmiterator = shared_tbmiterator;
+		bhs_begin_iterate(&scan->rs_bhs_iterator, tbm, dsa,
+						  pstate ?
+						  pstate->tbmiterator :
+						  InvalidDsaPointer);
+
+#ifdef USE_PREFETCH
+		if (node->prefetch_maximum > 0)
+		{
+			bhs_begin_iterate(&node->pf_iterator, tbm, dsa,
+							  pstate ?
+							  pstate->prefetch_iterator :
+							  InvalidDsaPointer);
+			/* Only used for serial BHS */
+			node->prefetch_pages = 0;
+			node->prefetch_target = -1;
+		}
+#endif							/* USE_PREFETCH */
 
 		node->initialized = true;
 
@@ -276,7 +314,7 @@ new_page:
 		 * ahead of the current block.
 		 */
 		if (node->pstate == NULL &&
-			node->prefetch_iterator &&
+			!node->pf_iterator.exhausted &&
 			node->pfblockno < node->blockno)
 			elog(ERROR, "prefetch and main iterators are out of sync");
 
@@ -317,21 +355,20 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node)
 {
 #ifdef USE_PREFETCH
 	ParallelBitmapHeapState *pstate = node->pstate;
+	BitmapHeapIterator *prefetch_iterator = &node->pf_iterator;
 	TBMIterateResult *tbmpre;
 
 	if (pstate == NULL)
 	{
-		TBMIterator *prefetch_iterator = node->prefetch_iterator;
-
 		if (node->prefetch_pages > 0)
 		{
 			/* The main iterator has closed the distance by one page */
 			node->prefetch_pages--;
 		}
-		else if (prefetch_iterator)
+		else if (!prefetch_iterator->exhausted)
 		{
 			/* Do not let the prefetch iterator get behind the main one */
-			tbmpre = tbm_iterate(prefetch_iterator);
+			tbmpre = bhs_iterate(prefetch_iterator);
 			node->pfblockno = tbmpre ? tbmpre->blockno : InvalidBlockNumber;
 		}
 		return;
@@ -344,8 +381,6 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node)
 	 */
 	if (node->prefetch_maximum > 0)
 	{
-		TBMSharedIterator *prefetch_iterator = node->shared_prefetch_iterator;
-
 		SpinLockAcquire(&pstate->mutex);
 		if (pstate->prefetch_pages > 0)
 		{
@@ -365,9 +400,9 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node)
 			 * we don't validate the blockno here as we do in non-parallel
 			 * case.
 			 */
-			if (prefetch_iterator)
+			if (!prefetch_iterator->exhausted)
 			{
-				tbmpre = tbm_shared_iterate(prefetch_iterator);
+				tbmpre = bhs_iterate(prefetch_iterator);
 				node->pfblockno = tbmpre ? tbmpre->blockno : InvalidBlockNumber;
 			}
 		}
@@ -427,23 +462,21 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 {
 #ifdef USE_PREFETCH
 	ParallelBitmapHeapState *pstate = node->pstate;
+	BitmapHeapIterator *prefetch_iterator = &node->pf_iterator;
 
 	if (pstate == NULL)
 	{
-		TBMIterator *prefetch_iterator = node->prefetch_iterator;
-
-		if (prefetch_iterator)
+		if (!prefetch_iterator->exhausted)
 		{
 			while (node->prefetch_pages < node->prefetch_target)
 			{
-				TBMIterateResult *tbmpre = tbm_iterate(prefetch_iterator);
+				TBMIterateResult *tbmpre = bhs_iterate(prefetch_iterator);
 				bool		skip_fetch;
 
 				if (tbmpre == NULL)
 				{
 					/* No more pages to prefetch */
-					tbm_end_iterate(prefetch_iterator);
-					node->prefetch_iterator = NULL;
+					bhs_end_iterate(prefetch_iterator);
 					break;
 				}
 				node->prefetch_pages++;
@@ -471,9 +504,7 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 
 	if (pstate->prefetch_pages < pstate->prefetch_target)
 	{
-		TBMSharedIterator *prefetch_iterator = node->shared_prefetch_iterator;
-
-		if (prefetch_iterator)
+		if (!prefetch_iterator->exhausted)
 		{
 			while (1)
 			{
@@ -496,12 +527,11 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 				if (!do_prefetch)
 					return;
 
-				tbmpre = tbm_shared_iterate(prefetch_iterator);
+				tbmpre = bhs_iterate(prefetch_iterator);
 				if (tbmpre == NULL)
 				{
 					/* No more pages to prefetch */
-					tbm_end_shared_iterate(prefetch_iterator);
-					node->shared_prefetch_iterator = NULL;
+					bhs_end_iterate(prefetch_iterator);
 					break;
 				}
 
@@ -568,18 +598,14 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 		table_rescan(node->ss.ss_currentScanDesc, NULL);
 
 	/* release bitmaps and buffers if any */
-	if (node->prefetch_iterator)
-		tbm_end_iterate(node->prefetch_iterator);
-	if (node->shared_prefetch_iterator)
-		tbm_end_shared_iterate(node->shared_prefetch_iterator);
+	if (node->pf_iterator.exhausted)
+		bhs_end_iterate(&node->pf_iterator);
 	if (node->tbm)
 		tbm_free(node->tbm);
 	if (node->pvmbuffer != InvalidBuffer)
 		ReleaseBuffer(node->pvmbuffer);
 	node->tbm = NULL;
-	node->prefetch_iterator = NULL;
 	node->initialized = false;
-	node->shared_prefetch_iterator = NULL;
 	node->pvmbuffer = InvalidBuffer;
 	node->recheck = true;
 	node->blockno = InvalidBlockNumber;
@@ -624,12 +650,10 @@ ExecEndBitmapHeapScan(BitmapHeapScanState *node)
 	/*
 	 * release bitmaps and buffers if any
 	 */
-	if (node->prefetch_iterator)
-		tbm_end_iterate(node->prefetch_iterator);
+	if (!node->pf_iterator.exhausted)
+		bhs_end_iterate(&node->pf_iterator);
 	if (node->tbm)
 		tbm_free(node->tbm);
-	if (node->shared_prefetch_iterator)
-		tbm_end_shared_iterate(node->shared_prefetch_iterator);
 	if (node->pvmbuffer != InvalidBuffer)
 		ReleaseBuffer(node->pvmbuffer);
 }
@@ -667,11 +691,9 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	scanstate->pvmbuffer = InvalidBuffer;
 	scanstate->exact_pages = 0;
 	scanstate->lossy_pages = 0;
-	scanstate->prefetch_iterator = NULL;
 	scanstate->prefetch_pages = 0;
 	scanstate->prefetch_target = 0;
 	scanstate->initialized = false;
-	scanstate->shared_prefetch_iterator = NULL;
 	scanstate->pstate = NULL;
 	scanstate->recheck = true;
 	scanstate->blockno = InvalidBlockNumber;
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index 92b829cebc..e520186b41 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -20,13 +20,11 @@
 #include "storage/buf.h"
 #include "storage/spin.h"
 #include "utils/relcache.h"
+#include "executor/nodeBitmapHeapscan.h"
 
 
 struct ParallelTableScanDescData;
 
-struct TBMIterator;
-struct TBMSharedIterator;
-
 /*
  * Generic descriptor for table scans. This is the base-class for table scans,
  * which needs to be embedded in the scans of individual AMs.
@@ -44,8 +42,7 @@ typedef struct TableScanDescData
 	ItemPointerData rs_maxtid;
 
 	/* Only used for Bitmap table scans */
-	struct TBMIterator *tbmiterator;
-	struct TBMSharedIterator *shared_tbmiterator;
+	BitmapHeapIterator rs_bhs_iterator;
 
 	/*
 	 * Information about type and behaviour of the scan, a bitmask of members
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 42c67a128e..618ab2b449 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -938,13 +938,9 @@ static inline TableScanDesc
 table_beginscan_bm(Relation rel, Snapshot snapshot,
 				   int nkeys, struct ScanKeyData *key, uint32 extra_flags)
 {
-	TableScanDesc result;
 	uint32		flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE | extra_flags;
 
-	result = rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags);
-	result->shared_tbmiterator = NULL;
-	result->tbmiterator = NULL;
-	return result;
+	return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags);
 }
 
 /*
@@ -991,20 +987,9 @@ table_beginscan_tid(Relation rel, Snapshot snapshot)
 static inline void
 table_endscan(TableScanDesc scan)
 {
-	if (scan->rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		if (scan->shared_tbmiterator)
-		{
-			tbm_end_shared_iterate(scan->shared_tbmiterator);
-			scan->shared_tbmiterator = NULL;
-		}
-
-		if (scan->tbmiterator)
-		{
-			tbm_end_iterate(scan->tbmiterator);
-			scan->tbmiterator = NULL;
-		}
-	}
+	if (scan->rs_flags & SO_TYPE_BITMAPSCAN &&
+		!scan->rs_bhs_iterator.exhausted)
+		bhs_end_iterate(&scan->rs_bhs_iterator);
 
 	scan->rs_rd->rd_tableam->scan_end(scan);
 }
@@ -1016,20 +1001,9 @@ static inline void
 table_rescan(TableScanDesc scan,
 			 struct ScanKeyData *key)
 {
-	if (scan->rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		if (scan->shared_tbmiterator)
-		{
-			tbm_end_shared_iterate(scan->shared_tbmiterator);
-			scan->shared_tbmiterator = NULL;
-		}
-
-		if (scan->tbmiterator)
-		{
-			tbm_end_iterate(scan->tbmiterator);
-			scan->tbmiterator = NULL;
-		}
-	}
+	if (scan->rs_flags & SO_TYPE_BITMAPSCAN &&
+		!scan->rs_bhs_iterator.exhausted)
+		bhs_end_iterate(&scan->rs_bhs_iterator);
 
 	scan->rs_rd->rd_tableam->scan_rescan(scan, key, false, false, false, false);
 }
diff --git a/src/include/executor/nodeBitmapHeapscan.h b/src/include/executor/nodeBitmapHeapscan.h
index ea003a9caa..7064f54686 100644
--- a/src/include/executor/nodeBitmapHeapscan.h
+++ b/src/include/executor/nodeBitmapHeapscan.h
@@ -29,4 +29,11 @@ extern void ExecBitmapHeapReInitializeDSM(BitmapHeapScanState *node,
 extern void ExecBitmapHeapInitializeWorker(BitmapHeapScanState *node,
 										   ParallelWorkerContext *pwcxt);
 
+extern TBMIterateResult *bhs_iterate(BitmapHeapIterator *iterator);
+extern void bhs_begin_iterate(BitmapHeapIterator *iterator, TIDBitmap *tbm,
+							  dsa_area *dsa, dsa_pointer dsp);
+
+extern void bhs_end_iterate(BitmapHeapIterator *iterator);
+
+
 #endif							/* NODEBITMAPHEAPSCAN_H */
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d96703b04d..714eb3e534 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1787,6 +1787,13 @@ typedef struct ParallelBitmapHeapState
 	ConditionVariable cv;
 } ParallelBitmapHeapState;
 
+typedef struct BitmapHeapIterator
+{
+	struct TBMIterator *serial;
+	struct TBMSharedIterator *parallel;
+	bool		exhausted;
+} BitmapHeapIterator;
+
 /* ----------------
  *	 BitmapHeapScanState information
  *
@@ -1795,12 +1802,11 @@ typedef struct ParallelBitmapHeapState
  *		pvmbuffer		   buffer for visibility-map lookups of prefetched pages
  *		exact_pages		   total number of exact pages retrieved
  *		lossy_pages		   total number of lossy pages retrieved
- *		prefetch_iterator  iterator for prefetching ahead of current page
+ *		pf_iterator        for prefetching ahead of current page
  *		prefetch_pages	   # pages prefetch iterator is ahead of current
  *		prefetch_target    current target prefetch distance
  *		prefetch_maximum   maximum value for prefetch_target
  *		initialized		   is node is ready to iterate
- *		shared_prefetch_iterator shared iterator for prefetching
  *		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
@@ -1815,12 +1821,11 @@ typedef struct BitmapHeapScanState
 	Buffer		pvmbuffer;
 	long		exact_pages;
 	long		lossy_pages;
-	TBMIterator *prefetch_iterator;
 	int			prefetch_pages;
 	int			prefetch_target;
 	int			prefetch_maximum;
 	bool		initialized;
-	TBMSharedIterator *shared_prefetch_iterator;
+	BitmapHeapIterator pf_iterator;
 	ParallelBitmapHeapState *pstate;
 	bool		recheck;
 	BlockNumber blockno;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 01845ee71d..848ce9b30c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -259,6 +259,7 @@ BitString
 BitmapAnd
 BitmapAndPath
 BitmapAndState
+BitmapHeapIterator
 BitmapHeapPath
 BitmapHeapScan
 BitmapHeapScanState
-- 
2.40.1

