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
