Hi Cary,
On Fri, Jun 6, 2025 at 5:24 AM Cary Huang <cary.hu...@highgo.ca> wrote:
>
> Hello
>
> Sorry David and all who have reviewed the patch, it's been awhile since the
> patch
> was last worked on :(. Thank you all for the reviews and comments! Attached is
> the rebased patch that adds support for parallel TID range scans. This
> feature is
> particularly useful scanning large tables where the data needs to be scanned
> in
> sizable segments using a TID range in the WHERE clause to define each segment.
> By enabling parallelism, this approach can improve performance compared to
> both non-parallel TID range scans and traditional sequential scans.
>
> Regards
>
> Cary Huang
Thanks for updating the patch. I have a few comments on it.
+ /*
+ * if parallel mode is used, store startblock and numblocks in parallel
+ * scan descriptor as well.
+ */
+ if (scan->rs_base.rs_parallel != NULL)
+ {
+ ParallelBlockTableScanDesc bpscan = NULL;
+
+ bpscan = (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
+ bpscan->phs_startblock = scan->rs_startblock;
+ bpscan->phs_numblock = scan->rs_numblocks;
+ }
It would be more intuitive and clearer to directly use startBlk and numBlks
to set these values. Since scan->rs_startblock and scan->rs_numblocks
are already set using these variables, using the same approach for bpscan
would make the code easier to understand.
Another nitty-gritty is that you might want to use a capital `If` in the
comments to maintain the same style.
+ if (nallocated >= pbscan->phs_nblocks || (pbscan->phs_numblock != 0 &&
+ nallocated >= pbscan->phs_numblock))
I'd suggest explictly setting phs_numblock to InvalidBlockNumber in
table_block_parallelscan_initialize, and compare with InvalidBlockNumber
here.
--
Regards
Junwang Zhao
