On Wed, 17 Mar 2021 at 15:31, Andres Freund <and...@anarazel.de> 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;