Re: Inadequate parallel-safety check for SubPlans

On Wed, Apr 12, 2017 at 02:41:09PM -0400, Tom Lane 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. Example:
>
> regression=# explain select * from tenk1 where int4(unique1 + random()) not in (select ten from tenk2);
> QUERY PLAN
> -----------------------------------------------------------------------------
> Gather (cost=470.00..884.25 rows=5000 width=244)
> Workers Planned: 4
> -> Parallel Seq Scan on tenk1 (cost=470.00..884.25 rows=1250 width=244)
> Filter: (NOT (hashed SubPlan 1))
> SubPlan 1
> -> Seq Scan on tenk2 (cost=0.00..445.00 rows=10000 width=4)
>
> random() is parallel-restricted so it's not cool that the SubPlan was
> allowed to be passed down to workers.
>
> I tried to fix it like this:
>
> else if (IsA(node, SubPlan))
> {
> if (!((SubPlan *) node)->parallel_safe &&
> max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
> return true;
> }
>
> but got some failures in the regression tests due to things not getting
> parallelized when expected. Poking into that, I remembered that for some
> SubPlans, the testexpr contains Params representing the output columns
> of the sub-select. So the recursive call of max_parallel_hazard_walker
> visits those and deems the expression parallel-restricted.
>
> 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.

The above-described topic is currently a PostgreSQL 10 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, I will look for an update
according to your current blanket status update[1]:

I will begin investigating this no later than April 24th, my first day back
from vacation, and will provide a further update by that same day.

Thanks.

[1] https://www.postgresql.org/message-id/CA+Tgmoa1-529KFwdB-+A1eG2MFc3j9eqJenBUFU=MsT-H9Q8BQ@mail.gmail.com