Re: Parallelize correlated subqueries that execute within each worker

On Tue, Jan 9, 2024 at 2:09 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> ...
> > Given all of that I settled on this approach:
> > 1. Modify is_parallel_safe() to by default ignore PARAM_EXEC params.
> > 2. Add is_parallel_safe_with_params() that checks for the existence of
> > such params.
> > 3. Store the required params in a bitmapset on each base rel.
> > 4. Union the bitmapset on join rels.
> > 5. Only insert a gather node if that bitmapset is empty.
> >
> > I have an intuition that there's some spot (e.g. joins) that we should
> > be removing params from this set (e.g., when we've satisfied them),
> > but I haven't been able to come up with such a scenario as yet.
> >
> > The attached v11 fixes the issue you reported.
>
> One of the tests has failed in CFBot at [1] with:
> +++ /tmp/cirrus-ci-build/build/testrun/pg_upgrade/002_pg_upgrade/data/results/select_parallel.out
> 2023-12-20 20:08:42.480004000 +0000
> @@ -137,23 +137,24 @@
> explain (costs off)
> select (select max((select pa1.b from part_pa_test pa1 where pa1.a = pa2.a)))
> from part_pa_test pa2;
> - QUERY PLAN
> ---------------------------------------------------------------
> - Aggregate
> + QUERY PLAN
> +--------------------------------------------------------------------
> + Finalize Aggregate
> -> Gather
> Workers Planned: 3
> - -> Parallel Append
> - -> Parallel Seq Scan on part_pa_test_p1 pa2_1
> - -> Parallel Seq Scan on part_pa_test_p2 pa2_2
> + -> Partial Aggregate
> + -> Parallel Append
> + -> Parallel Seq Scan on part_pa_test_p1 pa2_1
> + -> Parallel Seq Scan on part_pa_test_p2 pa2_2
> + SubPlan 1
> + -> Append
> + -> Seq Scan on part_pa_test_p1 pa1_1
> + Filter: (a = pa2.a)
> + -> Seq Scan on part_pa_test_p2 pa1_2
> + Filter: (a = pa2.a)
> SubPlan 2
> -> Result
> - SubPlan 1
> - -> Append
> - -> Seq Scan on part_pa_test_p1 pa1_1
> - Filter: (a = pa2.a)
> - -> Seq Scan on part_pa_test_p2 pa1_2
> - Filter: (a = pa2.a)
> -(14 rows)
> +(15 rows)
>
> More details of the failure is available at [2].
>
> [1] - https://cirrus-ci.com/task/5685696451575808
> [2] - https://api.cirrus-ci.com/v1/artifact/task/5685696451575808/testrun/build/testrun/pg_upgrade/002_pg_upgrade/log/regress_log_002_pg_upgrade

Thanks for noting this here.

I've finally had a chance to look at this, and I don't believe there's
any real failure here, merely drift of how the planner works on master
resulting in this query now being eligible for a different plan shape.

I was a bit wary at first because the changing test query is one I'd
previously referenced in [1] as likely exposing the bug I'd fixed
where params where being used across worker boundaries. However
looking at the diff in the patch at that point (v10) that particular
test query formed a different plan shape (there were two gather nodes
being created, and params crossing between them).

But in the current revision of master with the current patch applied
that's no longer true: we have a Gather node, and the Subplan using
the param is properly under that Gather node, and the param should be
being both generated and consumed within the same worker process.

So I've updated the patch to show that plan change as part of the diff.

See attached v12

Regards,
James Coleman

1: https://www.postgresql.org/message-id/CAAaqYe-_TObm5KwmZLYXBJ3BJGh4cUZWM0v1mY1gWTMkRNQXDQ%40mail.gmail.com