Postgres Pro
Downloads
Home   >   mailing lists

Re: [HACKERS] parallelize queries containing initplans - Mailing list pgsql-hackers

From Haribabu Kommi
Subject Re: [HACKERS] parallelize queries containing initplans
Date
Msg-id CAJrrPGc5Os4ejb=FPCn_moLiprEQiHvp-J22TBaDHTcHEyut-g@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] parallelize queries containing initplans  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: [HACKERS] parallelize queries containing initplans
List pgsql-hackers
Tree view


On Fri, Aug 11, 2017 at 1:18 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 9, 2017 at 6:51 PM, Haribabu Kommi <kommi.haribabu@gmail.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

pgsql-hackers by date:

Previous
From: Christoph Berg
Date:
Subject: [HACKERS] initdb failure on Debian sid/mips64el in EventTriggerEndCompleteQuery
Next
From: Haribabu Kommi
Date:
Subject: Re: [HACKERS] Pluggable storage