Re: Support tid range scan in parallel?

Hi, Cary,

I have two comments:
1. Does table_beginscan_parallel_tidrange() need an assert of relid,
like what table_beginscan_parallel() did?
Assert(RelationGetRelid(relation) == pscan->phs_relid);

2. The new field phs_numblock in ParallelBlockTableScanDescData
structure has almost the same name as another field phs_nblocks. Would
you consider changing it to another name, for example,
phs_maxnumblocktoscan?

Thanks,
Steven


在 2025/6/10 7:04, Cary Huang 写道:
> Hi Junwang
>
> Thank you so much for the review!
>
>> + /*
>   > + * 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.
>
> Agreed, made the adjustment in the attached patch.
>
>
>   > + 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.
>
> Also agreed, phs_numblock should be initialized in
> table_block_parallelscan_initialize just like all other parameters in parallel scan
> context. You are right, it is much neater to use InvalidBlockNumber rather
> than 0 to indicate if an upper bound has been specified in the TID range scan.
>
> I have addressed your comment in the attached v6 patch. Thank you again for
> the review.
>
> Best regards
> Cary Huang
>