Horton

Thread

Commits

  1. Remove [Merge]AppendPath.partitioned_rels.

  2. Remove incidental dependencies on partitioned_rels lists.

  3. Revise make_partition_pruneinfo to not use its partitioned_rels input.

  4. Fix two issues in TOAST decompression.

  1. [sqlsmith] Failed assertion during partition pruning

    Andreas Seltenreich <seltenreich@gmx.de> — 2020-11-28T21:43:06Z

    Hi,

testing master at 3df51ca8 with sqlsmith triggers the following
assertion:

    TRAP: FailedAssertion("!bms_is_empty(present_parts)", File: "partprune.c", Line: 588, PID: 8540)

I looked at a dozen backtraces and they all sport a window aggregate but
that may still be random chance since sqlsmith really likes generating
these a lot... Below is the shortest recipe I found to reproduce it on a
fresh regression database:

--8<---------------cut here---------------start------------->8---
regression=# insert into trigger_parted values (1);
ERROR:  control reached end of trigger procedure without RETURN
CONTEXT:  PL/pgSQL function trigger_parted_trigfunc()
regression=# select myaggp05a(a) over (partition by a order by a) from trigger_parted where pg_trigger_depth() <> a limit 40;
server closed the connection unexpectedly
--8<---------------cut here---------------end--------------->8---

Backtrace of this one below.

regards,
Andreas

#2  0x000055bafaa95b81 in ExceptionalCondition (conditionName=conditionName@entry=0x55bafabe7812 "!bms_is_empty(present_parts)", errorType=errorType@entry=0x55bafaae901d "FailedAssertion", fileName=0x7ffee4fbbe40 "b[\251\372\272U",
    fileName@entry=0x55bafabe765b "partprune.c", lineNumber=lineNumber@entry=588) at assert.c:69
#3  0x000055bafa8f02c0 in make_partitionedrel_pruneinfo (matchedsubplans=<synthetic pointer>, prunequal=<optimized out>, partrelids=<optimized out>, relid_subplan_map=0x55bafba17fd0, parentrel=0x55bafb928050, root=0x55bafb9f4f78) at partprune.c:588
#4  make_partition_pruneinfo (root=root@entry=0x55bafb9f4f78, parentrel=parentrel@entry=0x55bafb928050, subpaths=0x55bafba16c10, partitioned_rels=0x55bafba16d58, prunequal=prunequal@entry=0x55bafba17f78) at partprune.c:274
#5  0x000055bafa8b05cd in create_append_plan (root=0x55bafb9f4f78, best_path=0x55bafba16cc0, flags=6) at createplan.c:1249
#6  0x000055bafa8acd5e in create_windowagg_plan (best_path=0x55bafba174c0, root=0x55bafb9f4f78) at createplan.c:2452
#7  create_plan_recurse (root=0x55bafb9f4f78, best_path=0x55bafba174c0, flags=1) at createplan.c:492
#8  0x000055bafa8ad341 in create_limit_plan (flags=1, best_path=0x55bafba17910, root=0x55bafb9f4f78) at createplan.c:2699
#9  create_plan_recurse (root=0x55bafb9f4f78, best_path=0x55bafba17910, flags=1) at createplan.c:514
#10 0x000055bafa8b00d1 in create_plan (root=root@entry=0x55bafb9f4f78, best_path=<optimized out>) at createplan.c:333
#11 0x000055bafa8bf013 in standard_planner (parse=0x55bafb9287a8, query_string=<optimized out>, cursorOptions=256, boundParams=<optimized out>) at planner.c:409
#12 0x000055bafa989da8 in pg_plan_query (querytree=0x55bafb9287a8, querytree@entry=0x7ffee4fbc620,
    query_string=query_string@entry=0x55bafb9082f0 "explain select myaggp05a(a) over (partition by a order by a) from trigger_parted where pg_trigger_depth() <> a limit 40;", cursorOptions=cursorOptions@entry=256, boundParams=boundParams@entry=0x0) at postgres.c:875
#13 0x000055bafa7b5dcf in ExplainOneQuery (query=0x7ffee4fbc620, cursorOptions=256, into=0x0, es=0x55bafb927f80, queryString=0x55bafb9082f0 "explain select myaggp05a(a) over (partition by a order by a) from trigger_parted where pg_trigger_depth() <> a limit 40;",
    params=0x0, queryEnv=0x0) at explain.c:391
#14 0x000055bafa7b6507 in ExplainQuery (pstate=0x55bafb92a1c0, stmt=0x55bafb909930, params=0x0, dest=0x55bafb92a128) at ../../../src/include/nodes/nodes.h:592
#15 0x000055bafa98f87d in standard_ProcessUtility (pstmt=0x55bafb9c6090, queryString=0x55bafb9082f0 "explain select myaggp05a(a) over (partition by a order by a) from trigger_parted where pg_trigger_depth() <> a limit 40;", context=PROCESS_UTILITY_TOPLEVEL,
    params=0x0, queryEnv=0x0, dest=0x55bafb92a128, qc=0x7ffee4fbc8c0) at utility.c:829
#16 0x000055bafa98cb36 in PortalRunUtility (portal=0x55bafb96b590, pstmt=0x55bafb9c6090, isTopLevel=<optimized out>, setHoldSnapshot=<optimized out>, dest=0x55bafb92a128, qc=0x7ffee4fbc8c0) at pquery.c:1159
#17 0x000055bafa98d910 in FillPortalStore (portal=0x55bafb96b590, isTopLevel=<optimized out>) at ../../../src/include/nodes/nodes.h:592
#18 0x000055bafa98e54d in PortalRun (portal=portal@entry=0x55bafb96b590, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true, dest=dest@entry=0x55bafb9c6180, altdest=altdest@entry=0x55bafb9c6180, qc=0x7ffee4fbcac0)
    at pquery.c:751
#19 0x000055bafa98a29c in exec_simple_query (query_string=0x55bafb9082f0 "explain select myaggp05a(a) over (partition by a order by a) from trigger_parted where pg_trigger_depth() <> a limit 40;") at postgres.c:1239
#20 0x000055bafa98beaa in PostgresMain (argc=argc@entry=1, argv=argv@entry=0x7ffee4fbcff0, dbname=<optimized out>, username=<optimized out>) at postgres.c:4308
#21 0x000055bafa9054fa in BackendRun (port=<optimized out>, port=<optimized out>) at postmaster.c:4488
#22 BackendStartup (port=<optimized out>) at postmaster.c:4210
#23 ServerLoop () at postmaster.c:1727
#24 0x000055bafa906452 in PostmasterMain (argc=<optimized out>, argv=0x55bafb902c70) at postmaster.c:1400
#25 0x000055bafa65e980 in main (argc=3, argv=0x55bafb902c70) at main.c:209

  2. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-28T22:52:28Z

    Andreas Seltenreich <seltenreich@gmx.de> writes:
> testing master at 3df51ca8 with sqlsmith triggers the following
> assertion:
>     TRAP: FailedAssertion("!bms_is_empty(present_parts)", File: "partprune.c", Line: 588, PID: 8540)

> I looked at a dozen backtraces and they all sport a window aggregate but
> that may still be random chance since sqlsmith really likes generating
> these a lot...

Yeah, it doesn't seem to need a window aggregate:

regression=# select a from trigger_parted where pg_trigger_depth() <> a order by a limit 40;
server closed the connection unexpectedly

What it looks like to me is that the code for setting up run-time
partition pruning has failed to consider the possibility of nested
partitioning: it's expecting that every partitioned table will have
at least one direct child that is a leaf.  I'm not sure though
whether just the Assert is wrong, or there's more fundamental
issues here.

It's also somewhat interesting that you need the "order by a limit 40"
to get a crash.  Poking around in the failing backend, I can see that
that causes the leaf-partition subplan to be an indexscan not a seqscan,
but it's far from clear why that'd make any difference to the partition
pruning logic.

			regards, tom lane

  3. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-12-01T20:31:24Z

    I wrote:
> What it looks like to me is that the code for setting up run-time
> partition pruning has failed to consider the possibility of nested
> partitioning: it's expecting that every partitioned table will have
> at least one direct child that is a leaf.  I'm not sure though
> whether just the Assert is wrong, or there's more fundamental
> issues here.

After looking into the git history I realized that this assertion is
quite new, stemming from David's a929e17e5a8 of 2020-11-02.  So there's
something not right about that.

			regards, tom lane

  4. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-29T20:28:26Z

    I wrote:
>> What it looks like to me is that the code for setting up run-time
>> partition pruning has failed to consider the possibility of nested
>> partitioning: it's expecting that every partitioned table will have
>> at least one direct child that is a leaf.  I'm not sure though
>> whether just the Assert is wrong, or there's more fundamental
>> issues here.

> After looking into the git history I realized that this assertion is
> quite new, stemming from David's a929e17e5a8 of 2020-11-02.  So there's
> something not right about that.

I took some more time to poke at this today, and I now think that
the assertion in make_partitionedrel_pruneinfo is probably OK,
and what it's pointing out is a bug upstream in path creation.
Specifically, I noted that in

select a from trigger_parted where pg_trigger_depth() <> a order by a;

we arrive at make_partitionedrel_pruneinfo with partrelids equal
to (b 1 2), which seems to be correct.  The RTE list is

RTE 1: trigger_parted
RTE 2: trigger_parted_p1
RTE 3: trigger_parted_p1_1

Like so much else of the partitioning code, AppendPath.partitioned_rels
is abysmally underdocumented, but what I think it means is the set of
non-leaf partitioned tables that are notionally scanned by the
AppendPath.  The only table directly mentioned by the AppendPath's
subpath is RTE 3, so that all seems fine.

However, upon adding a LIMIT:

select a from trigger_parted where pg_trigger_depth() <> a order by a limit 40;
server closed the connection unexpectedly

we arrive at make_partitionedrel_pruneinfo with partrelids equal
to just (b 1); trigger_parted_p1 has been left out.  The Path
in this case has been made by generate_orderedappend_paths, which
is what's responsible for computing AppendPath.partitioned_rels that
eventually winds up as the argument to make_partitionedrel_pruneinfo.
So I think that that code is somehow failing to account for nested
partitioning, while the non-ordered-append code is doing it right.
But I didn't spot exactly where the discrepancy is.

			regards, tom lane

  5. Re: [sqlsmith] Failed assertion during partition pruning

    Zhihong Yu <zyu@yugabyte.com> — 2021-01-29T22:42:49Z

    Hi,
I wonder if the (failed) assertion should be converted to an if statement:

diff --git a/src/backend/partitioning/partprune.c
b/src/backend/partitioning/partprune.c
index fac921eea5..d646f08a07 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -585,7 +585,7 @@ make_partitionedrel_pruneinfo(PlannerInfo *root,
RelOptInfo *parentrel,
          * partitioned tables that we have no sub-paths or
          * sub-PartitionedRelPruneInfo for.
          */
-        Assert(!bms_is_empty(present_parts));
+        if (bms_is_empty(present_parts)) return NIL;

         /* Record the maps and other information. */
         pinfo->present_parts = present_parts;

Cheers

On Fri, Jan 29, 2021 at 12:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> >> What it looks like to me is that the code for setting up run-time
> >> partition pruning has failed to consider the possibility of nested
> >> partitioning: it's expecting that every partitioned table will have
> >> at least one direct child that is a leaf.  I'm not sure though
> >> whether just the Assert is wrong, or there's more fundamental
> >> issues here.
>
> > After looking into the git history I realized that this assertion is
> > quite new, stemming from David's a929e17e5a8 of 2020-11-02.  So there's
> > something not right about that.
>
> I took some more time to poke at this today, and I now think that
> the assertion in make_partitionedrel_pruneinfo is probably OK,
> and what it's pointing out is a bug upstream in path creation.
> Specifically, I noted that in
>
> select a from trigger_parted where pg_trigger_depth() <> a order by a;
>
> we arrive at make_partitionedrel_pruneinfo with partrelids equal
> to (b 1 2), which seems to be correct.  The RTE list is
>
> RTE 1: trigger_parted
> RTE 2: trigger_parted_p1
> RTE 3: trigger_parted_p1_1
>
> Like so much else of the partitioning code, AppendPath.partitioned_rels
> is abysmally underdocumented, but what I think it means is the set of
> non-leaf partitioned tables that are notionally scanned by the
> AppendPath.  The only table directly mentioned by the AppendPath's
> subpath is RTE 3, so that all seems fine.
>
> However, upon adding a LIMIT:
>
> select a from trigger_parted where pg_trigger_depth() <> a order by a
> limit 40;
> server closed the connection unexpectedly
>
> we arrive at make_partitionedrel_pruneinfo with partrelids equal
> to just (b 1); trigger_parted_p1 has been left out.  The Path
> in this case has been made by generate_orderedappend_paths, which
> is what's responsible for computing AppendPath.partitioned_rels that
> eventually winds up as the argument to make_partitionedrel_pruneinfo.
> So I think that that code is somehow failing to account for nested
> partitioning, while the non-ordered-append code is doing it right.
> But I didn't spot exactly where the discrepancy is.
>
>                         regards, tom lane
>
>
>

  6. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-29T22:52:37Z

    Zhihong Yu <zyu@yugabyte.com> writes:
> I wonder if the (failed) assertion should be converted to an if statement:

As I said, I'm now thinking it's not the Assert that's faulty.
If I'm right about that, it's likely that the mistaken labeling
of these paths has other consequences beyond triggering this
assertion.  (If it has none, I think we'd be better off to remove
these Path fields altogether, and re-identify the parent rels
here from the RelOptInfo data.)

			regards, tom lane

  7. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-30T22:42:00Z

    I wrote:
> As I said, I'm now thinking it's not the Assert that's faulty.
> If I'm right about that, it's likely that the mistaken labeling
> of these paths has other consequences beyond triggering this
> assertion.  (If it has none, I think we'd be better off to remove
> these Path fields altogether, and re-identify the parent rels
> here from the RelOptInfo data.)

After more time poking at this, I've concluded that my parenthetical
remark is the right solution: [Merge]AppendPath.partitioned_rels
ought to be nuked from orbit.  It requires a whole lot of squirrely,
now-known-to-be-buggy logic in allpaths.c, and practically the only
place where we use the result is in make_partition_pruneinfo().
That means we're expending a lot of work *per AppendPath*, which
is quite foolish unless there's some reason we can't get the
information at create_plan() time instead.  Which we can.

For simplicity of review I divided the patch into two parts.
0001 revises make_partition_pruneinfo() and children to identify
the relevant parent partitions for themselves, which is not too
hard to do by chasing up the child-to-parent AppendRelInfo links.
Not formerly documented, AFAICT, is that we want to collect just
the parent partitions that are between the Append path's own rel
(possibly equal to it) and the subpaths' rels.  I'd first tried
to code this by using the top_parent_relids and part_rels[] links
in the RelOptInfos, but that turns out not to work.  We might
ascend to a top parent that's above the Append's rel (if considering
an appendpath for a sub-partition, which happens in partitionwise
join).  We could also descend to a child at or below some subpath
level, since there are also cases where subpaths correspond to
unflattened non-leaf partitions.  Either of those things result
in failures.  But once you wrap your head around handling those
restrictions, it's quite simple.

Then 0002 just nukes all the no-longer-necessary logic to compute
the partitioned_rels fields.  There are a couple of changes that
are not quite trivial.  create_[merge_]append_plan were testing
best_path->partitioned_rels != NIL to shortcut calling
make_partition_pruneinfo at all.  I just dropped that, reasoning
that it wasn't saving enough to worry about.  Also,
create_append_path was similarly testing partitioned_rels != NIL
to decide if it needed to go to the trouble of using
get_baserel_parampathinfo.  I changed that to an IS_PARTITIONED_REL()
test, which isn't *quite* the same thing but seems close enough.
(It changes no regression results, anyway.)

This fixes the cases reported by Andreas and Jaime, leaving me
more confident that there's nothing wrong with David's Assert.

I did not try to add a regression test case, mainly because it's
not quite clear to me where the original bug is.  (I'm a little
suspicious that the blame might lie with the "match_partition_order"
cases in generate_orderedappend_paths, which try to bypass
accumulate_append_subpath without updating the partitioned_rels
data.  But I'm not excited about trying to prove that.)

I wonder whether we should consider back-patching this.  Another
thing that seems unclear is whether there is any serious consequence
to omitting some intermediate partitions from the set considered
by make_partition_pruneinfo.  Presumably it could lead to missing
some potential run-time-pruning opportunities, but is there any
worse issue?  If there isn't, this is a bigger change than I want
to put in the back braches.

			regards, tom lane

  8. Re: [sqlsmith] Failed assertion during partition pruning

    David Rowley <dgrowleyml@gmail.com> — 2021-01-31T23:49:22Z

    On Sun, 31 Jan 2021 at 11:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This fixes the cases reported by Andreas and Jaime, leaving me
> more confident that there's nothing wrong with David's Assert.

I agree that there is nothing wrong with the Assert.

The commit message of a929e17e5 mentions:

> Here we tighten that up so that partitioned_rels only ever contains the RT
> index for partitioned tables which actually have subpaths in the given
> Append/MergeAppend.  We can now Assert that every PartitionedRelPruneInfo
> has a non-empty present_parts.  That should allow us to catch any weird
> corner cases that have been missed.

It seems in this case the Assert *did* find a case that I missed.
Unfortunately, I just missed the email that reported the problem,
until yesterday.

> I did not try to add a regression test case, mainly because it's
> not quite clear to me where the original bug is.  (I'm a little
> suspicious that the blame might lie with the "match_partition_order"
> cases in generate_orderedappend_paths, which try to bypass
> accumulate_append_subpath without updating the partitioned_rels
> data.  But I'm not excited about trying to prove that.)

Yeah, that suspicion is correct. More specifically when
get_singleton_append_subpath() finds a single subpath Append /
MergeAppend, the single subpath is returned.

So what's happening is that we first build the Append paths for the
sub-partitioned table which, in the example case, will have a single
subpath Append path with partitioned_rels containing just the parent
of that partition (i.e trigger_parted_p1).  When it comes to doing
generate_orderedappend_paths() for the base relation (trigger_parted),
we find match_partition_order to be true and allow
get_singleton_append_subpath() to pullup the single-subplan Append
path. Unfortunately we just completely ignore that Append path's
partitioned_rels. Back in generate_orderedappend_paths() the
startup_partitioned_rels and total_partitioned_rels variables are
used, both of which only mention the trigger_parted table. The mention
of trigger_parted_p1 is lost.

It could be fixed by modifying get_singleton_append_subpath() to
modify the partitioned_rels when it collapses these single-subpath
Append/MergeAppend path, but I'm very open to looking at just getting
rid of the partitioned_rels field. Prior to a929e17e5 that field was
very loosely set and in serval cases could contain RT indexes of
partitioned tables that didn't even have any subpaths in the given
Append/MergeAppend. a929e17e5 attempted to tighten all that up but
looks like I missed the case above.

> I wonder whether we should consider back-patching this.  Another
> thing that seems unclear is whether there is any serious consequence
> to omitting some intermediate partitions from the set considered
> by make_partition_pruneinfo.  Presumably it could lead to missing
> some potential run-time-pruning opportunities, but is there any
> worse issue?  If there isn't, this is a bigger change than I want
> to put in the back braches.

It shouldn't be backpatched. a929e17e5 only exists in master. Prior to
that AppendPath/MergeAppendPath's partitioned_rels field could only
contain additional partitioned table RT index. There are no known
cases of missing ones prior to a929e17e5, so this bug shouldn't exist
in PG13.

a929e17e5 introduced run-time partition pruning for
sub-Append/MergeAppend paths.  The commit message of that explains
that there are plenty of legitimate cases where we can't flatten these
sub-Append/MergeAppend paths down into the top-level
Append/MergeAppend.   Originally I had thought we should only bother
doing run-time pruning on the top-level Append/MergeAppend because I
thought these cases were very uncommon.  I've now changed my mind.

For a929e17e5, it was not just a matter of removing the lines in [1]
to allow run-time pruning on nested Append/MergeAppends. I also needed
to clean up the sloppy setting of partitioned_rels. The remainder of
the patch attempted that.

FWIW, I hacked together a patch which fixes the bug by passing a
Bitmapset ** pointer to get_singleton_append_subpath(), which set the
bits for the Append / MergeAppend path's partitioned_rels that we get
rid of when it only has a single subpath. This stops the Assert
failure mentioned here.  However, I'd much rather explore getting rid
of partitioned_rels completely. I'll now have a look at the patch
you're proposing for that.

Thanks for investigating this and writing the patch. Apologies for
this email missing my attention closer to the time to when it was
initially reported.

David

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/optimizer/plan/createplan.c;h=40abe6f9f623ed2922ccc4e1991e97e90322f47d;hp=94280a730c4d9abb2143416fca8e74e76dada042;hb=a929e17e5;hpb=dfc797730fc7a07c0e6bd636ad1a564aecab3161

  9. Re: [sqlsmith] Failed assertion during partition pruning

    Amit Langote <amitlangote09@gmail.com> — 2021-02-01T02:46:15Z

    On Mon, Feb 1, 2021 at 8:50 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Sun, 31 Jan 2021 at 11:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > This fixes the cases reported by Andreas and Jaime, leaving me
> > more confident that there's nothing wrong with David's Assert.
>
> It could be fixed by modifying get_singleton_append_subpath() to
> modify the partitioned_rels when it collapses these single-subpath
> Append/MergeAppend path, but I'm very open to looking at just getting
> rid of the partitioned_rels field. Prior to a929e17e5 that field was
> very loosely set and in serval cases could contain RT indexes of
> partitioned tables that didn't even have any subpaths in the given
> Append/MergeAppend. a929e17e5 attempted to tighten all that up but
> looks like I missed the case above.
>
> > I wonder whether we should consider back-patching this.  Another
> > thing that seems unclear is whether there is any serious consequence
> > to omitting some intermediate partitions from the set considered
> > by make_partition_pruneinfo.  Presumably it could lead to missing
> > some potential run-time-pruning opportunities, but is there any
> > worse issue?  If there isn't, this is a bigger change than I want
> > to put in the back braches.
>
> It shouldn't be backpatched. a929e17e5 only exists in master. Prior to
> that AppendPath/MergeAppendPath's partitioned_rels field could only
> contain additional partitioned table RT index. There are no known
> cases of missing ones prior to a929e17e5, so this bug shouldn't exist
> in PG13.
>
> a929e17e5 introduced run-time partition pruning for
> sub-Append/MergeAppend paths.  The commit message of that explains
> that there are plenty of legitimate cases where we can't flatten these
> sub-Append/MergeAppend paths down into the top-level
> Append/MergeAppend.   Originally I had thought we should only bother
> doing run-time pruning on the top-level Append/MergeAppend because I
> thought these cases were very uncommon.  I've now changed my mind.
>
> For a929e17e5, it was not just a matter of removing the lines in [1]
> to allow run-time pruning on nested Append/MergeAppends. I also needed
> to clean up the sloppy setting of partitioned_rels. The remainder of
> the patch attempted that.
>
> FWIW, I hacked together a patch which fixes the bug by passing a
> Bitmapset ** pointer to get_singleton_append_subpath(), which set the
> bits for the Append / MergeAppend path's partitioned_rels that we get
> rid of when it only has a single subpath. This stops the Assert
> failure mentioned here.  However, I'd much rather explore getting rid
> of partitioned_rels completely. I'll now have a look at the patch
> you're proposing for that.

I've read Tom's patch (0001) and would definitely vote for that over
having partitioned_rels in Paths anymore.

> Thanks for investigating this and writing the patch.

+1.  My apologies as well for failing to notice this thread sooner.

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

  10. Re: [sqlsmith] Failed assertion during partition pruning

    David Rowley <dgrowleyml@gmail.com> — 2021-02-01T05:36:02Z

    On Sun, 31 Jan 2021 at 11:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> For simplicity of review I divided the patch into two parts.
> 0001 revises make_partition_pruneinfo() and children to identify
> the relevant parent partitions for themselves, which is not too
> hard to do by chasing up the child-to-parent AppendRelInfo links.
> Not formerly documented, AFAICT, is that we want to collect just
> the parent partitions that are between the Append path's own rel
> (possibly equal to it) and the subpaths' rels.  I'd first tried
> to code this by using the top_parent_relids and part_rels[] links
> in the RelOptInfos, but that turns out not to work.  We might
> ascend to a top parent that's above the Append's rel (if considering
> an appendpath for a sub-partition, which happens in partitionwise
> join).  We could also descend to a child at or below some subpath
> level, since there are also cases where subpaths correspond to
> unflattened non-leaf partitions.  Either of those things result
> in failures.  But once you wrap your head around handling those
> restrictions, it's quite simple.

I had a look at this one and it all makes sense and the logic for
obtaining the lineage of parent partitioned tables seems fine.

What I can't understand is why you changed to a List-of-Lists rather
than a List-of-Relids. This makes the patch both bigger than it needs
to be and the processing quite a bit less efficient.

For example, in make_partition_pruneinfo() when you're walking up to
the top-level target partitioned table you must lcons() each new list
member to ensure the children always come after the parents. These
chains are likely to be short so the horrible overheads of the
memmove() in lcons() won't cost that much, but there's more to it.
The other seemingly needlessly slow part is in the
list_concat_unique_ptr() call. This needs to be done for every subpath
in the Append/Merge append. It would be good to get rid of that.
Given, the list are most likely going to be small, but that's still a
quadratic function, so it seems like a good idea to try to avoid using
it if there is another way to do it.

The memory allocations could also be more efficient for Relids rather
than Lists.  Since we're working up from the child to the parent in
the lineage calculation code in make_partition_pruneinfo(), we'll
always allocate the Bitmapset to the correct size right away, rather
than possibly having to increase the size if the next RT index were
not to fit in the current number of words. Of course, with a
List-of-Lists, not every lcons() would require a new allocation, but
there's an above zero chance that it might.

I've attached a version of your 0001 patch which just maintains using
a List-of-Relids. This shrinks the diff down about 3kb.

Parent RT indexes are guaranteed to be lower than their children RT
indexes, so it's pretty simple to figure out the target RT index by
just looking at the lowest set bit.  Doing it this way also simplifies
things as add_part_rel_list() no longer must insist that the sublists
are in parent-to-child order.

David

  11. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-01T05:48:57Z

    David Rowley <dgrowleyml@gmail.com> writes:
> What I can't understand is why you changed to a List-of-Lists rather
> than a List-of-Relids.

Yeah, I spent no effort on micro-optimizing the data structure.  I figured
that since we were not including leaf partitions, there would never be
enough rels involved to worry about.  Perhaps that's wrong though.

> Parent RT indexes are guaranteed to be lower than their children RT
> indexes,

I was intentionally avoiding that assumption ;-).  Maybe it buys enough
to be worth the loss of generality, but ...

			regards, tom lane

  12. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-01T05:57:53Z

    I wrote:
> David Rowley <dgrowleyml@gmail.com> writes:
>> Parent RT indexes are guaranteed to be lower than their children RT
>> indexes,

> I was intentionally avoiding that assumption ;-).  Maybe it buys enough
> to be worth the loss of generality, but ...

Oh, it's too late at night.  I now remember that the real problem
I had with that representation was that it cannot work for joinrels.
Currently we only apply this logic to partitioned baserels, but
don't you think it might someday be called on to optimize
partitionwise joins?

			regards, tom lane

  13. Re: [sqlsmith] Failed assertion during partition pruning

    David Rowley <dgrowleyml@gmail.com> — 2021-02-01T06:20:28Z

    On Mon, 1 Feb 2021 at 18:57, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > David Rowley <dgrowleyml@gmail.com> writes:
> >> Parent RT indexes are guaranteed to be lower than their children RT
> >> indexes,
>
> > I was intentionally avoiding that assumption ;-).  Maybe it buys enough
> > to be worth the loss of generality, but ...
>
> Oh, it's too late at night.  I now remember that the real problem
> I had with that representation was that it cannot work for joinrels.
> Currently we only apply this logic to partitioned baserels, but
> don't you think it might someday be called on to optimize
> partitionwise joins?

I've not looked in detail, but I think the code would need a pretty
big overhaul before that could happen. For example, ever since we
allowed ATTACH PARTITION to work without taking an AEL we now have a
PartitionedRelPruneInfo.relid_map field that stores Oids for the
executor to look at to see if it can figure out if a partition has
been added since the plan was generated.  Not sure how that can work
with non-base rels as we have no Oid for join rels.  Perhaps I'm just
not thinking hard enough, but either way, it does seem like it would
take a pretty big hit with a hammer to make it work. My current
thinking is that being unable to represent join rels in a set of
Relids is fairly insignificant compared to what would be required to
get the feature to work correctly.

David

  14. Re: [sqlsmith] Failed assertion during partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-01T19:58:34Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Mon, 1 Feb 2021 at 18:57, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Oh, it's too late at night.  I now remember that the real problem
>> I had with that representation was that it cannot work for joinrels.
>> Currently we only apply this logic to partitioned baserels, but
>> don't you think it might someday be called on to optimize
>> partitionwise joins?

> I've not looked in detail, but I think the code would need a pretty
> big overhaul before that could happen.

Yeah, there's no doubt that a lot of other work would be needed too;
I'd just figured that maybe this code could be ready for it.  But on
looking at it again, I agree that the bitmapset approach is simpler
and cheaper than what I did.  I realized that a good bit of what
I did not like about the pre-existing logic is that it was calling
the values "Relids" rather than Bitmapsets.  To my mind, the Relids
typedef is meant to denote values that identify individual relations
(which might be joins) to the planner.  The values we're dealing with
in this code do *not* correspond to anything that is or could be a
RelOptInfo; rather they are sets of distinct relations.  It's fine
to use a Bitmapset if it's a convenient representation, but we should
call it that and not a Relids.

I renamed things that way, did some more work on the comments,
and pushed it.  Thanks for reviewing!

			regards, tom lane

  15. Re: [sqlsmith] Failed assertion during partition pruning

    David Rowley <dgrowleyml@gmail.com> — 2021-02-01T21:26:23Z

    On Tue, 2 Feb 2021 at 08:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I renamed things that way, did some more work on the comments,
> and pushed it.  Thanks for reviewing!

Thanks for working on this and coming up with the idea to nuke partitioned_rels.

David