On Fri, Dec 23, 2016 at 5:48 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:
>>> 5. Comment for _bt_parallel_seize() says:
>>> "False indicates that we have reached the end of scan for
>>> current scankeys and for that we return block number as P_NONE."
>>>
>>> What is the reason to check (blkno == P_NONE) after checking (status ==
>>> false)
>>> in _bt_first() (see code below)? If comment is correct
>>> we'll never reach _bt_parallel_done()
>>>
>>> + blkno = _bt_parallel_seize(scan, &status);
>>> + if (status == false)
>>> + {
>>> + BTScanPosInvalidate(so->currPos);
>>> + return false;
>>> + }
>>> + else if (blkno == P_NONE)
>>> + {
>>> + _bt_parallel_done(scan);
>>> + BTScanPosInvalidate(so->currPos);
>>> + return false;
>>> + }
>>>
>>The first time master backend or worker hits last page (calls this
>>API), it will return P_NONE and after that any worker tries to fetch
>>next page, it will return status as false. I think we can expand a
>>comment to explain it clearly. Let me know, if you need more
>>clarification, I can explain it in detail.
>
> Probably this was confusing because we have not mentioned
> that P_NONE can be returned even when status = TRUE and
> not just when status is false.
>
> I think, the comment above the function can be modified as follows,
>
> + /*
> + * True indicates that the block number returned is either valid including
> P_NONE
> + * and scan is continued or block number is invalid and scan has just
> + * begun.
>
I think the modification (including P_NONE and scan is continued)
suggested by you can confuse the reader, because if the returned block
number is P_NONE, then we don't continue the scan. I have used
slightly different words in the patch I have just posted, please check
and see if that looks fine to you.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com