Re: Inadequate parallel-safety check for SubPlans

On Wed, Apr 19, 2017 at 1:27 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> I have ended up doing something along the lines suggested by you (or
>> at least what I have understood from your e-mail). Basically, pass
>> the safe-param-ids list to parallel safety function and decide based
>> on that if Param node reference in input expression is safe.
>
> I did not like changing the signature of is_parallel_safe() for this:
> that's getting a lot of call sites involved in something they don't care
> about, and I'm not even sure that it's formally correct. The key thing
> here is that a Param could only be considered parallel-safe in context.
> That is, if you looked at the testexpr alone and asked if it could be
> pushed down to a worker, the answer would have to be "no". Only when
> you look at the whole SubPlan tree and ask if it can be pushed down
> should the answer be "yes". Therefore, I feel pretty strongly that
> what we want is for the safe_param_ids whitelist to be mutated
> internally in is_parallel_safe() as it descends the tree. That way
> it can give the right answer when considering a fragment of a plan.
> I've committed a patch that does it like that.
>

Thanks.

>> ... Also, I think we don't need to check
>> parallel-safety for splan->args as that also is related to correlated
>> queries for which parallelism is not supported as of now.
>
> I think leaving that sort of thing out is just creating a latent bug
> that is certain to bite you later. It's true that as long as the args
> list contains only Vars, it would never be parallel-unsafe --- but
> primnodes.h is pretty clear that one shouldn't assume that it will
> stay that way.

Sure, but the point I was trying to make was whenever subplan has
args, I think it won't be parallel-safe as those args are used to pass
params.

> So it's better to recurse over the whole tree rather
> than ignoring parts of it, especially if you're not going to document
> the assumption you're making anywhere.
>

No problem.

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