Amit Kapila <amit.kapila16@gmail.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.
> ... 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. 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.
> I have also explored it to do it by checking parallel-safety of
> testexpr before PARAM_EXEC params are replaced in testexpr, however
> that won't work because we add PARAM_SUBLINK params at the time of
> parse-analysis in transformSubLink().
AFAICS we don't do parallel-safety checks early enough to need to bother
with that, so the existing handling of SubLink and PARAM_SUBLINK params
should be fine.
regards, tom lane
