On Wed, 17 Mar 2021 at 15:31, Andres Freund <[email protected]> wrote:
> I'm a bit confused about the precise design of rs_private /
> ParallelBlockTableScanWorkerData, specifically why it's been added to
> TableScanDesc, instead of just adding it to HeapScanDesc? And why is it
> allocated unconditionally, instead of just for parallel scans?
That's a good point. In hindsight, I didn't spend enough effort
questioning that design in the original patch. I see now that the
rs_private field makes very little sense as we can just store what's
private to heapam in HeapScanDescData.
I've done that in the attached. I added the
ParallelBlockTableScanWorkerData as a pointer field in
HeapScanDescData and change it so we only allocate memory for it for
just parallel scans. The field is left as NULL for non-parallel
scans.
I've also added a pfree in heap_endscan() to free the memory when the
pointer is not NULL. I'm hoping that'll fix the valgrind warning, but
I've not run it to check.
David
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 90711b2fcd..595310ba1b 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -540,7 +540,7 @@ heapgettup(HeapScanDesc scan,
ParallelBlockTableScanDesc pbscan =
(ParallelBlockTableScanDesc)
scan->rs_base.rs_parallel;
ParallelBlockTableScanWorker pbscanwork =
- (ParallelBlockTableScanWorker)
scan->rs_base.rs_private;
+ scan->rs_parallelworkerdata;
table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
pbscanwork, pbscan);
@@ -748,7 +748,7 @@ heapgettup(HeapScanDesc scan,
ParallelBlockTableScanDesc pbscan =
(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
ParallelBlockTableScanWorker pbscanwork =
- (ParallelBlockTableScanWorker) scan->rs_base.rs_private;
+ scan->rs_parallelworkerdata;
page =
table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
pbscanwork, pbscan);
@@ -864,7 +864,7 @@ heapgettup_pagemode(HeapScanDesc scan,
ParallelBlockTableScanDesc pbscan =
(ParallelBlockTableScanDesc)
scan->rs_base.rs_parallel;
ParallelBlockTableScanWorker pbscanwork =
- (ParallelBlockTableScanWorker)
scan->rs_base.rs_private;
+ scan->rs_parallelworkerdata;
table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
pbscanwork, pbscan);
@@ -1057,7 +1057,7 @@ heapgettup_pagemode(HeapScanDesc scan,
ParallelBlockTableScanDesc pbscan =
(ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
ParallelBlockTableScanWorker pbscanwork =
- (ParallelBlockTableScanWorker) scan->rs_base.rs_private;
+ scan->rs_parallelworkerdata;
page =
table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
pbscanwork, pbscan);
@@ -1194,8 +1194,6 @@ heap_beginscan(Relation relation, Snapshot snapshot,
scan->rs_base.rs_nkeys = nkeys;
scan->rs_base.rs_flags = flags;
scan->rs_base.rs_parallel = parallel_scan;
- scan->rs_base.rs_private =
- palloc(sizeof(ParallelBlockTableScanWorkerData));
scan->rs_strategy = NULL; /* set in initscan */
/*
@@ -1231,6 +1229,15 @@ heap_beginscan(Relation relation, Snapshot snapshot,
/* we only need to set this up once */
scan->rs_ctup.t_tableOid = RelationGetRelid(relation);
+ /*
+ * Allocate memory to keep track of page allocation for parallel workers
+ * when doing a parallel scan.
+ */
+ if (parallel_scan != NULL)
+ scan->rs_parallelworkerdata =
palloc(sizeof(ParallelBlockTableScanWorkerData));
+ else
+ scan->rs_parallelworkerdata = NULL;
+
/*
* we do this here instead of in initscan() because heap_rescan also
calls
* initscan() and we don't want to allocate memory again
@@ -1306,6 +1313,9 @@ heap_endscan(TableScanDesc sscan)
if (scan->rs_strategy != NULL)
FreeAccessStrategy(scan->rs_strategy);
+ if (scan->rs_parallelworkerdata != NULL)
+ pfree(scan->rs_parallelworkerdata);
+
if (scan->rs_base.rs_flags & SO_TEMP_SNAPSHOT)
UnregisterSnapshot(scan->rs_base.rs_snapshot);
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index bc0936bc2d..d803f27787 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -65,6 +65,12 @@ typedef struct HeapScanDescData
HeapTupleData rs_ctup; /* current tuple in scan, if any */
+ /*
+ * For parallel scans to store page allocation data. NULL when not
+ * performing a parallel scan.
+ */
+ 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 */
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index 0ef6d8edf7..17a161c69a 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -46,7 +46,6 @@ typedef struct TableScanDescData
*/
uint32 rs_flags;
- void *rs_private; /* per-worker private memory for AM to
use */
struct ParallelTableScanDescData *rs_parallel; /* parallel scan
* information */
} TableScanDescData;
