Re: [HACKERS] parallelize queries containing subplans

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers