Re: Parallel Index Scans

On Mon, Jan 16, 2017 at 11:11 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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