Re: parallelize queries containing subplans

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Dilip Kumar <dilipbalaut(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: parallelize queries containing subplans
Date: 2017-02-14 04:07:08
Message-ID: CAA4eK1J9mDZLcp-OskkdzAf_yT8W4dBSGL9E=koEiJkdpVZsEA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Thu, Feb 2, 2017 at 9:23 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Feb 1, 2017 at 10:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Jan 31, 2017 at 6:01 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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

Attachment Content-Type Size
pq_pushdown_subplan_v5.patch application/octet-stream 9.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2017-02-14 04:35:52 Re: possibility to specify template database for pg_regress
Previous Message Robert Haas 2017-02-14 04:01:16 Re: Removal of deprecated views pg_user, pg_group, pg_shadow