On Thu, Feb 2, 2017 at 9:23 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Wed, Feb 1, 2017 at 10:04 PM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Tue, Jan 31, 2017 at 6:01 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>>> Moved this patch to next CF.
>>
>> So here is what seems to be the key hunk from this patch:
>>
>> /*
>> - * Since we don't have the ability to push subplans down to workers at
>> - * present, we treat subplan references as parallel-restricted. We need
>> - * not worry about examining their contents; if they are unsafe, we
>> would
>> - * have found that out while examining the whole tree before reduction
>> of
>> - * sublinks to subplans. (Really we should not see SubLink during a
>> - * max_interesting == restricted scan, but if we do, return true.)
>> + * We can push the subplans only if they don't contain any
>> parallel-aware
>> + * node as we don't support multi-level parallelism (parallel workers
>> + * invoking another set of parallel workers).
>> */
>> - else if (IsA(node, SubLink) ||
>> - IsA(node, SubPlan) ||
>> - IsA(node, AlternativeSubPlan))
>> + else if (IsA(node, SubPlan))
>> + return !((SubPlan *) node)->parallel_safe;
>> + else if (IsA(node, AlternativeSubPlan))
>> {
>> - if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
>> - return true;
>> + AlternativeSubPlan *asplan = (AlternativeSubPlan *) node;
>> + ListCell *lc;
>> +
>> + foreach(lc, asplan->subplans)
>> + {
>> + SubPlan *splan = (SubPlan *) lfirst(lc);
>> +
>> + Assert(IsA(splan, SubPlan));
>> +
>> + if (max_parallel_hazard_walker((Node *) splan, context))
>> + return true;
>> + }
>> +
>> + return false;
>> }
>>
>> I don't see the reason for having this new code that makes
>> AlternativeSubPlan walk over the subplans; expression_tree_walker
>> already does that.
>>I have removed the check of AlternativeSubPlan so that it can be handled by expression_tree_walker. > > It is done this way mainly to avoid recursion/nested calls, but I > think you prefer to handle it via expression_tree_walker so that code > looks clean. Another probable reason could be that there is always a > chance that we miss handling of some expression of a particular node > (in this case AlternativeSubPlan), but if that is the case then there > are other places in the code which do operate on individual subplan/'s > in AlternativeSubPlan list. > >> On the flip side I don't see the reason for >> removing the max_parallel_hazard_test() call for AlternativeSubPlan or >> for SubLink. > > If we don't remove the current test of max_parallel_hazard_test() for > AlternativeSubPlan, then AlternativeSubPlan node will be considered > parallel restricted, so why do we want that after this patch. For > Sublinks, I think they would have converted to Subplans by the time we > reach this function for the parallel restricted check. Can you > elaborate what you have in mind for the handling of AlternativeSubPlan > and SubLink? > I have removed the changes for SubLink node. >> + * CTE scans are not consider for parallelism (cf >> >> >> considered >> Fixed. >> + select count(*)from tenk1 where (two, four) not in >> >> whitespace Fixed. Attached patch fixes all the comments raised till now. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
pq_pushdown_subplan_v5.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
