>+ /* Check if the scan for current scan keys is finished */
>+ if (so->arrayKeyCount < btscan->btps_arrayKeyCount)
>+ *status = false;
>I didn't clearly understand, in which scenario the arrayKeyCount is less
>than btps_arrayKeyCount?
Consider following array scan keys
select * from test2 where j=ANY(ARRAY[1000,2000,3000]);
By the time the current worker has finished reading heap tuples
corresponding
to array key 1000(arrayKeyCount = 0), some other worker might have advanced
the scan to the
array key 2000(btps_arrayKeyCount =1). In this case when the current worker
fetches next page to scan,
it must advance its scan keys before scanning the next page of parallel
scan.
I hope this helps.
>+BlockNumber
>+_bt_parallel_seize(IndexScanDesc scan, bool *status)
>The return value of the above function is validated only in _bt_first
>function, but in other cases it is not.
In other cases it is validated in _bt_readnextpage() which is called after
_bt_parallel_seize().
>From the function description,
>it is possible to return P_NONE for the workers also with status as
>true. I feel it is better to handle the P_NONE case internally only
>so that callers just check for the status. Am i missing anything?
In case of the next block being P_NONE and status true, the code
calls _bt_parallel_done() to notify other workers followed by
BTScanPosInvalidate(). Similar check for block = P_NONE also
happens in existing code. See following in _bt_readnextpage(),
if (blkno == P_NONE || !so->currPos.moreRight)
{
_bt_parallel_done(scan);
BTScanPosInvalidate(so->currPos);
return false;
}
So to keep it consistent with the existing code, the check
is kept outside _bt_parallel_seize().
Thank you,
Rahila Syed
On Wed, Jan 18, 2017 at 6:25 AM, Haribabu Kommi <kommi.harib...@gmail.com>
wrote:
>
>
> On Mon, Jan 16, 2017 at 11:11 PM, Amit Kapila <amit.kapil...@gmail.com>
> wrote:
>
>>
>> Changed as per suggestion.
>>
>>
>> I have also rebased the optimizer/executor support patch
>> (parallel_index_opt_exec_support_v4.patch) and added a test case in
>> it.
>
>
> Thanks for the patch. Here are comments found during review.
>
> parallel_index_scan_v4.patch:
>
>
> + amtarget = (char *) ((void *) target) + offset;
>
> The above calcuation can be moved after NULL check?
>
> + * index_beginscan_parallel - join parallel index scan
>
> The name and the description doesn't sync properly, any better description?
>
> + BTPARALLEL_DONE,
> + BTPARALLEL_IDLE
> +} PS_State;
>
> The order of above two enum values can be changed according to their use.
>
> + /* Check if the scan for current scan keys is finished */
> + if (so->arrayKeyCount < btscan->btps_arrayKeyCount)
> + *status = false;
>
> I didn't clearly understand, in which scenario the arrayKeyCount is less
> than btps_arrayKeyCount?
>
>
> +BlockNumber
> +_bt_parallel_seize(IndexScanDesc scan, bool *status)
>
> The return value of the above function is validated only in _bt_first
> function, but in other cases it is not. From the function description,
> it is possible to return P_NONE for the workers also with status as
> true. I feel it is better to handle the P_NONE case internally only
> so that callers just check for the status. Am i missing anything?
>
>
> +extern BlockNumber _bt_parallel_seize(IndexScanDesc scan, bool *status);
> +extern void _bt_parallel_release(IndexScanDesc scan, BlockNumber
> scan_page);
>
> Any better names for the above functions, as these function will
> provide/set
> the next page that needs to be read.
>
>
> parallel_index_opt_exec_support_v4.patch:
>
> +#include "access/parallel.h"
>
> Why is it required to be include nbtree.c? i didn't find
> any code changes in the patch.
>
>
> + /* reset (parallel) index scan */
> + if (node->iss_ScanDesc)
> + {
>
> Why this if check required? There is an assert check in later function
> calls.
>
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
