Re: Support tid range scan in parallel?

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