On Wed, Aug 9, 2017 at 6:51 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
> On Wed, Aug 9, 2017 at 9:26 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>
> By the way, I tested the patch with by DML support for parallel patch to
> check the returning of clause of insert, and all the returning clause init
> plans
> are parallel plans with this patch.
>
Good to know.
>>
>> By the way, the patch doesn't apply on HEAD, so attached rebased patch.
>
>
>
> Thanks for the updated patch. I have some comments and I am yet to finish
> the review.
>
> + /*
> + * We don't want to generate gather or gather merge node if there are
> + * initplans at some query level below the current query level as those
> + * plans could be parallel-unsafe or undirect correlated plans. Ensure to
> + * mark all the partial and non-partial paths for a relation at this level
> + * to be parallel-unsafe.
> + */
> + if (is_initplan_below_current_query_level(root))
> + {
> + foreach(lc, rel->partial_pathlist)
> + {
> + Path *subpath = (Path *) lfirst(lc);
> +
> + subpath->parallel_safe = false;
> + }
> +
> + foreach(lc, rel->pathlist)
> + {
> + Path *subpath = (Path *) lfirst(lc);
> +
> + subpath->parallel_safe = false;
> + }
> + return;
> + }
> +
>
> The above part of the code is almost same in mutiple places, is it possible
> to change to function?
>
Sure, we can do that and I think that makes sense as well, so changed
accordingly in the attached patch.
>
> + node->initParam = NULL;
>
> This new parameter is set to NULL in make_gather function, the same
> parameter
> is added to GatherMerge structure also, but anyway this parameter is set to
> NULL
> makeNode macro, why only setting it to here, but not the other place.
>
> Do we need to set it to default value such as NULL or false if it is already
> the same value?
> This is not related to the above parameter, for all existing parameters
> also.
>
Strictly speaking, it is not required, but I have initialised at only
one of the place to make it consistent to near by code. We already do
similar stuff in some other functions like make_seqscan,
make_samplescan, ..
I don't see any value in trying to initialize it for GatherMerge as
well, so left it as it is.
>
> + 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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
