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
>
