v7-0002-fixup-Parallel-Bitmap-Heap-Scan-reports-per-worke.patch

From 6b6776c895d3ddb4a9aa0423c41fb71d26f2661e Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Fri, 5 Jul 2024 01:33:47 +1200
Subject: [PATCH v7 2/2] fixup! Parallel Bitmap Heap Scan reports per-worker
 stats

---
 src/backend/commands/explain.c            |  8 ++++--
 src/backend/executor/nodeBitmapHeapscan.c | 30 +++++++++++++-------
 src/include/nodes/execnodes.h             | 34 +++++++++++------------
 src/tools/pgindent/typedefs.list          |  2 ++
 4 files changed, 44 insertions(+), 30 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d6f66dc5e1..f5634d3c0d 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3628,8 +3628,12 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
 			if (es->format == EXPLAIN_FORMAT_TEXT)
 			{
 				ExplainIndentText(es);
-				appendStringInfo(es->str, "Heap Blocks: exact=%ld lossy=%ld\n",
-						 si->exact_pages, si->lossy_pages);
+				appendStringInfoString(es->str, "Heap Blocks:");
+				if (si->exact_pages > 0)
+					appendStringInfo(es->str, " exact=%ld", si->exact_pages);
+				if (si->lossy_pages > 0)
+					appendStringInfo(es->str, " lossy=%ld", si->lossy_pages);
+				appendStringInfoChar(es->str, '\n');
 			}
 			else
 			{
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 973bf56022..3c63bdd93d 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -634,8 +634,18 @@ ExecEndBitmapHeapScan(BitmapHeapScanState *node)
 	 */
 	if (node->sinstrument != NULL && IsParallelWorker())
 	{
+		BitmapHeapScanInstrumentation *si;
+
 		Assert(ParallelWorkerNumber <= node->sinstrument->num_workers);
-		BitmapHeapScanInstrumentation *si = &node->sinstrument->sinstrument[ParallelWorkerNumber];
+		si = &node->sinstrument->sinstrument[ParallelWorkerNumber];
+
+		/*
+		 * Here we accumulate the stats rather than performing memcpy on
+		 * node->stats into si.  When a Gather/GatherMerge node finishes it
+		 * will perform planner shutdown on the workers.  On rescan it will
+		 * spin up new workers which will have a new BitmapHeapScanState and
+		 * zeroed stats.
+		 */
 		si->exact_pages += node->stats.exact_pages;
 		si->lossy_pages += node->stats.lossy_pages;
 	}
@@ -707,8 +717,10 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	scanstate->tbmiterator = NULL;
 	scanstate->tbmres = NULL;
 	scanstate->pvmbuffer = InvalidBuffer;
-	scanstate->stats.exact_pages = 0;
-	scanstate->stats.lossy_pages = 0;
+
+	/* Zero the statistics counters */
+	memset(&scanstate->stats, 0, sizeof(BitmapHeapScanInstrumentation));
+
 	scanstate->prefetch_iterator = NULL;
 	scanstate->prefetch_pages = 0;
 	scanstate->prefetch_target = 0;
@@ -818,13 +830,9 @@ ExecBitmapHeapEstimate(BitmapHeapScanState *node,
 {
 	Size		size;
 
-	/*
-	 * We store ParallelBitmapHeapState followed by
-	 * SharedBitmapHeapInstrumentationInfo in the same shmem chunk.
-	 */
 	size = MAXALIGN(sizeof(ParallelBitmapHeapState));
 
-	/* don't need this if not instrumenting or no workers */
+	/* account for instrumentation, if required */
 	if (node->ss.ps.instrument && pcxt->nworkers > 0)
 	{
 		size = add_size(size, offsetof(SharedBitmapHeapInstrumentation, sinstrument));
@@ -879,11 +887,13 @@ ExecBitmapHeapInitializeDSM(BitmapHeapScanState *node,
 
 	ConditionVariableInit(&pstate->cv);
 
-	/* ensure any unfilled slots will contain zeroes */
 	if (sinstrument)
 	{
 		sinstrument->num_workers = pcxt->nworkers;
-		memset(sinstrument->sinstrument, 0, pcxt->nworkers * sizeof(BitmapHeapScanInstrumentation));
+
+		/* ensure any unfilled slots will contain zeroes */
+		memset(sinstrument->sinstrument, 0,
+			   pcxt->nworkers * sizeof(BitmapHeapScanInstrumentation));
 	}
 
 	shm_toc_insert(pcxt->toc, node->ss.ps.plan->plan_node_id, pstate);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f8a6956ee8..8b1239ff4f 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1746,10 +1746,17 @@ typedef struct BitmapIndexScanState
 	struct IndexScanDescData *biss_ScanDesc;
 } BitmapIndexScanState;
 
+/* ----------------
+ *	 BitmapHeapScanInstrumentation information
+ *
+ *		exact_pages		   total number of exact pages retrieved
+ *		lossy_pages		   total number of lossy pages retrieved
+ * ----------------
+ */
 typedef struct BitmapHeapScanInstrumentation
 {
-	long lossy_pages;
-	long exact_pages;
+	long		exact_pages;
+	long		lossy_pages;
 } BitmapHeapScanInstrumentation;
 
 /* ----------------
@@ -1798,10 +1805,10 @@ typedef struct ParallelBitmapHeapState
 /* ----------------
  *	 Instrumentation data for a parallel bitmap heap scan.
  *
- * During a parallel bitmap heap scan, this lives in shared memory.  At the
- * end, each worker copies their own stats to the right slot.  Finally, the
- * leader copies the data to its local memory
-  * ----------------
+ * A shared memory struct that each parallel worker copies its
+ * BitmapHeapScanInstrumentation information into at executor shutdown to
+ * allow the leader to display the information in EXPLAIN ANALYZE.
+ * ----------------
  */
 typedef struct SharedBitmapHeapInstrumentation
 {
@@ -1817,8 +1824,7 @@ typedef struct SharedBitmapHeapInstrumentation
  *		tbmiterator		   iterator for scanning current pages
  *		tbmres			   current-page data
  *		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
+ *		stats			   execution statistics
  *		prefetch_iterator  iterator for prefetching ahead of current page
  *		prefetch_pages	   # pages prefetch iterator is ahead of current
  *		prefetch_target    current target prefetch distance
@@ -1827,6 +1833,7 @@ typedef struct SharedBitmapHeapInstrumentation
  *		shared_tbmiterator	   shared iterator
  *		shared_prefetch_iterator shared iterator for prefetching
  *		pstate			   shared state for parallel bitmap scan
+ *		sinstrument		   statistics for parallel workers
  * ----------------
  */
 typedef struct BitmapHeapScanState
@@ -1837,6 +1844,7 @@ typedef struct BitmapHeapScanState
 	TBMIterator *tbmiterator;
 	TBMIterateResult *tbmres;
 	Buffer		pvmbuffer;
+	BitmapHeapScanInstrumentation stats;
 	TBMIterator *prefetch_iterator;
 	int			prefetch_pages;
 	int			prefetch_target;
@@ -1845,17 +1853,7 @@ typedef struct BitmapHeapScanState
 	TBMSharedIterator *shared_tbmiterator;
 	TBMSharedIterator *shared_prefetch_iterator;
 	ParallelBitmapHeapState *pstate;
-
-	/*
-	 * In a parallelized bitmap heap scan, each worker sets their
-	 * instrumentaton data in pstate->sinstrument at the end.  The leader
-	 * copies the shared-memory info back to local storage before DSM
-	 * shutdown, and stores it here in 'instrument'.  It remains NULL in
-	 * workers and in non-parallel scans.
-	 */
 	SharedBitmapHeapInstrumentation *sinstrument;
-
-	BitmapHeapScanInstrumentation stats;
 } BitmapHeapScanState;
 
 /* ----------------
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index e710fa48e5..3ad15485e9 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -262,6 +262,7 @@ BitmapAndPath
 BitmapAndState
 BitmapHeapPath
 BitmapHeapScan
+BitmapHeapScanInstrumentation
 BitmapHeapScanState
 BitmapIndexScan
 BitmapIndexScanState
@@ -2603,6 +2604,7 @@ SetToDefault
 SetupWorkerPtrType
 ShDependObjectInfo
 SharedAggInfo
+SharedBitmapHeapInstrumentation
 SharedBitmapState
 SharedDependencyObjectType
 SharedDependencyType
-- 
2.34.1