Re: Inadequate parallel-safety check for SubPlans

On Thu, Apr 13, 2017 at 1:05 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> This is 100% wrong. It's failing to recurse into the subexpressions of
>>> the SubPlan, which could very easily include parallel-unsafe function
>>> calls.
>
>> My understanding (apparently flawed?) is that the parallel_safe flag
>> on the SubPlan is supposed to encapsulate whether than SubPlan is
>> entirely parallel safe, so that no recursion is needed. If the
>> parallel_safe flag on the SubPlan is being set wrong, doesn't that
>> imply that the Path from which the subplan was created had the wrong
>> value of this flag, too?
>
>> ...
>
>> Oh, I see. The testexpr is separate from the Path. Oops.
>
> Right. Although you're nudging up against an alternate idea that
> I just had: we could define the parallel_safe flag on the SubPlan as
> encapsulating not only whether the child plan is safe, but whether
> the contained testexpr (and args) are safe. If we were to make
> an is_parallel_safe() check on the testexpr before injecting
> PARAM_EXEC Params into it, and record the results in the SubPlan,
> maybe that would lead to a simpler answer.
>
> OTOH, that might not work out nicely; and I have a feeling that
> we'd end up needing a whitelist anyway later, to handle the
> case of correlated subplans.
>

The problem with supporting correlated subplans is that it is not easy
to decide about params that belong to whitelist because we need to
ensure that such params are available below the gather node. It is
quite clear how whitelist of params can be useful for the case in hand
but it is not immediately clear to me how we will use it for
correlated sublans. However, if you have ideas on how to make it
work, I can try to draft a patch as well on those lines as it can
certainly help some TPC-H queries.

>> Sounds reasonable. Do you want to have a go at that?
>
> Yeah. I'm knocking off for the day a bit early today, but I'll have
> a look at it tomorrow, unless anyone in the Far East beats me to it.
>

Thanks for looking into it.

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