Re: Parallel INSERT (INTO ... SELECT ...)

On Tue, Feb 23, 2021 at 10:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Feb 23, 2021 at 4:47 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> >
> > On Tue, Feb 23, 2021 at 2:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Feb 23, 2021 at 6:37 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > > >
> > > > On Tue, Feb 23, 2021 at 12:33 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Mon, Feb 22, 2021 at 8:41 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > On Fri, Feb 19, 2021 at 9:38 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > > > >
> > > > > > > On Thu, Feb 18, 2021 at 11:05 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > > > > > >
> > > > > > > > > > It also occurred to me that we can avoid pointless adding of
> > > > > > > > > > partitions if the final plan won't use parallelism. For that, the
> > > > > > > > > > patch adds checking glob->parallelModeNeeded, which seems to do the
> > > > > > > > > > trick though I don't know if that's the correct way of doing that.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I'm not sure if's pointless adding partitions even in the case of NOT
> > > > > > > > > using parallelism, because we may be relying on the result of
> > > > > > > > > parallel-safety checks on partitions in both cases.
> > > > > > > > > For example, insert_parallel.sql currently includes a test (that you
> > > > > > > > > originally provided in a previous post) that checks a non-parallel
> > > > > > > > > plan is generated after a parallel-unsafe trigger is created on a
> > > > > > > > > partition involved in the INSERT.
> > > > > > > > > If I further add to that test by then dropping that trigger and then
> > > > > > > > > again using EXPLAIN to see what plan is generated, then I'd expect a
> > > > > > > > > parallel-plan to be generated, but with the setrefs-v3.patch it still
> > > > > > > > > generates a non-parallel plan. So I think the "&&
> > > > > > > > > glob->parallelModeNeeded" part of test needs to be removed.
> > > > > > > >
> > > > > > > > Ah, okay, I didn't retest my case after making that change.
> > > > > > > >
> > > > > > >
> > > > > > > Greg has point here but I feel something on previous lines (having a
> > > > > > > test of glob->parallelModeNeeded) is better. We only want to
> > > > > > > invalidate the plan if the prepared plan is unsafe to execute next
> > > > > > > time. It is quite possible that there are unsafe triggers on different
> > > > > > > partitions and only one of them is dropped, so next time planning will
> > > > > > > again yield to the same non-parallel plan. If we agree with that I
> > > > > > > think it is better to add this dependency in set_plan_refs (along with
> > > > > > > Gather node handling).
> > > > > > >
> > > > > >
> > > > > > I think we should try to be consistent with current plan-cache
> > > > > > functionality, rather than possibly inventing new rules for
> > > > > > partitions.
> > > > > > I'm finding that with current Postgres functionality (master branch),
> > > > > > if there are two parallel-unsafe triggers defined on a normal table
> > > > > > and one is dropped, it results in replanning and it yields the same
> > > > > > (non-parallel) plan.
> > > > > >
> > > > >
> > > > > Does such a plan have partitions access in it? Can you share the plan?
> > > > >
> > > >
> > > > Er, no (it's just a regular table), but that was exactly my point:
> > > > aren't you suggesting functionality for partitions that doesn't seem
> > > > to work the same way for non-partitions?
> > > >
> > >
> > > I don't think so. The non-parallel plan for Insert doesn't directly
> > > depend on partitions so we don't need to invalidate those.
> > >
> >
> > But the non-parallel plan was chosen (instead of a parallel plan)
> > because of parallel-safety checks on the partitions, which found
> > attributes of the partitions which weren't parallel-safe.
> > So it's not so clear to me that the dependency doesn't exist - the
> > non-parallel plan does in fact depend on the state of the partitions.
> >
>
> Hmm, I think that is not what we can consider as a dependency.
>

Then if it's not a dependency, then we shouldn't have to check the
attributes of the partitions for parallel-safety, to determine whether
we must use a non-parallel plan (or can use a parallel plan).
Except, of course, we do have to ...

> > I know you're suggesting to reduce plan-cache invalidation by only
> > recording a dependency in the parallel-plan case, but I've yet to see
> > that in the existing code, and in fact it seems to be inconsistent
> > with the behaviour I've tested so far (one example given prior, but
> > will look for more).
> >
>
> I don't see your example matches what you are saying as in it the
> regular table exists in the plan whereas for the case we are
> discussing partitions doesn't exist in the plan. Amit L. seems to have
> given a correct explanation [1] of why we don't need to invalidate for
> non-parallel plans which match my understanding.
>
> [1] - https://www.postgresql.org/message-id/CA%2BHiwqFKJfzgBbkg0i0Fz_FGsCiXW-Fw0tBjdsaUbNbpyv0JhA%40mail.gmail.com
>

Amit L's explanation was:

I may be wrong but it doesn't seem to me that the possibility of
constructing a better plan due to a given change is enough reason for
plancache.c to invalidate plans that depend on that change. AIUI,
plancache.c only considers a change interesting if it would *break* a
Query or a plan.

So in this case, a non-parallel plan may be slower, but it isn't
exactly rendered *wrong* by changes that make a parallel plan
possible.

This explanation doesn't seem to match existing planner behavior
AFAICS, and we should try to be consistent with existing behavior
(rather than doing something different, for partitions specifically in
this case).

Using a concrete example, to avoid any confusion, consider the
following SQL (using unpatched Postgres, master branch):

-- encourage use of parallel plans, where possible
set parallel_setup_cost=0;
set parallel_tuple_cost=0;
set min_parallel_table_scan_size=0;
set max_parallel_workers_per_gather=4;

create or replace function myfunc() returns boolean as $$
begin
return true;
end;
$$ language plpgsql parallel unsafe immutable;

create table mytable(x int);
insert into mytable(x) values(generate_series(1,10));

prepare q as select * from mytable, myfunc();
explain execute q;

-- change myfunc to be parallel-safe, to see the effect
-- on the planner for the same select
create or replace function myfunc() returns boolean as $$
begin
return true;
end;
$$ language plpgsql parallel safe immutable;

explain execute q;

Here a function referenced in the SELECT statement is changed from
parallel-unsafe to parallel-safe, to see the effect on plancache.
According to your referenced explanation, that shouldn't be considered
an "interesting" change by plancache.c, as it wouldn't "break" the
previously planned and cached (non-parallel) query - the existing
non-parallel plan could and should be used, as it still would execute
OK, even if slower. Right?

BUT - for the above example, it DOES cause the query to be replanned,
and it then uses a parallel plan, instead of keeping the original
non-parallel plan.
This does not match the explanation about how plancache works.

Output is below:

test=# \i test.sql
-- encourage use of parallel plans, where possible
set parallel_setup_cost=0;
SET
set parallel_tuple_cost=0;
SET
set min_parallel_table_scan_size=0;
SET
set max_parallel_workers_per_gather=4;
SET
create or replace function myfunc() returns boolean as $$
begin
return true;
end;
$$ language plpgsql parallel unsafe immutable;
CREATE FUNCTION
create table mytable(x int);
CREATE TABLE
insert into mytable(x) values(generate_series(1,10));
INSERT 0 10
prepare q as select * from mytable, myfunc();
PREPARE
explain execute q;
QUERY PLAN
-----------------------------------------------------------
Seq Scan on mytable (cost=0.00..35.50 rows=2550 width=5)
(1 row)

-- change myfunc to be parallel-safe, to see the effect
-- on the planner for the same select
create or replace function myfunc() returns boolean as $$
begin
return true;
end;
$$ language plpgsql parallel safe immutable;
CREATE FUNCTION
explain execute q;
QUERY PLAN
-------------------------------------------------------------------------
Gather (cost=0.00..18.23 rows=2550 width=5)
Workers Planned: 3
-> Parallel Seq Scan on mytable (cost=0.00..18.23 rows=823 width=5)
(3 rows)

So what I am saying is why should this behavior be different for our
partition case?
Doing something different for partitions would be inconsistent with
this behavior, would it not?

Going back to my original example, I pointed out that if there are two
parallel-unsafe triggers defined on a normal table and one is dropped,
it results in replanning and it yields the same (non-parallel) plan.
If the 2nd trigger is dropped, it also results in replanning (and with
patch applied, will yield a parallel plan).
And with the functionality you are suggesting (only add partition OIDs
as dependencies in the case of using a parallel plan), the behavior
for partitions (as opposed to a normal table) will be different. Drop
one or two of the parallel-unsafe triggers and it won't result in
replanning and will still use the original non-parallel plan.
So why should the behavior for partitions be different here?

Regards,
Greg Nancarrow
Fujitsu Australia