Re: parallelize queries containing subplans

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.
>

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?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com