On Tue, Jan 10, 2017 at 11:55 AM, Dilip Kumar <dilipbal...@gmail.com> wrote: > On Wed, Dec 28, 2016 at 11:47 AM, Amit Kapila <amit.kapil...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers