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
