Re: Inadequate parallel-safety check for SubPlans

On Wed, Apr 12, 2017 at 2:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> While poking at the question of parallel_safe marking for Plans,
> I noticed that max_parallel_hazard_walker() does this:
>
> /* We can push the subplans only if they are parallel-safe. */
> else if (IsA(node, SubPlan))
> return !((SubPlan *) node)->parallel_safe;
>
> 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.

> Basically, therefore, this logic is totally inadequate. I think what
> we need to do is improve matters so that while looking at the testexpr
> of a SubPlan, we pass down a whitelist saying that the Params having
> the numbers indicated by the SubLink's paramIds list are OK.

Sounds reasonable. Do you want to have a go at that?

> I'm also suspicious that the existing dumb treatment of Params here
> may be blocking recognition that correlated subplans are parallel-safe.
> But that would only be a failure to apply a possible optimization,
> not a failure to generate a valid plan.

Smarter treatment of Params would be very nice, but possibly hard.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company