On Tue, 10 Jun 2025 at 11:04, Cary Huang <cary.huang@highgo.ca> wrote:
> I have addressed your comment in the attached v6 patch. Thank you again for
> the review.
Here's a review of v6:
1. In cost_tidrangescan() you're dividing the total costs by the
number of workers yet the comment is claiming that's CPU cost. I think
this needs to follow the lead of cost_seqscan() and separate out the
CPU and IO cost then add the IO cost at the end, after the divide by
the number of workers.
2. In execParallel.c, could you move the case for T_TidRangeScanState
below T_ForeignScanState? What you have right now is now quite
following the unwritten standard set out by the other nodes, i.e
non-parallel aware nodes are last. A good spot seems to be putting it
at the end of the scan types... Custom scan seems slightly misplaced,
but probably can ignore that one and put it after T_ForeignScanState
3. The following comment should mention what behaviour occurs when the
field is set to InvalidBlockNumber:
BlockNumber phs_numblock; /* max number of blocks to scan */
Something like /* # of blocks to scan, or InvalidBlockNumber if no limit */
4. I think the following would be clearer if written using an else if:
if (nallocated >= pbscan->phs_nblocks || (pbscan->phs_numblock !=
InvalidBlockNumber && nallocated >= pbscan->phs_numblock))
page = InvalidBlockNumber; /* all blocks have been allocated */
else
page = (nallocated + pbscan->phs_startblock) % pbscan->phs_nblocks;
e.g:
if (nallocated >= pbscan->phs_nblocks)
page = InvalidBlockNumber; /* all blocks have been allocated */
else if (pbscan->phs_numblock != InvalidBlockNumber &&
nallocated >= pbscan->phs_numblock)
page = InvalidBlockNumber; /* upper scan limit reached */
else
page = (nallocated + pbscan->phs_startblock) % pbscan->phs_nblocks;
That way the comment after the assignment is accurate.
5. For the tests, is there any reason not to reuse the tidrangescan table?
I don't see any other issues, but I've not tested the patch yet. I'll
do that if you can fix the 5 above.
Thanks
David
