Horton

Thread

Commits

  1. Remove bogus Assert in make_partitionedrel_pruneinfo().

  2. LLVMJIT: Adapt to API changes in gdb and perf support.

  3. Rewrite the code that applies scan/join targets to paths.

  1. FailedAssertion on partprune

    Jaime Casanova <jaime.casanova@2ndquadrant.com> — 2018-07-24T04:55:52Z

    Hi,

I was trying sqlsmith on REL_11_STABLE (commit
1b957e59b92dc44c14708762f882d7910463a9ac) with a database i have at
hand, and got an assertion failure.
It seems to happen during planning on prunning time but only when
tables get bigger than certain size.

I configured it with "--enable-debug --enable-depend
--enable-cassert", attached is the backtrace and a script to reproduce
the problem.

--
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-07-24T07:16:24Z

    On 24 July 2018 at 16:55, Jaime Casanova <jaime.casanova@2ndquadrant.com> wrote:
> I was trying sqlsmith on REL_11_STABLE (commit
> 1b957e59b92dc44c14708762f882d7910463a9ac) with a database i have at
> hand, and got an assertion failure.
> It seems to happen during planning on prunning time but only when
> tables get bigger than certain size.
>
> I configured it with "--enable-debug --enable-depend
> --enable-cassert", attached is the backtrace and a script to reproduce
> the problem.

It looks like an AppendPath which has a non-NIL partitioned_rels List
won't always only contain leaf partitions as subpaths.  The plan that
the planner is trying to make here is:

https://explain.depesz.com/s/26zj

In this case, the 2nd to 5th Append subplan's parents are actually
partitioned tables rather than leaf partitions.
make_partition_pruneinfo() assumes these will always be leaf
partitions. Looking more closely at accumulate_append_subpath() I can
see that there are cases where it won't pull sub-Append paths into the
main Append. That's what we're seeing in this plan.

The fix is either to have make_partition_pruneinfo() return NIL to
disable run-time pruning when any of the subpaths are not a leaf
partition (it might be a good safety precaution regardless), or we
could maybe think harder about what can be done in
accumulate_append_subpath() to flatten the Append/MergeAppend paths in
this case.

I've attached a patch for the former.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  3. Re: FailedAssertion on partprune

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-24T13:46:03Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> On 24 July 2018 at 16:55, Jaime Casanova <jaime.casanova@2ndquadrant.com> wrote:
>> I was trying sqlsmith on REL_11_STABLE (commit
>> 1b957e59b92dc44c14708762f882d7910463a9ac) with a database i have at
>> hand, and got an assertion failure.

> In this case, the 2nd to 5th Append subplan's parents are actually
> partitioned tables rather than leaf partitions.
> make_partition_pruneinfo() assumes these will always be leaf
> partitions. Looking more closely at accumulate_append_subpath() I can
> see that there are cases where it won't pull sub-Append paths into the
> main Append. That's what we're seeing in this plan.

Hm, wouldn't this be fixed by your pending patch at
<CAKJS1f_eYwHk2x0xX7qW42rV_GRsJGBMe3AqN9MYLRSs1S+CiA@mail.gmail.com>
?

(I apologize for having seemingly dropped the ball on that thread;
RL has intruded on my time a bit lately.  I hope to get it dealt with
next week.)

			regards, tom lane

  4. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-07-24T14:14:34Z

    On 25 July 2018 at 01:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hm, wouldn't this be fixed by your pending patch at
> <CAKJS1f_eYwHk2x0xX7qW42rV_GRsJGBMe3AqN9MYLRSs1S+CiA@mail.gmail.com>
> ?

It's a different issue. I coded run-time pruning with the incorrect
assumption that we only get leaf partitions under an Append which have
a non-empty partitioned_rels List.  The other patch fixes it to
supported mixed hierarchies from UNION ALLs. It'll still trip up on
anything apart from leaf partitions being in the subpaths list.

Thinking again about the patch I submitted upthread; I wonder if it's
actually possible to support pruning with Jamie's query. Without
looking at the code, I don't quite see the reason that the
sub-partitioned table wouldn't be correctly pruned by the run-time
pruning code.  It could just be a matter of removing the failing
Assert(). I'll do a bit more testing and confirm.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  5. Re: FailedAssertion on partprune

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-30T23:25:54Z

    On 2018-Jul-25, David Rowley wrote:

> Thinking again about the patch I submitted upthread; I wonder if it's
> actually possible to support pruning with Jamie's query. Without
> looking at the code, I don't quite see the reason that the
> sub-partitioned table wouldn't be correctly pruned by the run-time
> pruning code.  It could just be a matter of removing the failing
> Assert(). I'll do a bit more testing and confirm.

Not looking at the code right now either, but removing that assert and
then removing the TABLESAMPLE clause, the query returns identical
results with and without pruning, so maybe you're right.  No time for
further looking now.

(SELECT 'Jaime' <> 'Jamie' COLLATE es_EC)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-08-01T14:35:09Z

    On 31 July 2018 at 11:25, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Not looking at the code right now either, but removing that assert and
> then removing the TABLESAMPLE clause, the query returns identical
> results with and without pruning, so maybe you're right.  No time for
> further looking now.

I thought about this a bit more and it seems fine just to remove the
Assert. The patch that's pending in [1] adds all the correct handling
for subplans that don't belong in any partition hierarchy. That's not
the case for Jaime's plan, but those sub-partitioned tables will be
identified just like any other partition by the run-time pruning code
and can be pruned in the same way.

The attached patch removes the Assert, but I think it should be
probably be done as part of [1]'s patch since that's also adding the
code to handle subplans for tables that don't belong in the partition
hierarchy.

I also spent a bit of time trying to create a simple test case for
this and I've discovered that it's really not very easy and I have
doubts about how stable such a plan might be.

> (SELECT 'Jaime' <> 'Jamie' COLLATE es_EC)

Apologies. It was a finger auto-pilot malfunction.

[1] https://www.postgresql.org/message-id/CAKJS1f_eYwHk2x0xX7qW42rV_GRsJGBMe3AqN9MYLRSs1S%2BCiA%40mail.gmail.com

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  7. Re: FailedAssertion on partprune

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-02T17:54:14Z

    [ bringing this discussion back to the original thread ]

David Rowley <david.rowley@2ndquadrant.com> writes:
> The attached patch removes the Assert, but I think it should be
> probably be done as part of [1]'s patch since that's also adding the
> code to handle subplans for tables that don't belong in the partition
> hierarchy.

Over in that thread, we had

David Rowley <david.rowley@2ndquadrant.com> writes:
> On 2 August 2018 at 11:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I didn't include this change because (a) it's late, (b) no test
>> case was included, and (c) I don't entirely believe it anyway.
>> How would a rel be both leaf and nonleaf?  Isn't this indicative
>> of a problem somewhere upstream in the planner?

> It's probably best discussed on the other thread, but it seems to be
> by design in accumulate_append_subpath(). Parallel Append nodes don't
> get flattened if they contain a mix of parallel aware and non-parallel
> aware subplans.

Yeah, looking at the explain posted upthread, the issue is specifically
that some of the child paths might be for Gathers over subsets of the
partitioning hierarchy.  It's not real clear to me whether such a subset
would necessarily correspond to a subtree of the hierarchy, but if it
does, you'd want to be able to apply run-time pruning at the parent
Append, in hopes of not having to execute the Gather at all.  Separately
from that, if you do execute the Gather, applying runtime pruning within
its child Append is also a good thing.

So what's unclear to me is whether removing that Assert is sufficient
to make all of that work nicely.  I'm concerned in particular that the
planner might get confused about which pruning tests to apply at each
level.  (EXPLAIN isn't a very adequate tool for testing this, since
it fails to show anything about what pruning tests are attached to an
Append.  I wonder if we need to create something that does show that.)

I think it'd be a real good idea to have a test case to exercise this.
Jaime's presented test case is too expensive to consider as a regression
test, but I bet that a skinnied-down version could be made to work
once you'd set all the parallelization parameters to liberal values,
like in select_parallel.sql.

			regards, tom lane

  8. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-02T17:56:19Z

    On Thu, Aug 2, 2018 at 1:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> (EXPLAIN isn't a very adequate tool for testing this, since
> it fails to show anything about what pruning tests are attached to an
> Append.  I wonder if we need to create something that does show that.)

I've asked for that more than once and still think it's a good idea.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-02T19:53:41Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
>> It's probably best discussed on the other thread, but it seems to be
>> by design in accumulate_append_subpath(). Parallel Append nodes don't
>> get flattened if they contain a mix of parallel aware and non-parallel
>> aware subplans.

I don't really understand the issue at hand, but let me just make a
comment about accumulate_append_subpath().  If we have a regular
Append on top of another regular Append or on top of a MergeAppend, we
can flatten the lower Merge(Append) into the upper one.  No problem.
If, however, the lower path is a Parallel Append, we can't.  Consider
this plan:

Gather
-> Append
  -> Partial Subpath #1
  -> Parallel Append
    -> Partial Subpath #2
    -> Non-partial Subpath

Making Partial Subpath #2 a child of the Append rather than the
Parallel Append would be correct; the only downside is that we'd lose
the nice worker-balancing stuff that the Parallel Append does.
However, making Non-partial Subpath a child of the Append rather than
the Parallel Append would be dead wrong, because the Parallel Append
will make sure that it only gets executed by one of the cooperating
processes, and the regular Append won't.  If the upper Append were
changed to a Parallel Append, then we could flatten the whole thing
just fine, as long as we're careful to keep partial paths and
non-partial paths separated.  Also, if the situation is reversed, with
an upper Parallel Append and a regular Append beneath it, that's
always flatten-able.

> Yeah, looking at the explain posted upthread, the issue is specifically
> that some of the child paths might be for Gathers over subsets of the
> partitioning hierarchy.  It's not real clear to me whether such a subset
> would necessarily correspond to a subtree of the hierarchy, but if it
> does, you'd want to be able to apply run-time pruning at the parent
> Append, in hopes of not having to execute the Gather at all.  Separately
> from that, if you do execute the Gather, applying runtime pruning within
> its child Append is also a good thing.

That sounds right to me.  If I'm not mistaken, in that EXPLAIN, the
radicado2012 and radicado2017 partitions are ineligible for parallel
query for some reason, maybe a reloption setting, but the other
partitions are all eligible, or rather, their subpartitions are
eligible.  If they were all OK for parallel query, the plan would
probably just end up with Gather over Parallel Append over individual
sample scans.

In general, I think the only ways to get an Append are inheritance,
partitioning, and set operations.  If an upper Append is due to
partitioning or inheritance, then I think that any (Merge)Append nodes
beneath it must be a subtree of the partitioning or inheritance
hierarchy, respectively.  Partitioning and inheritance can't be mixed,
and set operations can't occur below partition expansion.  However, if
the upper Append was created by a set operation, then its child Append
nodes could be from any of those causes.  For instance, it seems
possible to imagine an upper Append that is implementing EXCEPT and a
lower Append that is implementing both partition expansion and UNION
ALL.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  10. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-08-03T00:39:59Z

    On 3 August 2018 at 05:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, looking at the explain posted upthread, the issue is specifically
> that some of the child paths might be for Gathers over subsets of the
> partitioning hierarchy.  It's not real clear to me whether such a subset
> would necessarily correspond to a subtree of the hierarchy, but if it
> does, you'd want to be able to apply run-time pruning at the parent
> Append, in hopes of not having to execute the Gather at all.  Separately
> from that, if you do execute the Gather, applying runtime pruning within
> its child Append is also a good thing.

Yeah, that "good thing" was the reason I changed my mind from aborting
run-time pruning in my first patch idea to allowing it because if
pruning says so, it's fine to remove an entire sub-partitioned
hierarchy.

If we look at the Assert that's in question.

/* Same rel cannot be both leaf and non-leaf */
Assert(relid_subplan_map[rti] == 0);

If this fails then the subplan path that was found must have a parent
that's a partitioned table. If we're worried about those containing
subplans which are not partitions then we can probably add some
Asserts somewhere else.

One place that we could do that with a bit of cover would be inside
the bms_num_members(allmatchedsubplans) < list_length(subpaths)
condition, we could do:

Assert(root->simple_rte_array[parentrel->relid]->relkind !=
   RELKIND_PARTITIONED_TABLE);

There should never be any other_subplans when the parent is a partitioned table.

I've attached a patch to demonstrate what I mean here.

> So what's unclear to me is whether removing that Assert is sufficient
> to make all of that work nicely.  I'm concerned in particular that the
> planner might get confused about which pruning tests to apply at each
> level.  (EXPLAIN isn't a very adequate tool for testing this, since
> it fails to show anything about what pruning tests are attached to an
> Append.  I wonder if we need to create something that does show that.)

Pruning is only ever applied at the top-level when the RelOptInfo is a
RELOPT_BASEREL.  It might well be that we want to reconsider that now
that we've seen some plans where the subpaths are not always pulled
into the top-level [Merge]Append path, but as of now I don't quite see
how this could get broken. We're just pruning with a coarser
granularity.

> I think it'd be a real good idea to have a test case to exercise this.
> Jaime's presented test case is too expensive to consider as a regression
> test, but I bet that a skinnied-down version could be made to work
> once you'd set all the parallelization parameters to liberal values,
> like in select_parallel.sql.

Yeah, I did spend a bit of time trying to cut down that test and
failed at it. The fact that it seemed so hard to do that didn't give
me much confidence that the plan produced by the test would be very
stable, but perhaps we can just deal with that if we see its unstable.

I'll try again with the test.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  11. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-08-03T00:42:21Z

    On 3 August 2018 at 07:53, Robert Haas <robertmhaas@gmail.com> wrote:
> I don't really understand the issue at hand, but let me just make a
> comment about accumulate_append_subpath().  If we have a regular
> Append on top of another regular Append or on top of a MergeAppend, we
> can flatten the lower Merge(Append) into the upper one.  No problem.
> If, however, the lower path is a Parallel Append, we can't.  Consider
> this plan:
>
> Gather
> -> Append
>   -> Partial Subpath #1
>   -> Parallel Append
>     -> Partial Subpath #2
>     -> Non-partial Subpath
>
> Making Partial Subpath #2 a child of the Append rather than the
> Parallel Append would be correct; the only downside is that we'd lose
> the nice worker-balancing stuff that the Parallel Append does.
> However, making Non-partial Subpath a child of the Append rather than
> the Parallel Append would be dead wrong, because the Parallel Append
> will make sure that it only gets executed by one of the cooperating
> processes, and the regular Append won't.  If the upper Append were
> changed to a Parallel Append, then we could flatten the whole thing
> just fine, as long as we're careful to keep partial paths and
> non-partial paths separated.  Also, if the situation is reversed, with
> an upper Parallel Append and a regular Append beneath it, that's
> always flatten-able.

Wouldn't that code have more flexibility to flatten the Append if it
were to, instead of looking at the Append's subpaths, look at the
subpath's Parent RelOptInfo paths and just use the Append and
MergeAppend as a container to identify the relations that must be
included, rather than the paths that should be?

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  12. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-03T13:25:50Z

    On Thu, Aug 2, 2018 at 8:42 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> Wouldn't that code have more flexibility to flatten the Append if it
> were to, instead of looking at the Append's subpaths, look at the
> subpath's Parent RelOptInfo paths and just use the Append and
> MergeAppend as a container to identify the relations that must be
> included, rather than the paths that should be?

Well, we actually start with a list of RelOptInfos, take the cheapest
path from each whether partial or non-partial, and then build a
Parallel Append out of the result, so there's no need to work backward
from the chosen path to the Parallel Append.  As to whether it's ever
better to take a path other than the cheapest to facilitate
flattening, I'm not sure.  At the moment, I can't think of a scenario
in which that would pay off, but there may be one.  I think that if
we're using Parallel Append at all, we're likely going to use it
throughout the plan tree, in which case everything will get flattened
just fine.  There might well be corner cases where that isn't so,
though, and in some of those flattening may be advantageous, but I
can't think of anything really clear and obvious.  You might have
better ideas than I do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  13. Re: FailedAssertion on partprune

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-08T21:52:28Z

    I spent some more time poking at Jaime's example.  I can reduce the
problem query to this and still get the Assert crash:

select random()
from radicado tablesample bernoulli (9.7)
where radi_texto = radi_inst_actu
limit 33;

None of the elements of this query can be removed without causing the
Assert to go away, which is weird because none of them have any apparent
connection to partition pruning.

With the Assert taken out, we find that this is the plan that's causing
the problem:

                                            QUERY PLAN                                            
--------------------------------------------------------------------------------------------------
 Limit  (cost=0.00..919.71 rows=33 width=8)
   ->  Append  (cost=0.00..12792.40 rows=459 width=8)
         ->  Sample Scan on radicado2012  (cost=0.00..2939.18 rows=93 width=8)
               Sampling: bernoulli ('9.7'::real)
               Filter: (radi_texto = radi_inst_actu)
         ->  Gather  (cost=1000.00..2485.72 rows=93 width=8)
               Workers Planned: 2
               ->  Parallel Append  (cost=0.00..1476.18 rows=39 width=0)
                     ->  Sample Scan on radicado2013_part00  (cost=0.00..1475.99 rows=47 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
                     ->  Sample Scan on radicado2013_part01  (cost=0.00..1461.88 rows=46 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
         ->  Gather  (cost=1000.00..2491.15 rows=93 width=8)
               Workers Planned: 2
               ->  Parallel Append  (cost=0.00..1481.62 rows=39 width=0)
                     ->  Sample Scan on radicado2014_part00  (cost=0.00..1481.42 rows=47 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
                     ->  Sample Scan on radicado2014_part01  (cost=0.00..1464.05 rows=46 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
         ->  Gather  (cost=1000.00..2482.47 rows=93 width=8)
               Workers Planned: 2
               ->  Parallel Append  (cost=0.00..1472.93 rows=39 width=0)
                     ->  Sample Scan on radicado2015_part00  (cost=0.00..1472.74 rows=47 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
                     ->  Sample Scan on radicado2015_part01  (cost=0.00..1454.28 rows=46 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
         ->  Gather  (cost=1000.00..2380.72 rows=86 width=8)
               Workers Planned: 2
               ->  Parallel Append  (cost=0.00..1371.90 rows=36 width=0)
                     ->  Sample Scan on radicado2016_part00  (cost=0.00..1371.72 rows=43 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
                     ->  Sample Scan on radicado2016_part01  (cost=0.00..1365.21 rows=43 width=0)
                           Sampling: bernoulli ('9.7'::real)
                           Filter: (radi_texto = radi_inst_actu)
         ->  Sample Scan on radicado2017  (cost=0.00..10.87 rows=1 width=8)
               Sampling: bernoulli ('9.7'::real)
               Filter: (radi_texto = radi_inst_actu)
(44 rows)

Now that seems to me to be a rather weird plan: why doesn't it prefer
to flatten everything into one parallel append?  Indeed, if you take
out any of the remaining query parts such as the LIMIT, that's what
it does.  I think that its willingness to do this is actually kind
of a bug, because this query is going to be a total disaster in terms
of the number of workers it will try to use --- way more than the
user would expect given max_parallel_workers_per_gather = 2.

In any case, I now understand David's concern about creating a
usable regression test case that produces a plan like this.
It seems to depend on a very corner-case-y and possibly buggy
set of costing behaviors.

So, while I'd be willing to go ahead and commit the Assert-removal, this
actually increases my concern about whether a correct set of pruning
instructions get generated in cases like this, because I now feel fairly
confident in saying that we haven't tested the logic for such cases *at
all*.  (Note that Jaime's query doesn't have any pruning steps after the
dust settles --- if make_partition_pruneinfo doesn't crash, it eventually
figures out that there are no quals usable for pruning.  So the fact that
it goes through without the Assert does nothing to assuage my fear.)

I think we'd be well advised to either change the logic to prohibit
multi-level pruning plans from being generated, or do something to
allow forcing them to be generated for testing purposes.

			regards, tom lane

  14. Re: FailedAssertion on partprune

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-09T03:33:17Z

    I wrote:
> Now that seems to me to be a rather weird plan: why doesn't it prefer
> to flatten everything into one parallel append?  Indeed, if you take
> out any of the remaining query parts such as the LIMIT, that's what
> it does.  I think that its willingness to do this is actually kind
> of a bug, because this query is going to be a total disaster in terms
> of the number of workers it will try to use --- way more than the
> user would expect given max_parallel_workers_per_gather = 2.

Oh ... never mind that last.  The parent Append will run its children
sequentially, so that the Gathers execute one at a time, and at no
point will more than 2 workers be active.

Nonetheless, it's a damfool query plan, because we'll be going through
parallel worker startup/shutdown overhead 4 times for no benefit.
We really should put in some kind of logic to force those sibling
Gathers to be aggregated into one, or else disallow them altogether.

			regards, tom lane

  15. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-08-11T13:16:13Z

    On 9 August 2018 at 15:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Nonetheless, it's a damfool query plan, because we'll be going through
> parallel worker startup/shutdown overhead 4 times for no benefit.
> We really should put in some kind of logic to force those sibling
> Gathers to be aggregated into one, or else disallow them altogether.

I started debugging this to see where things go wrong. I discovered
that add_paths_to_append_rel() is called yet again from
apply_scanjoin_target_to_paths() and this is where it's all going
wrong. The problem is that the gather paths have been tagged onto the
partial paths by this time, so accumulate_append_subpath() has no code
to look through those to fetch the Append/MergeAppend subpaths, so it
just appends the entire path to the subpaths List.  This all worked
before 96030f9a481. This commit moved where generate_gather_paths() is
called.

I'm having a hard time understanding why we need to call
add_paths_to_append_rel() from apply_scanjoin_target_to_paths().  The
name of the function does not seem to imply that we'd do this. The
comment just explains "what" rather than "why" making it a bit
useless.

/* Build new paths for this relation by appending child paths. */
if (live_children != NIL)
add_paths_to_append_rel(root, rel, live_children);

I also think that accumulate_append_subpath() is a bit of a fragile
way of flattening the append rel's paths, but I feel it's probably
apply_scanjoin_target_to_paths() that's at fault here.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  16. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-13T18:52:45Z

    On Wed, Aug 8, 2018 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Oh ... never mind that last.  The parent Append will run its children
> sequentially, so that the Gathers execute one at a time, and at no
> point will more than 2 workers be active.

Yep.

> Nonetheless, it's a damfool query plan, because we'll be going through
> parallel worker startup/shutdown overhead 4 times for no benefit.
> We really should put in some kind of logic to force those sibling
> Gathers to be aggregated into one, or else disallow them altogether.

Disallowing them could be a big performance regression.  Combining
them is reasonable, but it's not clear to me how you'd fit that into
the planner structure.  There's no convenient RelOptInfo to associate
with the aggregated-Gather.  It can't use the parent RelOptInfo unless
all of the children have a partial path, and in that case this plan
never would have been generated in the first place.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  17. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-13T21:23:22Z

    On Sat, Aug 11, 2018 at 9:16 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> I started debugging this to see where things go wrong. I discovered
> that add_paths_to_append_rel() is called yet again from
> apply_scanjoin_target_to_paths() and this is where it's all going
> wrong. The problem is that the gather paths have been tagged onto the
> partial paths by this time, so accumulate_append_subpath() has no code
> to look through those to fetch the Append/MergeAppend subpaths, so it
> just appends the entire path to the subpaths List.  This all worked
> before 96030f9a481. This commit moved where generate_gather_paths() is
> called.

I'm baffled as to why looking through Gather to find
Append/MergeAppend subpaths would ever be a sane thing to do.

> I'm having a hard time understanding why we need to call
> add_paths_to_append_rel() from apply_scanjoin_target_to_paths().  The
> name of the function does not seem to imply that we'd do this. The
> comment just explains "what" rather than "why" making it a bit
> useless.
>
> /* Build new paths for this relation by appending child paths. */
> if (live_children != NIL)
> add_paths_to_append_rel(root, rel, live_children);
>
> I also think that accumulate_append_subpath() is a bit of a fragile
> way of flattening the append rel's paths, but I feel it's probably
> apply_scanjoin_target_to_paths() that's at fault here.

In my original design, apply_scanjoin_target_to_paths() -- or whatever
I called it in the original patch versions -- built an entirely new
RelOptInfo, so that we ended up with one RelOptInfo representing the
unvarnished result of scan/join planning, and a second one
representing the result of applying the scan/join target to that
result.  However, Amit Kapila was able to demonstrate that this caused
a small but noticeable regression in planning speed, which seemed like
it might plausibly cause some issues for people running very simple
queries.

In that original design, if the topmost scan/join rel was partitioned,
the new "tlist" upper rel was also partitioned, and in the same way.
In short, this was a kind of "partitionwise tlist" feature.  For each
child of the topmost scan/join rel, we built a child "tlist" rel,
which got the same paths but with the correct tlist applied to each
one.  The path for the toplevel "tlist" upper rel could then be built
by appending the children from each child "tlist" rel, or else by the
usual method of sticking a Result node over the cheapest path for the
topmost scan/join rel.  In general, the first method is superior.
Instead of a plan like Result -> Append -> (N children each producing
the unprojected tlist), you get Append -> (N children each producing
the projected tlist), which is cheaper.

To avoid the performance overhead of creating a whole new tree of
upper rels, I rewrote the code so that it modifies the RelOptInfos in
place.  That unfortunately makes the logic a bit more confusing, and
it sounds from your remarks like I didn't comment it as clearly as
might have been possible.  But the basic idea is the same: we want the
projection to be passed down to the child nodes rather than getting a
Result node on top.  The commit that did most of this --
11cf92f6e2e13c0a6e3f98be3e629e6bd90b74d5 -- also solved a few other
problems, as noted in the commit message, so I don't think we want to
rip it out wholesale.  There might be better ways to do some of it,
though.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  18. Re: FailedAssertion on partprune

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-08-16T16:36:29Z

    On 2018-Aug-13, Robert Haas wrote:

> In my original design, apply_scanjoin_target_to_paths() -- or whatever
> I called it in the original patch versions -- built an entirely new
> RelOptInfo, so that we ended up with one RelOptInfo representing the
> unvarnished result of scan/join planning, and a second one
> representing the result of applying the scan/join target to that
> result.  However, Amit Kapila was able to demonstrate that this caused
> a small but noticeable regression in planning speed, which seemed like
> it might plausibly cause some issues for people running very simple
> queries.
> 
> In that original design, if the topmost scan/join rel was partitioned,
> the new "tlist" upper rel was also partitioned, and in the same way.
> In short, this was a kind of "partitionwise tlist" feature.  For each
> child of the topmost scan/join rel, we built a child "tlist" rel,
> which got the same paths but with the correct tlist applied to each
> one.  The path for the toplevel "tlist" upper rel could then be built
> by appending the children from each child "tlist" rel, or else by the
> usual method of sticking a Result node over the cheapest path for the
> topmost scan/join rel.  In general, the first method is superior.
> Instead of a plan like Result -> Append -> (N children each producing
> the unprojected tlist), you get Append -> (N children each producing
> the projected tlist), which is cheaper.

I have to admit I have a hard time understanding this stuff.  I don't
know what a "scan/join target" is (or scan/join relation for that
matter) ... and this "tlist relation" thing is an entirely new concept
to me.

I agree that pushing down the projection looks worthwhile.

> To avoid the performance overhead of creating a whole new tree of
> upper rels, I rewrote the code so that it modifies the RelOptInfos in
> place.  That unfortunately makes the logic a bit more confusing, and
> it sounds from your remarks like I didn't comment it as clearly as
> might have been possible.  But the basic idea is the same: we want the
> projection to be passed down to the child nodes rather than getting a
> Result node on top.  The commit that did most of this --
> 11cf92f6e2e13c0a6e3f98be3e629e6bd90b74d5 -- also solved a few other
> problems, as noted in the commit message, so I don't think we want to
> rip it out wholesale.  There might be better ways to do some of it,
> though.

So,
1. I'm not sure that partition pruning devs are on the hook for
   producing a test case for the problem originally reported,
   since we're clearly dealing with a wacko plan,
2. I wonder if this should be considered an open item related to commit
   11cf92f6e2e1 instead of partprune.

The other option is to ignore this whole affair, mark this as solved in
the open items list, and move on.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  19. Re: FailedAssertion on partprune

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-08-16T16:37:57Z

    On 2018-Jul-23, Jaime Casanova wrote:


> I was trying sqlsmith on REL_11_STABLE (commit
> 1b957e59b92dc44c14708762f882d7910463a9ac) with a database i have at
> hand, and got an assertion failure.
> It seems to happen during planning on prunning time but only when
> tables get bigger than certain size.

Hi Jaime

Did you try your luck running sqlsmith after this fix?  It'd be
interesting to know if there are further failures.

Thanks for the report,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  20. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-16T18:52:10Z

    On Thu, Aug 16, 2018 at 12:36 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> I have to admit I have a hard time understanding this stuff.  I don't
> know what a "scan/join target" is (or scan/join relation for that
> matter) ... and this "tlist relation" thing is an entirely new concept
> to me.

Well, I invented the "tlist relation" concept, and it never actually
got committed, so it's not surprising that it's new to you.

The point is that if you do something like SELECT a.x + b.y FROM x, y
WHERE <stuff>, the planner will initially produce a plan that emits
a.x and b.y.  But that's not actually what we want to produce: we want
to compute the expression a.x + b.y.  So that expression is the
scan/join target.  After getting the initial plan that produces a.x
and b.y as separate columns, we perform surgery on that plan to make
it return the computed expression instead.

If x and y are partitioned and the join is partition-wise, it's better
to perform this transformation recursively, getting each child-join to
return a.x + b.y; the alternative is for each child-join to return a.x
and b.y as a separate columns and then perform the projection at the
top level.

> So,
> 1. I'm not sure that partition pruning devs are on the hook for
>    producing a test case for the problem originally reported,
>    since we're clearly dealing with a wacko plan,
> 2. I wonder if this should be considered an open item related to commit
>    11cf92f6e2e1 instead of partprune.
>
> The other option is to ignore this whole affair, mark this as solved in
> the open items list, and move on.

I don't know whether there's actually a defect here any more.  I was
trying to dispel some perceived confusion on the part of David and Tom
about what this code was trying to accomplish, but the fact that the
code caused some confusion does not necessarily mean that there's
anything wrong with it.  On the other hand, perhaps it does have
functional deficiencies, or perhaps the comments need work.  Or maybe
this code is fine taken in isolation but there are still problems in
how it interacts with partition pruning.  Let's start by deciding what
we think the problems are, and then we can decide who should fix them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  21. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-08-17T06:49:24Z

    On 17 August 2018 at 06:52, Robert Haas <robertmhaas@gmail.com> wrote:
> I don't know whether there's actually a defect here any more.  I was
> trying to dispel some perceived confusion on the part of David and Tom
> about what this code was trying to accomplish, but the fact that the
> code caused some confusion does not necessarily mean that there's
> anything wrong with it.  On the other hand, perhaps it does have
> functional deficiencies, or perhaps the comments need work.  Or maybe
> this code is fine taken in isolation but there are still problems in
> how it interacts with partition pruning.  Let's start by deciding what
> we think the problems are, and then we can decide who should fix them.

I think Tom and I both agree that the plan mentioned in [1] and [2] is
not quite as optimal as it could be. The investigation I did in [3]
highlights where this got broken and the reason why it got broken.

Tom removed the offending Assert() in 59ef49d26d2, but mentioned
there's no test case which tries to ensure having a partitioned table
as an Append child works correctly.  I agree that it would be nice to
have a test for this, but I did try a couple of times to come up with
a test case to do this, but I was unable to come up with anything
suitable compact for the regression tests, and even at that, given how
difficult it is to get a plan into this shape, I didn't hold up much
hope of the test's plan remaining stable. Tom seems to agree with this
as he mentioned in [2].

I think the best path forward is if you (Robert) could verify the
behaviour that I mentioned in [3] is expected behaviour, if it is then
perhaps the path forward is for me to try harder to write a compact
test for this, but if you discover that the behaviour is unexpected
then I think the best course of action is to fix it to properly
flatten the
Append which will mean there's no need for a test case, and perhaps
the Assert that was removed can be put back again.

[1] https://www.postgresql.org/message-id/CAKJS1f9MQd5ntg6xXg7jJe0jB_bWvCDRmaH6yNzZGF%2BTVa5M%3DA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/8600.1533765148%40sss.pgh.pa.us
[3] https://www.postgresql.org/message-id/CAKJS1f_j9BSJ7svDF7uid6EMm%2Bfu%2BcAhonZ7hcqiYU4ssv3rtg%40mail.gmail.com

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  22. Re: FailedAssertion on partprune

    David Rowley <david.rowley@2ndquadrant.com> — 2018-08-17T06:58:23Z

    On 14 August 2018 at 09:23, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Aug 11, 2018 at 9:16 AM, David Rowley
> <david.rowley@2ndquadrant.com> wrote:
>> I started debugging this to see where things go wrong. I discovered
>> that add_paths_to_append_rel() is called yet again from
>> apply_scanjoin_target_to_paths() and this is where it's all going
>> wrong. The problem is that the gather paths have been tagged onto the
>> partial paths by this time, so accumulate_append_subpath() has no code
>> to look through those to fetch the Append/MergeAppend subpaths, so it
>> just appends the entire path to the subpaths List.  This all worked
>> before 96030f9a481. This commit moved where generate_gather_paths() is
>> called.
>
> I'm baffled as to why looking through Gather to find
> Append/MergeAppend subpaths would ever be a sane thing to do.

Can you explain why it's less sane than what the current code is
doing?  Below a Gather there will be partial paths, but we can also
get those in a Parallel Append, which the accumulate_append_subpath()
code already attempts to handle. If the Gather Path is there already
then I guess one difference would be that the caller would need to
ensure that another Gather path is placed below the Parallel Append
again.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  23. Re: FailedAssertion on partprune

    Jonathan S. Katz <jkatz@postgresql.org> — 2018-08-27T22:05:16Z

    > On Aug 17, 2018, at 2:49 AM, David Rowley <david.rowley@2ndquadrant.com> wrote:
> 
> On 17 August 2018 at 06:52, Robert Haas <robertmhaas@gmail.com> wrote:
>> I don't know whether there's actually a defect here any more.  I was
>> trying to dispel some perceived confusion on the part of David and Tom
>> about what this code was trying to accomplish, but the fact that the
>> code caused some confusion does not necessarily mean that there's
>> anything wrong with it.  On the other hand, perhaps it does have
>> functional deficiencies, or perhaps the comments need work.  Or maybe
>> this code is fine taken in isolation but there are still problems in
>> how it interacts with partition pruning.  Let's start by deciding what
>> we think the problems are, and then we can decide who should fix them.
> 
> I think Tom and I both agree that the plan mentioned in [1] and [2] is
> not quite as optimal as it could be. The investigation I did in [3]
> highlights where this got broken and the reason why it got broken.
> 
> Tom removed the offending Assert() in 59ef49d26d2, but mentioned
> there's no test case which tries to ensure having a partitioned table
> as an Append child works correctly.  I agree that it would be nice to
> have a test for this, but I did try a couple of times to come up with
> a test case to do this, but I was unable to come up with anything
> suitable compact for the regression tests, and even at that, given how
> difficult it is to get a plan into this shape, I didn't hold up much
> hope of the test's plan remaining stable. Tom seems to agree with this
> as he mentioned in [2].
> 
> I think the best path forward is if you (Robert) could verify the
> behaviour that I mentioned in [3] is expected behaviour, if it is then
> perhaps the path forward is for me to try harder to write a compact
> test for this, but if you discover that the behaviour is unexpected
> then I think the best course of action is to fix it to properly
> flatten the
> Append which will mean there's no need for a test case, and perhaps
> the Assert that was removed can be put back again.
> 
> [1] https://www.postgresql.org/message-id/CAKJS1f9MQd5ntg6xXg7jJe0jB_bWvCDRmaH6yNzZGF%2BTVa5M%3DA%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/8600.1533765148%40sss.pgh.pa.us
> [3] https://www.postgresql.org/message-id/CAKJS1f_j9BSJ7svDF7uid6EMm%2Bfu%2BcAhonZ7hcqiYU4ssv3rtg%40mail.gmail.com <https://www.postgresql.org/message-id/CAKJS1f_j9BSJ7svDF7uid6EMm+fu+cAhonZ7hcqiYU4ssv3rtg@mail.gmail.com>

On behalf of the RMT, I just want to make sure this keeps moving along.
It sounds like the next step is for Robert to verify that [3] is the expected
behavior and then David can decide what to do from there.

Thanks!

Jonathan

  24. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-29T17:38:57Z

    On Mon, Aug 27, 2018 at 6:05 PM, Jonathan S. Katz <jkatz@postgresql.org> wrote:
> On behalf of the RMT, I just want to make sure this keeps moving along.
> It sounds like the next step is for Robert to verify that [3] is the
> expected
> behavior and then David can decide what to do from there.

Yes, that's the expected behavior.  If we didn't regenerate the paths
there, we'd end up with

Result
-> Append
 -> [various paths that produce a tlist which needs to be adjusted
later by the result node]

Instead of:

Append
-> [various paths that produce an adjusted tlist]

Paths of the former kind have already been generated; we regenerate
paths here to get the latter kind as well, which end up displacing the
old ones on cost.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  25. Re: FailedAssertion on partprune

    Robert Haas <robertmhaas@gmail.com> — 2018-08-29T18:23:58Z

    On Fri, Aug 17, 2018 at 2:58 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>> I'm baffled as to why looking through Gather to find
>> Append/MergeAppend subpaths would ever be a sane thing to do.
>
> Can you explain why it's less sane than what the current code is
> doing?  Below a Gather there will be partial paths, but we can also
> get those in a Parallel Append, which the accumulate_append_subpath()
> code already attempts to handle.

Sorry, I lost track of this email.  accumulate_append_subpath's job is
to flatten stacked Append nodes into a single Append node.  But you
can't in general flatten two Append nodes that are separated by
something else in the middle, because that other node probably does
something.  To take a really simple example, consider:

Append
-> Result
  -> Append
    -> ...
-> ...

Well, the Result node computes a new target list, so the inner Append
node can't just blindly be combined with the outer Append node.  You
might be able to make it work if you adjusted the inner Append node's
children to produce the same target list that the Result node
produces, but you can't just flatten it blindly.

The same thing is true of Gather:

Append
-> Gather
  -> Append
    -> ...
-> ...

The child paths of the inner Append node are known to be parallel-safe
and are necessarily partial paths unless the inner Append is a
Parallel Append; the paths under the outer Append are definitely not
partial and might not even be parallel-safe (they can't be
parallel-unsafe, or we couldn't have a Gather anywhere in the plan,
but they *could* be parallel-restricted).  If you tried to flatten the
inner Append into the outer one, you'd no longer have anything like a
valid plan, because you can't have a partial path anywhere in the tree
without a Gather someplace higher up, which wouldn't be true here any
more, which I guess is what you meant by ...

> If the Gather Path is there already
> then I guess one difference would be that the caller would need to
> ensure that another Gather path is placed below the Parallel Append
> again.

...this, but that's not right.  The Gather would have to go ABOVE the
Parallel Append.

The broader point here, though, is that even if you could do this kind
of surgery, it's the wrong way of going about the problem.  In any
case where Append-Gather-Append could be flattened to Gather-Append,
we should have just generated Gather-Append initially, rather than
trying to rejigger things that way later.  And I think that's what the
code does.  I'm not sure exactly what's going wrong with runtime
partition pruning in this case, but ISTM that if runtime partition
pruning doesn't work in some obscure case, that's just a limitation we
should accept until somebody gets around to working on it.  The key
thing right now is not to have crashes or wrong behavior, so that we
can ship.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  26. Re: FailedAssertion on partprune

    Jaime Casanova <jaime.casanova@2ndquadrant.com> — 2018-08-31T16:48:55Z

    On Thu, 16 Aug 2018 at 11:38, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2018-Jul-23, Jaime Casanova wrote:
>
>
> > I was trying sqlsmith on REL_11_STABLE (commit
> > 1b957e59b92dc44c14708762f882d7910463a9ac) with a database i have at
> > hand, and got an assertion failure.
> > It seems to happen during planning on prunning time but only when
> > tables get bigger than certain size.
>
> Hi Jaime
>
> Did you try your luck running sqlsmith after this fix?  It'd be
> interesting to know if there are further failures.
>

intermitently... so far so good!
i'm adding things (taken from release notes) slowly every often

-- 
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  27. Re: FailedAssertion on partprune

    Jonathan S. Katz <jkatz@postgresql.org> — 2018-08-31T17:24:11Z

    On 8/29/18 1:38 PM, Robert Haas wrote:
> On Mon, Aug 27, 2018 at 6:05 PM, Jonathan S. Katz <jkatz@postgresql.org> wrote:
>> On behalf of the RMT, I just want to make sure this keeps moving along.
>> It sounds like the next step is for Robert to verify that [3] is the
>> expected
>> behavior and then David can decide what to do from there.
> 
> Yes, that's the expected behavior.  If we didn't regenerate the paths
> there, we'd end up with
> 
> Result
> -> Append
>  -> [various paths that produce a tlist which needs to be adjusted
> later by the result node]
> 
> Instead of:
> 
> Append
> -> [various paths that produce an adjusted tlist]
> 
> Paths of the former kind have already been generated; we regenerate
> paths here to get the latter kind as well, which end up displacing the
> old ones on cost.
> 

An update from the RMT: after our meeting today, we decided that we
would drop this from the list of open items for the 11 major release.
The initial issue was already fixed and we understand that developing
the test for this will take some work and we do not thing it is needed
for the v11 release.

Thanks again for working on this!

Jonathan