Обсуждение: wrong Append/MergeAppend elision?

Hi,

It seems that the planner currently elides an Append/MergeAppend that
has run-time pruning info (part_prune_index) set, but which I think is
a bug.  Here's an example:

create table p (a int) partition by list (a);
create table p1 partition of p for values in (1);
set plan_cache_mode to force_generic_plan ;
prepare q as select * from p where a = $1;
explain execute q (0);
                      QUERY PLAN
------------------------------------------------------
 Seq Scan on p1 p  (cost=0.00..41.88 rows=13 width=4)
   Filter: (a = $1)
(2 rows)

Because the Append is elided in this case, run-time pruning doesn't
kick in to prune p1, even though PartitionPruneInfo to do so has been
generated.

Attached find a patch to fix that.  There are some expected output
diffs in partition_prune suite, though they all look sane to me.

Thoughts?

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Fri, 27 Jan 2023 at 01:30, Amit Langote <amitlangote09@gmail.com> wrote:
> It seems that the planner currently elides an Append/MergeAppend that
> has run-time pruning info (part_prune_index) set, but which I think is
> a bug.

This is actually how I intended it to work. Whether it was a good idea
or not, I'm currently unsure. I mentioned it in [1].

I think the plan shapes I was talking about were some ordered paths
from partial paths per what is being added right at the end of
add_paths_to_append_rel().  However, now that I look at it again, I'm
not sure why it wouldn't be correct to still have those paths with a
single-child Append. Certainly, the "if (list_length(live_childrels)
== 1)" test made in add_paths_to_append_rel() is no longer aligned to
the equivalent test in set_append_references(), so it's possible even
now that we make a plan that uses the extra sorted partial paths added
in add_paths_to_append_rel() and still have the Append in the final
plan.

There is still the trade-off of having to pull tuples through the
Append node for when run-time pruning is unable to prune the last
partition. So your proposal to leave the Append alone when there's
run-time pruning info is certainly not a no-brainer.

[1] https://www.postgresql.org/message-id/CAKJS1f_utf1Mbp8UeoByAarziO4e4qb4Z8FksurpaM+3Q_HOmQ@mail.gmail.com

David Rowley <dgrowleyml@gmail.com> writes:
> On Fri, 27 Jan 2023 at 01:30, Amit Langote <amitlangote09@gmail.com> wrote:
>> It seems that the planner currently elides an Append/MergeAppend that
>> has run-time pruning info (part_prune_index) set, but which I think is
>> a bug.

> There is still the trade-off of having to pull tuples through the
> Append node for when run-time pruning is unable to prune the last
> partition. So your proposal to leave the Append alone when there's
> run-time pruning info is certainly not a no-brainer.

Yeah.  Amit's proposal amounts to optimizing for the case that all
partitions get pruned, which does not seem to me to be the way
to bet.  I'm inclined to think it's fine as-is.

            regards, tom lane

On Fri, Jan 27, 2023 at 5:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <dgrowleyml@gmail.com> writes:
> > On Fri, 27 Jan 2023 at 01:30, Amit Langote <amitlangote09@gmail.com> wrote:
> >> It seems that the planner currently elides an Append/MergeAppend that
> >> has run-time pruning info (part_prune_index) set, but which I think is
> >> a bug.
>
> > There is still the trade-off of having to pull tuples through the
> > Append node for when run-time pruning is unable to prune the last
> > partition. So your proposal to leave the Append alone when there's
> > run-time pruning info is certainly not a no-brainer.
>
> Yeah.  Amit's proposal amounts to optimizing for the case that all
> partitions get pruned, which does not seem to me to be the way
> to bet.  I'm inclined to think it's fine as-is.

Fair enough.  I thought for a second that maybe it was simply an
oversight but David confirms otherwise.  This was interacting badly
with the other patch I'm working on and I just figured out the problem
was with that other patch.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com