v14-0013-Unify-parallel-and-serial-BitmapHeapScan-iterato.patch

From 53362e02e069747a413aa11cf25df918a94b1cc1 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Thu, 4 Apr 2024 16:05:53 +0200
Subject: [PATCH v14 13/19] 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(). Now, on the first scan, the the
iterator is created after the scan descriptor is created.
---
 src/backend/access/heap/heapam_handler.c  |   5 +-
 src/backend/executor/nodeBitmapHeapscan.c | 163 ++++++++++++----------
 src/include/access/relscan.h              |   7 +-
 src/include/access/tableam.h              |  29 +---
 src/include/executor/nodeBitmapHeapscan.h |  10 ++
 src/include/nodes/execnodes.h             |   8 +-
 src/tools/pgindent/typedefs.list          |   1 +
 7 files changed, 114 insertions(+), 109 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index eea3b7f149e..e086cbd2cfd 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2207,10 +2207,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 5f987b40ede..a1f9acc2cfb 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -56,6 +56,56 @@ static inline void BitmapAdjustPrefetchTarget(BitmapHeapScanState *node);
 static inline void BitmapPrefetch(BitmapHeapScanState *node,
 								  TableScanDesc scan);
 static bool BitmapShouldInitializeSharedState(ParallelBitmapHeapState *pstate);
+static BitmapHeapIterator *bhs_begin_iterate(TIDBitmap *tbm,
+											 dsa_pointer shared_area,
+											 dsa_area *personal_area);
+
+BitmapHeapIterator *
+bhs_begin_iterate(TIDBitmap *tbm, dsa_pointer shared_area, dsa_area *personal_area)
+{
+	BitmapHeapIterator *result = palloc(sizeof(BitmapHeapIterator));
+
+	result->serial = NULL;
+	result->parallel = NULL;
+
+	/* Allocate a private iterator and attach the shared state to it */
+	if (DsaPointerIsValid(shared_area))
+		result->parallel = tbm_attach_shared_iterate(personal_area, shared_area);
+	else
+		result->serial = tbm_begin_iterate(tbm);
+
+	return result;
+}
+
+TBMIterateResult *
+bhs_iterate(BitmapHeapIterator *iterator)
+{
+	Assert(iterator);
+
+	if (iterator->serial)
+		return tbm_iterate(iterator->serial);
+	else
+		return tbm_shared_iterate(iterator->parallel);
+}
+
+void
+bhs_end_iterate(BitmapHeapIterator *iterator)
+{
+	Assert(iterator);
+
+	if (iterator->serial)
+	{
+		tbm_end_iterate(iterator->serial);
+		iterator->serial = NULL;
+	}
+	else
+	{
+		tbm_end_shared_iterate(iterator->parallel);
+		iterator->parallel = NULL;
+	}
+
+	pfree(iterator);
+}
 
 
 /* ----------------------------------------------------------------
@@ -98,46 +148,22 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	if (!node->initialized)
 	{
 		/*
-		 * XXX why don't we assign directly to the node-> fields? Then we
-		 * would not need these local variables at all.
+		 * The leader will immediately come out of the function, but others
+		 * will be blocked until leader populates the TBM and wakes them up.
 		 */
-		TBMIterator *tbmiterator = NULL;
-		TBMSharedIterator *shared_tbmiterator = NULL;
+		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)
-			{
-				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))
+			if (init_shared_state)
 			{
-				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
@@ -158,21 +184,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 */
 		}
 
 		/*
@@ -203,8 +217,21 @@ BitmapHeapNext(BitmapHeapScanState *node)
 			node->ss.ss_currentScanDesc = scan;
 		}
 
-		scan->tbmiterator = tbmiterator;
-		scan->shared_tbmiterator = shared_tbmiterator;
+		scan->rs_bhs_iterator = bhs_begin_iterate(tbm,
+												  pstate ? pstate->tbmiterator : InvalidDsaPointer,
+												  dsa);
+
+#ifdef USE_PREFETCH
+		if (node->prefetch_maximum > 0)
+		{
+			node->pf_iterator = bhs_begin_iterate(tbm,
+												  pstate ? pstate->prefetch_iterator : InvalidDsaPointer,
+												  dsa);
+			/* Only used for serial BHS */
+			node->prefetch_pages = 0;
+			node->prefetch_target = -1;
+		}
+#endif							/* USE_PREFETCH */
 
 		node->initialized = true;
 
@@ -285,7 +312,7 @@ new_page:
 		 * ahead of the current block.
 		 */
 		if (node->pstate == NULL &&
-			node->prefetch_iterator &&
+			node->pf_iterator &&
 			node->pfblockno < node->blockno)
 			elog(ERROR, "prefetch and main iterators are out of sync");
 
@@ -326,12 +353,11 @@ 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 */
@@ -340,7 +366,7 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node)
 		else if (prefetch_iterator)
 		{
 			/* 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;
@@ -353,8 +379,6 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node)
 	 */
 	if (node->prefetch_maximum > 0)
 	{
-		TBMSharedIterator *prefetch_iterator = node->shared_prefetch_iterator;
-
 		SpinLockAcquire(&pstate->mutex);
 		if (pstate->prefetch_pages > 0)
 		{
@@ -376,7 +400,7 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node)
 			 */
 			if (prefetch_iterator)
 			{
-				tbmpre = tbm_shared_iterate(prefetch_iterator);
+				tbmpre = bhs_iterate(prefetch_iterator);
 				node->pfblockno = tbmpre ? tbmpre->blockno : InvalidBlockNumber;
 			}
 		}
@@ -436,23 +460,22 @@ 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)
 		{
 			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);
+					node->pf_iterator = NULL;
 					break;
 				}
 				node->prefetch_pages++;
@@ -480,8 +503,6 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 
 	if (pstate->prefetch_pages < pstate->prefetch_target)
 	{
-		TBMSharedIterator *prefetch_iterator = node->shared_prefetch_iterator;
-
 		if (prefetch_iterator)
 		{
 			while (1)
@@ -505,12 +526,12 @@ 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);
+					node->pf_iterator = NULL;
 					break;
 				}
 
@@ -577,18 +598,17 @@ 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)
+	{
+		bhs_end_iterate(node->pf_iterator);
+		node->pf_iterator = NULL;
+	}
 	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;
@@ -633,12 +653,10 @@ ExecEndBitmapHeapScan(BitmapHeapScanState *node)
 	/*
 	 * release bitmaps and buffers if any
 	 */
-	if (node->prefetch_iterator)
-		tbm_end_iterate(node->prefetch_iterator);
+	if (node->pf_iterator)
+		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);
 }
@@ -676,11 +694,10 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	scanstate->pvmbuffer = InvalidBuffer;
 	scanstate->exact_pages = 0;
 	scanstate->lossy_pages = 0;
-	scanstate->prefetch_iterator = NULL;
+	scanstate->pf_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 92b829cebc7..fb22f305bf6 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -20,12 +20,12 @@
 #include "storage/buf.h"
 #include "storage/spin.h"
 #include "utils/relcache.h"
+#include "executor/nodeBitmapHeapscan.h"
 
 
 struct ParallelTableScanDescData;
 
-struct TBMIterator;
-struct TBMSharedIterator;
+struct BitmapHeapIterator;
 
 /*
  * Generic descriptor for table scans. This is the base-class for table scans,
@@ -44,8 +44,7 @@ typedef struct TableScanDescData
 	ItemPointerData rs_maxtid;
 
 	/* Only used for Bitmap table scans */
-	struct TBMIterator *tbmiterator;
-	struct TBMSharedIterator *shared_tbmiterator;
+	struct 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 74ddd5d66d3..d4fbf0d889b 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -942,8 +942,7 @@ table_beginscan_bm(Relation rel, Snapshot snapshot,
 	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;
+	result->rs_bhs_iterator = NULL;
 	return result;
 }
 
@@ -993,17 +992,8 @@ 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;
-		}
+		bhs_end_iterate(scan->rs_bhs_iterator);
+		scan->rs_bhs_iterator = NULL;
 	}
 
 	scan->rs_rd->rd_tableam->scan_end(scan);
@@ -1018,17 +1008,8 @@ table_rescan(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;
-		}
+		bhs_end_iterate(scan->rs_bhs_iterator);
+		scan->rs_bhs_iterator = NULL;
 	}
 
 	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 ea003a9caae..cb56d20dc6f 100644
--- a/src/include/executor/nodeBitmapHeapscan.h
+++ b/src/include/executor/nodeBitmapHeapscan.h
@@ -28,5 +28,15 @@ extern void ExecBitmapHeapReInitializeDSM(BitmapHeapScanState *node,
 										  ParallelContext *pcxt);
 extern void ExecBitmapHeapInitializeWorker(BitmapHeapScanState *node,
 										   ParallelWorkerContext *pwcxt);
+typedef struct BitmapHeapIterator
+{
+	struct TBMIterator *serial;
+	struct TBMSharedIterator *parallel;
+} BitmapHeapIterator;
+
+extern TBMIterateResult *bhs_iterate(BitmapHeapIterator *iterator);
+
+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 8e344155679..cf8b4995f0d 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1787,6 +1787,8 @@ typedef struct ParallelBitmapHeapState
 	ConditionVariable cv;
 } ParallelBitmapHeapState;
 
+struct BitmapHeapIterator;
+
 /* ----------------
  *	 BitmapHeapScanState information
  *
@@ -1795,12 +1797,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 +1816,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;
+	struct BitmapHeapIterator *pf_iterator;
 	ParallelBitmapHeapState *pstate;
 	bool		recheck;
 	BlockNumber blockno;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index b98e330e3ed..bcee567b65b 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.44.0