Re: Support tid range scan in parallel?

Hi Cary,

On Fri, Jun 6, 2025 at 5:24 AM Cary Huang <cary(dot)huang(at)highgo(dot)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