From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: parallelize queries containing subplans |
Date: | 2017-01-12 03:52:03 |
Message-ID: | CAA4eK1LRF6iJh5wK4ZRHT_Y6Bqyzn6D8dqgcUs+vaN3hYc9sXQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jan 10, 2017 at 11:55 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Attached patch implements the above idea. This will enable
>> parallelism for queries containing un-correlated subplans, an example
>> of which is as follows:
>
> I have reviewed the patch (pq_pushdown_subplan_v1.patch), Mostly patch
> looks clean to me.
> I have also done some basic functional testing and it's working fine.
>
> I have one comment.
>
> + else if (IsA(node, AlternativeSubPlan))
> + {
> + AlternativeSubPlan *asplan = (AlternativeSubPlan *) node;
> + ListCell *lc;
> +
> + Assert(context->root);
> +
> + foreach(lc, asplan->subplans)
> + {
> + SubPlan *splan = (SubPlan *) lfirst(lc);
> + Plan *plan;
> +
> + Assert(IsA(splan, SubPlan));
> +
> + plan = planner_subplan_get_plan(context->root, splan);
> + if (!plan->parallel_safe)
> + return true;
> + }
> }
>
> I see no reason why we need to process the subplan list of
> AlternativeSubPlan here, Anyway expression_tree_walker will take care
> of processing each subplan of AlternativeSubPlan. Now in the case
> where all the subplans in AlternativeSubPlan are parallel_safe we will
> process this list twice.
>
Valid point, but I think we can avoid that by returning false after
foreach(..) loop. I think one improvement could be that instead of
manually checking the parallel safety of each subplan, we can
recursively call max_parallel_hazard_walker for each subplan.
> But, more than that it will be cleaner to not
> handle AlternativeSubPlan here unless there is some strong reason?
>
Yeah, the reason is to avoid unnecessary recursion. Also, in all
other places in the code, we do handle both of them separately, refer
cost_qual_eval_walker for somewhat similar usage.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2017-01-12 03:53:04 | Re: Declarative partitioning - another take |
Previous Message | Thomas Munro | 2017-01-12 03:37:28 | Re: WIP: [[Parallel] Shared] Hash |