Re: parallelize queries containing initplans

On Fri, Aug 11, 2017 at 1:18 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Wed, Aug 9, 2017 at 6:51 PM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
> wrote:
> >
> >
> > + if (IsA(plan, Gather))
> > + ((Gather *) plan)->initParam = bms_intersect(plan->lefttree->extParam,
> > initSetParam);
> > + else if (IsA(plan, GatherMerge))
> > + ((GatherMerge *) plan)->initParam =
> > bms_intersect(plan->lefttree->extParam, initSetParam);
> > + else
> > + elog(ERROR, "unrecognized node type: %d", nodeTag(plan));
> >
> > The else case is not possible, because it is already validated for
> Gather or
> > GatherMerge.
> > Can we change it simple if and else?
> >
>
> As we already have an assert in this function to protect from any
> other node type (nodes other than Gather and Gather Merge), it makes
> sense to change the code to just if...else, so changed accordingly.

Thanks for the updated patch. Patch looks fine. I just have some
minor comments.

+ * ExecEvalParamExecParams
+ *
+ * Execute the subplan stored in PARAM_EXEC initplans params, if not
executed
+ * till now.
+ */
+void
+ExecEvalParamExecParams(Bitmapset *params, EState *estate)

I feel it is better to explain when this function executes the sub plans
that are
not executed till now? Means like in what scenario?

+ if (params == NULL)
+ nparams = 0;
+ else
+ nparams = bms_num_members(params);

bms_num_members return 0 in case if the params is NULL.
Is it fine to keep the specific check for NULL is required for performance
benefit
or just remove it? Anything is fine for me.

+ if (IsA(plan, Gather))
+ ((Gather *) plan)->initParam = bms_intersect(plan->lefttree->extParam,
initSetParam);
+ else
+ ((GatherMerge *) plan)->initParam =
bms_intersect(plan->lefttree->extParam, initSetParam);

I think the above code is to find out the common parameters that are prsent
in the external
and out params. It may be better to explain the logic in the comments.

Regards,
Hari Babu
Fujitsu Australia