Horton

Thread

Commits

  1. Put back regression test case in a more robust form.

  2. Allow executor startup pruning to prune all child nodes.

  3. Further adjust EXPLAIN's choices of table alias names.

  1. Runtime pruning problem

    Yuzuko Hosoya <hosoya.yuzuko@lab.ntt.co.jp> — 2019-04-16T11:54:36Z

    Hi all,

I found a runtime pruning test case which may be a problem as follows:

----
create table t1 (id int, dt date) partition by range(dt);
create table t1_1 partition of t1 for values from ('2019-01-01') to ('2019-04-01');
create table t1_2 partition of t1 for values from ('2019-04-01') to ('2019-07-01');
create table t1_3 partition of t1 for values from ('2019-07-01') to ('2019-10-01');
create table t1_4 partition of t1 for values from ('2019-10-01') to ('2020-01-01');

In this example, current_date is 2019-04-16.

postgres=# explain select * from t1 where dt = current_date + 400;
                         QUERY PLAN                         
------------------------------------------------------------
 Append  (cost=0.00..198.42 rows=44 width=8)
   Subplans Removed: 3
   ->  Seq Scan on t1_1  (cost=0.00..49.55 rows=11 width=8)
         Filter: (dt = (CURRENT_DATE + 400))
(4 rows)

postgres=# explain analyze select * from t1 where dt = current_date + 400;
                                      QUERY PLAN                                       
---------------------------------------------------------------------------------------
 Append  (cost=0.00..198.42 rows=44 width=8) (actual time=0.000..0.001 rows=0 loops=1)
   Subplans Removed: 3
   ->  Seq Scan on t1_1  (cost=0.00..49.55 rows=11 width=8) (never executed)
         Filter: (dt = (CURRENT_DATE + 400))
 Planning Time: 0.400 ms
 Execution Time: 0.070 ms
(6 rows)
----

I realized t1_1 was not scanned actually since "never executed" 
was displayed in the plan using EXPLAIN ANALYZE.  But I think 
"One-Time Filter: false" and "Subplans Removed: ALL" or something
like that should be displayed instead.

What do you think?


Best regards,
Yuzuko Hosoya
NTT Open Source Software Center

  2. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-16T12:09:52Z

    On Tue, 16 Apr 2019 at 23:55, Yuzuko Hosoya <hosoya.yuzuko@lab.ntt.co.jp> wrote:
> postgres=# explain analyze select * from t1 where dt = current_date + 400;
>                                       QUERY PLAN
> ---------------------------------------------------------------------------------------
>  Append  (cost=0.00..198.42 rows=44 width=8) (actual time=0.000..0.001 rows=0 loops=1)
>    Subplans Removed: 3
>    ->  Seq Scan on t1_1  (cost=0.00..49.55 rows=11 width=8) (never executed)
>          Filter: (dt = (CURRENT_DATE + 400))
>  Planning Time: 0.400 ms
>  Execution Time: 0.070 ms
> (6 rows)
> ----
>
> I realized t1_1 was not scanned actually since "never executed"
> was displayed in the plan using EXPLAIN ANALYZE.  But I think
> "One-Time Filter: false" and "Subplans Removed: ALL" or something
> like that should be displayed instead.
>
> What do you think?

This is intended behaviour explained by the following comment in nodeAppend.c

/*
* The case where no subplans survive pruning must be handled
* specially.  The problem here is that code in explain.c requires
* an Append to have at least one subplan in order for it to
* properly determine the Vars in that subplan's targetlist.  We
* sidestep this issue by just initializing the first subplan and
* setting as_whichplan to NO_MATCHING_SUBPLANS to indicate that
* we don't really need to scan any subnodes.
*/

It's true that there is a small overhead in this case of having to
initialise a useless subplan, but the code never tries to pull any
tuples from it, so it should be fairly minimal.  I expected that using
a value that matches no partitions would be unusual enough not to go
contorting explain.c into working for this case.

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

  3. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-17T01:12:56Z

    Hi,

On 2019/04/16 21:09, David Rowley wrote:
> On Tue, 16 Apr 2019 at 23:55, Yuzuko Hosoya <hosoya.yuzuko@lab.ntt.co.jp> wrote:
>> postgres=# explain analyze select * from t1 where dt = current_date + 400;
>>                                       QUERY PLAN
>> ---------------------------------------------------------------------------------------
>>  Append  (cost=0.00..198.42 rows=44 width=8) (actual time=0.000..0.001 rows=0 loops=1)
>>    Subplans Removed: 3
>>    ->  Seq Scan on t1_1  (cost=0.00..49.55 rows=11 width=8) (never executed)
>>          Filter: (dt = (CURRENT_DATE + 400))
>>  Planning Time: 0.400 ms
>>  Execution Time: 0.070 ms
>> (6 rows)
>> ----
>>
>> I realized t1_1 was not scanned actually since "never executed"
>> was displayed in the plan using EXPLAIN ANALYZE.  But I think
>> "One-Time Filter: false" and "Subplans Removed: ALL" or something
>> like that should be displayed instead.
>>
>> What do you think?
> 
> This is intended behaviour explained by the following comment in nodeAppend.c
> 
> /*
> * The case where no subplans survive pruning must be handled
> * specially.  The problem here is that code in explain.c requires
> * an Append to have at least one subplan in order for it to
> * properly determine the Vars in that subplan's targetlist.  We
> * sidestep this issue by just initializing the first subplan and
> * setting as_whichplan to NO_MATCHING_SUBPLANS to indicate that
> * we don't really need to scan any subnodes.
> */
> 
> It's true that there is a small overhead in this case of having to
> initialise a useless subplan, but the code never tries to pull any
> tuples from it, so it should be fairly minimal.  I expected that using
> a value that matches no partitions would be unusual enough not to go
> contorting explain.c into working for this case.

When I saw this, I didn't think as much of the overhead of initializing a
subplan as I was surprised to see that result at all.

When you see this:

explain select * from t1 where dt = current_date + 400;
                         QUERY PLAN
────────────────────────────────────────────────────────────
 Append  (cost=0.00..198.42 rows=44 width=8)
   Subplans Removed: 3
   ->  Seq Scan on t1_1  (cost=0.00..49.55 rows=11 width=8)
         Filter: (dt = (CURRENT_DATE + 400))
(4 rows)

Doesn't this give an impression that t1_1 *matches* the WHERE condition
where it clearly doesn't?  IMO, contorting explain.c to show an empty
Append like what Hosoya-san suggests doesn't sound too bad given that the
first reaction to seeing the above result is to think it's a bug of
partition pruning.

Thanks,
Amit

  4. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-17T02:29:17Z

    On Wed, 17 Apr 2019 at 13:13, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> When you see this:
>
> explain select * from t1 where dt = current_date + 400;
>                          QUERY PLAN
> ────────────────────────────────────────────────────────────
>  Append  (cost=0.00..198.42 rows=44 width=8)
>    Subplans Removed: 3
>    ->  Seq Scan on t1_1  (cost=0.00..49.55 rows=11 width=8)
>          Filter: (dt = (CURRENT_DATE + 400))
> (4 rows)
>
> Doesn't this give an impression that t1_1 *matches* the WHERE condition
> where it clearly doesn't?  IMO, contorting explain.c to show an empty
> Append like what Hosoya-san suggests doesn't sound too bad given that the
> first reaction to seeing the above result is to think it's a bug of
> partition pruning.

Where do you think the output list for EXPLAIN VERBOSE should put the
output column list in this case? On the Append node, or just not show
them?

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

  5. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-17T02:49:04Z

    On 2019/04/17 11:29, David Rowley wrote:
> On Wed, 17 Apr 2019 at 13:13, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> When you see this:
>>
>> explain select * from t1 where dt = current_date + 400;
>>                          QUERY PLAN
>> ────────────────────────────────────────────────────────────
>>  Append  (cost=0.00..198.42 rows=44 width=8)
>>    Subplans Removed: 3
>>    ->  Seq Scan on t1_1  (cost=0.00..49.55 rows=11 width=8)
>>          Filter: (dt = (CURRENT_DATE + 400))
>> (4 rows)
>>
>> Doesn't this give an impression that t1_1 *matches* the WHERE condition
>> where it clearly doesn't?  IMO, contorting explain.c to show an empty
>> Append like what Hosoya-san suggests doesn't sound too bad given that the
>> first reaction to seeing the above result is to think it's a bug of
>> partition pruning.
> 
> Where do you think the output list for EXPLAIN VERBOSE should put the
> output column list in this case? On the Append node, or just not show
> them?

Maybe, not show them?  That may be a bit inconsistent, because the point
of VERBOSE is to the targetlist among other things, but maybe the users
wouldn't mind not seeing it on such empty Append nodes.  OTOH, they are
more likely to think seeing a subplan that's clearly prunable as a bug of
the pruning logic.

Thanks,
Amit

  6. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-17T03:54:01Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/04/17 11:29, David Rowley wrote:
>> Where do you think the output list for EXPLAIN VERBOSE should put the
>> output column list in this case? On the Append node, or just not show
>> them?

> Maybe, not show them?

Yeah, I think that seems like a reasonable idea.  If we show the tlist
for Append in this case, when we never do otherwise, that will be
confusing, and it could easily break plan-reading apps like depesz.com.

What I'm more worried about is whether this breaks any internal behavior
of explain.c, as the comment David quoted upthread seems to think.
If we need to have a tlist to reference, can we make that code look
to the pre-pruning plan tree, rather than the planstate tree?

			regards, tom lane

  7. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-17T03:58:52Z

    On Wed, 17 Apr 2019 at 15:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> > On 2019/04/17 11:29, David Rowley wrote:
> >> Where do you think the output list for EXPLAIN VERBOSE should put the
> >> output column list in this case? On the Append node, or just not show
> >> them?
>
> > Maybe, not show them?
>
> Yeah, I think that seems like a reasonable idea.  If we show the tlist
> for Append in this case, when we never do otherwise, that will be
> confusing, and it could easily break plan-reading apps like depesz.com.
>
> What I'm more worried about is whether this breaks any internal behavior
> of explain.c, as the comment David quoted upthread seems to think.
> If we need to have a tlist to reference, can we make that code look
> to the pre-pruning plan tree, rather than the planstate tree?

I think most of the complexity is in what to do in
set_deparse_planstate() given that there might be no outer plan to
choose from for Append and MergeAppend. This controls what's done in
resolve_special_varno() as this descends the plan tree down the outer
side until it gets to the node that the outer var came from.

We wouldn't need to do this if we just didn't show the targetlist in
EXPLAIN VERBOSE, but there's also MergeAppend sort keys to worry about
too.  Should we just skip on those as well?

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

  8. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-17T04:04:03Z

    On 2019/04/17 12:58, David Rowley wrote:
> On Wed, 17 Apr 2019 at 15:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>>> On 2019/04/17 11:29, David Rowley wrote:
>>>> Where do you think the output list for EXPLAIN VERBOSE should put the
>>>> output column list in this case? On the Append node, or just not show
>>>> them?
>>
>>> Maybe, not show them?
>>
>> Yeah, I think that seems like a reasonable idea.  If we show the tlist
>> for Append in this case, when we never do otherwise, that will be
>> confusing, and it could easily break plan-reading apps like depesz.com.
>>
>> What I'm more worried about is whether this breaks any internal behavior
>> of explain.c, as the comment David quoted upthread seems to think.
>> If we need to have a tlist to reference, can we make that code look
>> to the pre-pruning plan tree, rather than the planstate tree?
> 
> I think most of the complexity is in what to do in
> set_deparse_planstate() given that there might be no outer plan to
> choose from for Append and MergeAppend. This controls what's done in
> resolve_special_varno() as this descends the plan tree down the outer
> side until it gets to the node that the outer var came from.
> 
> We wouldn't need to do this if we just didn't show the targetlist in
> EXPLAIN VERBOSE, but there's also MergeAppend sort keys to worry about
> too.  Should we just skip on those as well?

I guess so, if only to be consistent with Append.

Thanks,
Amit

  9. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-17T04:10:14Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> On Wed, 17 Apr 2019 at 15:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I'm more worried about is whether this breaks any internal behavior
>> of explain.c, as the comment David quoted upthread seems to think.
>> If we need to have a tlist to reference, can we make that code look
>> to the pre-pruning plan tree, rather than the planstate tree?

> I think most of the complexity is in what to do in
> set_deparse_planstate() given that there might be no outer plan to
> choose from for Append and MergeAppend. This controls what's done in
> resolve_special_varno() as this descends the plan tree down the outer
> side until it gets to the node that the outer var came from.

> We wouldn't need to do this if we just didn't show the targetlist in
> EXPLAIN VERBOSE, but there's also MergeAppend sort keys to worry about
> too.  Should we just skip on those as well?

No, the larger issue is that *any* plan node above the Append might
be recursing down to/through the Append to find out what to print for
a Var reference.  We have to be able to support that.

			regards, tom lane

  10. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-18T05:20:32Z

    On 2019/04/17 13:10, Tom Lane wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> On Wed, 17 Apr 2019 at 15:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> What I'm more worried about is whether this breaks any internal behavior
>>> of explain.c, as the comment David quoted upthread seems to think.
>>> If we need to have a tlist to reference, can we make that code look
>>> to the pre-pruning plan tree, rather than the planstate tree?
> 
>> I think most of the complexity is in what to do in
>> set_deparse_planstate() given that there might be no outer plan to
>> choose from for Append and MergeAppend. This controls what's done in
>> resolve_special_varno() as this descends the plan tree down the outer
>> side until it gets to the node that the outer var came from.
> 
>> We wouldn't need to do this if we just didn't show the targetlist in
>> EXPLAIN VERBOSE, but there's also MergeAppend sort keys to worry about
>> too.  Should we just skip on those as well?
> 
> No, the larger issue is that *any* plan node above the Append might
> be recursing down to/through the Append to find out what to print for
> a Var reference.  We have to be able to support that.

Hmm, yes.

I see that the targetlist of Append, MergeAppend, and ModifyTable nodes is
finalized using set_dummy_tlist_references(), wherein the Vars in the
nodes' (Plan struct's) targetlist are modified to be OUTER_VAR vars.  The
comments around set_dummy_tlist_references() says it's done for explain.c
to intercept any accesses to variables in these nodes' targetlist and
return the corresponding variables in the nodes' 1st child subplan's
targetlist, which must have all the variables in the nodes' targetlist.c.
This arrangement makes it mandatory for these nodes to have at least one
subplan, so the hack in runtime pruning code.

I wonder why the original targetlist of these nodes, adjusted using just
fix_scan_list(), wouldn't have been better for EXPLAIN to use?  If I
replace the set_dummy_tlist_references() call by fix_scan_list() for
Append for starters, I see that the targetlist of any nodes on top of the
Append list the Append's output variables without a "refname" prefix.
That can be confusing if the same parent table (Append's parent relation)
is referenced multiple times.  The refname is empty, because
select_rtable_names_for_explain() thinks an Append hasn't got one.  Same
is true for MergeAppend.  ModifyTable, OTOH, has one because it has the
nominalRelation field.  Maybe it's not possible to have such a field for
Append and MergeAppend, because they don't *always* refer to a single
table (UNION ALL, partitionwise join come to mind).  Anyway, even if we do
manage to print a refname for Append/MergeAppend somehow, that wouldn't be
back-patchable to 11.

Another idea is to teach explain.c about this special case of run-time
pruning having pruned all child subplans even though appendplans contains
one element to cater for targetlist accesses.  That is, Append will be
displayed with "Subplans Removed: All" and no child subplans listed below
it, even though appendplans[] has one.  David already said he didn't do in
the first place to avoid PartitionPruneInfo details creeping into other
modules, but maybe there's no other way?

Thanks,
Amit

  11. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-18T17:25:52Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/04/17 13:10, Tom Lane wrote:
>> No, the larger issue is that *any* plan node above the Append might
>> be recursing down to/through the Append to find out what to print for
>> a Var reference.  We have to be able to support that.

> I wonder why the original targetlist of these nodes, adjusted using just
> fix_scan_list(), wouldn't have been better for EXPLAIN to use?

So what I'm thinking is that I made a bad decision in 1cc29fe7c,
which did this:

    ... In passing, simplify the EXPLAIN code by
    having it deal primarily in the PlanState tree rather than separately
    searching Plan and PlanState trees.  This is noticeably cleaner for
    subplans, and about a wash elsewhere.

It was definitely silly to have the recursion in explain.c passing down
both Plan and PlanState nodes, when the former is always easily accessible
from the latter.  So that was an OK change, but at the same time I changed
ruleutils.c to accept PlanState pointers not Plan pointers from explain.c,
and that is now looking like a bad idea.  If we were to revert that
decision, then instead of assuming that an AppendState always has at least
one live child, we'd only have to assume that an Append has at least one
live child.  Which is true.

I don't recall that there was any really strong reason for switching
ruleutils' API like that, although maybe if we look harder we'll find one.
I think it was mainly just for consistency with the way that explain.c
now looks at the world; which is not a negligible consideration, but
it's certainly something we could overrule.

> Another idea is to teach explain.c about this special case of run-time
> pruning having pruned all child subplans even though appendplans contains
> one element to cater for targetlist accesses.  That is, Append will be
> displayed with "Subplans Removed: All" and no child subplans listed below
> it, even though appendplans[] has one.  David already said he didn't do in
> the first place to avoid PartitionPruneInfo details creeping into other
> modules, but maybe there's no other way?

I tried simply removing the hack in nodeAppend.c (per quick-hack patch
below), and it gets through the core regression tests without a crash,
and with output diffs that seem fine to me.  However, that just shows that
we lack enough test coverage; we evidently have no regression cases where
an upper node needs to print Vars that are coming from a fully-pruned
Append.  Given the test case mentioned in this thread, I get

regression=# explain verbose select * from t1 where dt = current_date + 400;
                 QUERY PLAN                  
---------------------------------------------
 Append  (cost=0.00..198.42 rows=44 width=8)
   Subplans Removed: 4
(2 rows)

which seems fine, but

regression=# explain verbose select * from t1 where dt = current_date + 400 order by id;
psql: server closed the connection unexpectedly

It's dying trying to resolve Vars in the Sort node, of course.

			regards, tom lane

  12. Re: Runtime pruning problem

    Robert Haas <robertmhaas@gmail.com> — 2019-04-18T18:13:56Z

    On Tue, Apr 16, 2019 at 10:49 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Maybe, not show them?  That may be a bit inconsistent, because the point
> of VERBOSE is to the targetlist among other things, but maybe the users
> wouldn't mind not seeing it on such empty Append nodes.  OTOH, they are
> more likely to think seeing a subplan that's clearly prunable as a bug of
> the pruning logic.

Or maybe we could show them, but the Append could also be flagged in
some way that indicates that its child is only a dummy.

Everything Pruned: Yes

Or something.

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

  13. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-18T19:50:44Z

    I wrote:
> [ let's fix this by reverting ruleutils back to using Plans not PlanStates ]

BTW, while I suspect the above wouldn't be a huge patch, it doesn't
seem trivial either.  Since the issue is (a) cosmetic and (b) not new
(v11 behaves the same way), I don't think we should consider it to be
an open item for v12.  I suggest leaving this as a to-do for v13.

			regards, tom lane

  14. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-19T08:00:57Z

    On 2019/04/19 2:25, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> Another idea is to teach explain.c about this special case of run-time
>> pruning having pruned all child subplans even though appendplans contains
>> one element to cater for targetlist accesses.  That is, Append will be
>> displayed with "Subplans Removed: All" and no child subplans listed below
>> it, even though appendplans[] has one.  David already said he didn't do in
>> the first place to avoid PartitionPruneInfo details creeping into other
>> modules, but maybe there's no other way?
> 
> I tried simply removing the hack in nodeAppend.c (per quick-hack patch
> below), and it gets through the core regression tests without a crash,
> and with output diffs that seem fine to me.  However, that just shows that
> we lack enough test coverage; we evidently have no regression cases where
> an upper node needs to print Vars that are coming from a fully-pruned
> Append.  Given the test case mentioned in this thread, I get
> 
> regression=# explain verbose select * from t1 where dt = current_date + 400;
>                  QUERY PLAN                  
> ---------------------------------------------
>  Append  (cost=0.00..198.42 rows=44 width=8)
>    Subplans Removed: 4
> (2 rows)
> 
> which seems fine, but
> 
> regression=# explain verbose select * from t1 where dt = current_date + 400 order by id;
> psql: server closed the connection unexpectedly
> 
> It's dying trying to resolve Vars in the Sort node, of course.

Another approach, as I mentioned above, is to extend the hack that begins
in nodeAppend.c (and nodeMergeAppend.c) into explain.c, as in the
attached.  Then:

explain verbose select * from t1 where dt = current_date + 400 order by id;
                    QUERY PLAN
───────────────────────────────────────────────────
 Sort  (cost=199.62..199.73 rows=44 width=8)
   Output: t1_1.id, t1_1.dt
   Sort Key: t1_1.id
   ->  Append  (cost=0.00..198.42 rows=44 width=8)
         Subplans Removed: 4
(5 rows)

It's pretty confusing to see t1_1 which has been pruned away, but you
didn't seem very interested in the idea of teaching explain.c to use the
original target list of plans like Append, MergeAppend, etc. that have
child subplans.

Just a note: runtime pruning for MergeAppend is new in PG 12.

Thanks,
Amit

  15. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-19T08:03:07Z

    On 2019/04/19 3:13, Robert Haas wrote:
> On Tue, Apr 16, 2019 at 10:49 PM Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Maybe, not show them?  That may be a bit inconsistent, because the point
>> of VERBOSE is to the targetlist among other things, but maybe the users
>> wouldn't mind not seeing it on such empty Append nodes.  OTOH, they are
>> more likely to think seeing a subplan that's clearly prunable as a bug of
>> the pruning logic.
> 
> Or maybe we could show them, but the Append could also be flagged in
> some way that indicates that its child is only a dummy.
> 
> Everything Pruned: Yes
> 
> Or something.

Such an approach has been proposed too, although not with a new property text.

Thanks,
Amit

  16. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-19T08:13:43Z

    On 2019/04/19 17:00, Amit Langote wrote:
> On 2019/04/19 2:25, Tom Lane wrote:
>> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>>> Another idea is to teach explain.c about this special case of run-time
>>> pruning having pruned all child subplans even though appendplans contains
>>> one element to cater for targetlist accesses.  That is, Append will be
>>> displayed with "Subplans Removed: All" and no child subplans listed below
>>> it, even though appendplans[] has one.  David already said he didn't do in
>>> the first place to avoid PartitionPruneInfo details creeping into other
>>> modules, but maybe there's no other way?
>>
>> I tried simply removing the hack in nodeAppend.c (per quick-hack patch
>> below), and it gets through the core regression tests without a crash,
>> and with output diffs that seem fine to me.  However, that just shows that
>> we lack enough test coverage; we evidently have no regression cases where
>> an upper node needs to print Vars that are coming from a fully-pruned
>> Append.  Given the test case mentioned in this thread, I get
>>
>> regression=# explain verbose select * from t1 where dt = current_date + 400;
>>                  QUERY PLAN                  
>> ---------------------------------------------
>>  Append  (cost=0.00..198.42 rows=44 width=8)
>>    Subplans Removed: 4
>> (2 rows)
>>
>> which seems fine, but
>>
>> regression=# explain verbose select * from t1 where dt = current_date + 400 order by id;
>> psql: server closed the connection unexpectedly
>>
>> It's dying trying to resolve Vars in the Sort node, of course.
> 
> Another approach, as I mentioned above, is to extend the hack that begins
> in nodeAppend.c (and nodeMergeAppend.c) into explain.c, as in the
> attached.  Then:
> 
> explain verbose select * from t1 where dt = current_date + 400 order by id;
>                     QUERY PLAN
> ───────────────────────────────────────────────────
>  Sort  (cost=199.62..199.73 rows=44 width=8)
>    Output: t1_1.id, t1_1.dt
>    Sort Key: t1_1.id
>    ->  Append  (cost=0.00..198.42 rows=44 width=8)
>          Subplans Removed: 4
> (5 rows)
> 
> It's pretty confusing to see t1_1 which has been pruned away, but you
> didn't seem very interested in the idea of teaching explain.c to use the
> original target list of plans like Append, MergeAppend, etc. that have
> child subplans.
> 
> Just a note: runtime pruning for MergeAppend is new in PG 12.

The patch I attached with the previous email didn't update the expected
output file.  Correct one attached.

Thanks,
Amit

  17. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-21T06:25:43Z

    On Fri, 19 Apr 2019 at 20:01, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2019/04/19 2:25, Tom Lane wrote:
> > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> >> Another idea is to teach explain.c about this special case of run-time
> >> pruning having pruned all child subplans even though appendplans contains
> >> one element to cater for targetlist accesses.  That is, Append will be
> >> displayed with "Subplans Removed: All" and no child subplans listed below
> >> it, even though appendplans[] has one.  David already said he didn't do in
> >> the first place to avoid PartitionPruneInfo details creeping into other
> >> modules, but maybe there's no other way?
> >
> > I tried simply removing the hack in nodeAppend.c (per quick-hack patch
> > below), and it gets through the core regression tests without a crash,
> > and with output diffs that seem fine to me.  However, that just shows that
> > we lack enough test coverage; we evidently have no regression cases where
> > an upper node needs to print Vars that are coming from a fully-pruned
> > Append.  Given the test case mentioned in this thread, I get
> >
> > regression=# explain verbose select * from t1 where dt = current_date + 400;
> >                  QUERY PLAN
> > ---------------------------------------------
> >  Append  (cost=0.00..198.42 rows=44 width=8)
> >    Subplans Removed: 4
> > (2 rows)
> >
> > which seems fine, but
> >
> > regression=# explain verbose select * from t1 where dt = current_date + 400 order by id;
> > psql: server closed the connection unexpectedly
> >
> > It's dying trying to resolve Vars in the Sort node, of course.
>
> Another approach, as I mentioned above, is to extend the hack that begins
> in nodeAppend.c (and nodeMergeAppend.c) into explain.c, as in the
> attached.  Then:
>
> explain verbose select * from t1 where dt = current_date + 400 order by id;
>                     QUERY PLAN
> ───────────────────────────────────────────────────
>  Sort  (cost=199.62..199.73 rows=44 width=8)
>    Output: t1_1.id, t1_1.dt
>    Sort Key: t1_1.id
>    ->  Append  (cost=0.00..198.42 rows=44 width=8)
>          Subplans Removed: 4
> (5 rows)

We could do that, but I feel that's making EXPLAIN tell lies, which is
probably a path we should avoid. The lies might be fairly innocent
today, but maintaining them over time, like any lie, might become more
difficult.  We did perform init on a subnode, the subnode might be an
index scan, which we'd have obtained a lock on the index. It could be
fairly difficult to explain why that is given the lack of mention of
it in the explain output.

The fix I was working on before heading away for Easter was around
changing ruleutils.c to look at Plan nodes rather than PlanState
nodes. I'm afraid that this would still suffer from showing the alias
of the first subnode but not show it as in the explain output you show
above, but it does allow us to get rid of the code the initialises the
first subnode. I think that's a much cleaner way to do it.

I agree with Tom about the v13 part. If we were having this discussion
this time last year, then I'd have likely pushed for a v11 fix, but
since it's already shipped like this in one release then there's not
much more additional harm in two releases working this way. I'll try
and finished off the patch I was working on soon and submit to v13's
first commitfest.

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

  18. Re: Runtime pruning problem

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-22T05:37:08Z

    On 2019/04/21 15:25, David Rowley wrote:
> On Fri, 19 Apr 2019 at 20:01, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Another approach, as I mentioned above, is to extend the hack that begins
>> in nodeAppend.c (and nodeMergeAppend.c) into explain.c, as in the
>> attached.  Then:
>>
>> explain verbose select * from t1 where dt = current_date + 400 order by id;
>>                     QUERY PLAN
>> ───────────────────────────────────────────────────
>>  Sort  (cost=199.62..199.73 rows=44 width=8)
>>    Output: t1_1.id, t1_1.dt
>>    Sort Key: t1_1.id
>>    ->  Append  (cost=0.00..198.42 rows=44 width=8)
>>          Subplans Removed: 4
>> (5 rows)
> 
> We could do that, but I feel that's making EXPLAIN tell lies, which is
> probably a path we should avoid. The lies might be fairly innocent
> today, but maintaining them over time, like any lie, might become more
> difficult.  We did perform init on a subnode, the subnode might be an
> index scan, which we'd have obtained a lock on the index. It could be
> fairly difficult to explain why that is given the lack of mention of
> it in the explain output.

I had overlooked the fact that ExecInitAppend and ExecInitMergeAppend
actually perform ExecInitNode on the subplan, so on second thought, I
agree we've got to show it.  Should this have been documented?  The chance
that users may query for values that they've not defined partitions for
might well be be non-zero.

> The fix I was working on before heading away for Easter was around
> changing ruleutils.c to look at Plan nodes rather than PlanState
> nodes. I'm afraid that this would still suffer from showing the alias
> of the first subnode but not show it as in the explain output you show
> above, but it does allow us to get rid of the code the initialises the
> first subnode. I think that's a much cleaner way to do it.

I agree.

> I agree with Tom about the v13 part. If we were having this discussion
> this time last year, then I'd have likely pushed for a v11 fix, but
> since it's already shipped like this in one release then there's not
> much more additional harm in two releases working this way. I'll try
> and finished off the patch I was working on soon and submit to v13's
> first commitfest.

OK, I'll try to review it.

Thanks,
Amit

  19. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-22T13:12:16Z

    On Fri, 19 Apr 2019 at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So what I'm thinking is that I made a bad decision in 1cc29fe7c,
> which did this:
>
>     ... In passing, simplify the EXPLAIN code by
>     having it deal primarily in the PlanState tree rather than separately
>     searching Plan and PlanState trees.  This is noticeably cleaner for
>     subplans, and about a wash elsewhere.
>
> It was definitely silly to have the recursion in explain.c passing down
> both Plan and PlanState nodes, when the former is always easily accessible
> from the latter.  So that was an OK change, but at the same time I changed
> ruleutils.c to accept PlanState pointers not Plan pointers from explain.c,
> and that is now looking like a bad idea.  If we were to revert that
> decision, then instead of assuming that an AppendState always has at least
> one live child, we'd only have to assume that an Append has at least one
> live child.  Which is true.
>
> I don't recall that there was any really strong reason for switching
> ruleutils' API like that, although maybe if we look harder we'll find one.
> I think it was mainly just for consistency with the way that explain.c
> now looks at the world; which is not a negligible consideration, but
> it's certainly something we could overrule.

I started working on this today and I've attached what I have so far.

For a plan like the following, as shown by master's EXPLAIN, we get:

postgres=# explain verbose select *,(select * from t1 where
t1.a=listp.a) z from listp where a = three() order by z;
                                            QUERY PLAN
--------------------------------------------------------------------------------------------------
 Sort  (cost=1386.90..1386.95 rows=22 width=12)
   Output: listp1.a, listp1.b, ((SubPlan 1))
   Sort Key: ((SubPlan 1))
   ->  Append  (cost=0.00..1386.40 rows=22 width=12)
         Subplans Removed: 1
         ->  Seq Scan on public.listp1  (cost=0.00..693.15 rows=11 width=12)
               Output: listp1.a, listp1.b, (SubPlan 1)
               Filter: (listp1.a = three())
               SubPlan 1
                 ->  Index Only Scan using t1_pkey on public.t1
(cost=0.15..8.17 rows=1 width=4)
                       Output: t1.a
                       Index Cond: (t1.a = listp1.a)
(12 rows)

With the attached we end up with:

postgres=# explain verbose select *,(select * from t1 where
t1.a=listp.a) z from listp where a = three() order by z;
                     QUERY PLAN
-----------------------------------------------------
 Sort  (cost=1386.90..1386.95 rows=22 width=12)
   Output: listp1.a, listp1.b, ((SubPlan 1))
   Sort Key: ((SubPlan 1))
   ->  Append  (cost=0.00..1386.40 rows=22 width=12)
         Subplans Removed: 2
(5 rows)

notice the reference to SubPlan 1, but no definition of what Subplan 1
actually is. I don't think this is particularly good, but not all that
sure what to do about it.

The code turned a bit more complex than I'd have hoped. In order to
still properly resolve the parameters in find_param_referent() I had
to keep the ancestor list, but also had to add an ancestor_plan list
so that we can properly keep track of the Plan node parents too.
Remember that a Plan node may not have a corresponding PlanState node
if the state was never initialized.

For Append and MergeAppend I ended up always using the first of the
initialized subnodes if at least one is present and only resorted to
using the first planned subnode if there are no subnodes in the
AppendState/MergeAppendState. Without this, the Vars shown in the
MergeAppend sort keys lost their alias prefix if the first subplan
happened to have been pruned, but magically would gain it again if it
was some other node that was pruned. This was just a bit too weird, so
I ended up making a special case for this.

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

  20. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-23T23:42:10Z

    On Tue, 23 Apr 2019 at 01:12, David Rowley <david.rowley@2ndquadrant.com> wrote:
> I started working on this today and I've attached what I have so far.

I've added this to the July commitfest so that I don't forget about it.

https://commitfest.postgresql.org/23/2102/

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

  21. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-05-25T06:55:11Z

    On Wed, 24 Apr 2019 at 11:42, David Rowley <david.rowley@2ndquadrant.com> wrote:
> I've added this to the July commitfest so that I don't forget about it.
>
> https://commitfest.postgresql.org/23/2102/

and an updated patch, rebased after the pgindent run.

Hopefully, this will make the CF bot happy again.

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

  22. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-07-23T08:49:52Z

    On Sat, 25 May 2019 at 18:55, David Rowley <david.rowley@2ndquadrant.com> wrote:
> and an updated patch, rebased after the pgindent run.
>
> Hopefully, this will make the CF bot happy again.

and rebased again due to a conflict with some List changes that
touched ruleutils.c.

I also made another couple of passes over this adding a few comments
and fixing some spelling mistakes. I also added another regression
test to validate the EXPLAIN VERBOSE target list output of a
MergeAppend that's had all its subnodes pruned. Previously the Vars
from the pruned rel were only shown in the MergeAppend's sort clause.
After doing all that I'm now pretty happy with it.

The part I wouldn't mind another set of eyes on is the ruleutils.c
changes. The patch changes things around so that we don't just pass
around and track PlanStates, we also pass around the Plan node for
that state.  In some cases, the PlanState can be NULL if the Plan has
no PlanState. Currently, that only happens when run-time pruning
didn't initialise any PlanStates for the given subplan's Plan node.
I've coded it so that Append and MergeAppend use the first PlanState
to resolve Vars. I only resort to using the first Plan's vars when
there are no PlanStates.  If we just took the first Plan node all the
time then it might get confusing for users reading an EXPLAIN when the
first subplan was run-time pruned as we'd be resolving Vars from a
pruned subnode.  It seems much less confusing to print the Plan vars
when the Append/MergeAppend has no subplans.

If there are no objections to the changes then I'd really like to be
pushing this early next week.

The v3 patch is attached.

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

  23. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-30T22:27:19Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> The part I wouldn't mind another set of eyes on is the ruleutils.c
> changes.

Um, sorry for not getting to this sooner.

What I had in mind was to revert 1cc29fe7c's ruleutils changes
entirely, so that ruleutils deals only in Plans not PlanStates.
Perhaps we've grown some code since then that really needs the
PlanStates, but what is that, and could we do it some other way?
I'm not thrilled with passing both of these around, especially
if the PlanState sometimes isn't there, meaning that no code in
ruleutils could safely assume it's there anyway.

			regards, tom lane

  24. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-07-30T22:32:35Z

    On Wed, 31 Jul 2019 at 10:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <david.rowley@2ndquadrant.com> writes:
> > The part I wouldn't mind another set of eyes on is the ruleutils.c
> > changes.
>
> Um, sorry for not getting to this sooner.
>
> What I had in mind was to revert 1cc29fe7c's ruleutils changes
> entirely, so that ruleutils deals only in Plans not PlanStates.
> Perhaps we've grown some code since then that really needs the
> PlanStates, but what is that, and could we do it some other way?
> I'm not thrilled with passing both of these around, especially
> if the PlanState sometimes isn't there, meaning that no code in
> ruleutils could safely assume it's there anyway.

Are you not worried about the confusion that run-time pruning might
cause if we always show the Vars from the first Append/MergeAppend
plan node, even though the corresponding executor node might have been
pruned?

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

  25. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-30T22:50:32Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> On Wed, 31 Jul 2019 at 10:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I had in mind was to revert 1cc29fe7c's ruleutils changes
>> entirely, so that ruleutils deals only in Plans not PlanStates.
>> Perhaps we've grown some code since then that really needs the
>> PlanStates, but what is that, and could we do it some other way?
>> I'm not thrilled with passing both of these around, especially
>> if the PlanState sometimes isn't there, meaning that no code in
>> ruleutils could safely assume it's there anyway.

> Are you not worried about the confusion that run-time pruning might
> cause if we always show the Vars from the first Append/MergeAppend
> plan node, even though the corresponding executor node might have been
> pruned?

The upper-level Vars should ideally be labeled with the append parent
rel's name anyway, no?  I think it's likely *more* confusing if those
Vars change appearance depending on which partitions get pruned or not.

This may be arguing for a change in ruleutils' existing behavior,
not sure.  But when dealing with traditional-style inheritance,
I've always thought that Vars above the Append were referring to
the parent rel in its capacity as the parent, not in its capacity
as the first child.  With new-style partitioning drawing a clear
distinction between the parent and all its children, it's easier
to understand the difference.

			regards, tom lane

  26. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-30T22:56:47Z

    I wrote:
> This may be arguing for a change in ruleutils' existing behavior,
> not sure.  But when dealing with traditional-style inheritance,
> I've always thought that Vars above the Append were referring to
> the parent rel in its capacity as the parent, not in its capacity
> as the first child.  With new-style partitioning drawing a clear
> distinction between the parent and all its children, it's easier
> to understand the difference.

OK, so experimenting, I see that it is a change: HEAD does

regression=# explain verbose select * from part order by a;
                                   QUERY PLAN                                   
---------------------------------------------------------------------------------
 Sort  (cost=362.21..373.51 rows=4520 width=8)
   Output: part_p1.a, part_p1.b
   Sort Key: part_p1.a
   ->  Append  (cost=0.00..87.80 rows=4520 width=8)
         ->  Seq Scan on public.part_p1  (cost=0.00..32.60 rows=2260 width=8)
               Output: part_p1.a, part_p1.b
         ->  Seq Scan on public.part_p2_p1  (cost=0.00..32.60 rows=2260 width=8)
               Output: part_p2_p1.a, part_p2_p1.b
(8 rows)

The portion of this below the Append is fine, but I argue that
the Vars above the Append should say "part", not "part_p1".
In that way they'd look the same regardless of which partitions
have been pruned or not.

			regards, tom lane

  27. Re: Runtime pruning problem

    David Rowley <david.rowley@2ndquadrant.com> — 2019-07-30T23:14:29Z

    On Wed, 31 Jul 2019 at 10:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> OK, so experimenting, I see that it is a change: HEAD does
>
> regression=# explain verbose select * from part order by a;
>                                    QUERY PLAN
> ---------------------------------------------------------------------------------
>  Sort  (cost=362.21..373.51 rows=4520 width=8)
>    Output: part_p1.a, part_p1.b
>    Sort Key: part_p1.a
>    ->  Append  (cost=0.00..87.80 rows=4520 width=8)
>          ->  Seq Scan on public.part_p1  (cost=0.00..32.60 rows=2260 width=8)
>                Output: part_p1.a, part_p1.b
>          ->  Seq Scan on public.part_p2_p1  (cost=0.00..32.60 rows=2260 width=8)
>                Output: part_p2_p1.a, part_p2_p1.b
> (8 rows)
>
> The portion of this below the Append is fine, but I argue that
> the Vars above the Append should say "part", not "part_p1".
> In that way they'd look the same regardless of which partitions
> have been pruned or not.

That seems perfectly reasonable for Append / MergeAppend that are for
scanning partitioned tables. What do you propose we do for inheritance
and UNION ALLs?

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

  28. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-30T23:31:10Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> On Wed, 31 Jul 2019 at 10:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The portion of this below the Append is fine, but I argue that
>> the Vars above the Append should say "part", not "part_p1".
>> In that way they'd look the same regardless of which partitions
>> have been pruned or not.

> That seems perfectly reasonable for Append / MergeAppend that are for
> scanning partitioned tables. What do you propose we do for inheritance
> and UNION ALLs?

For inheritance, I don't believe there would be any change, precisely
because we've historically used the parent rel as reference.

For setops we've traditionally used the left input as reference.
Maybe we could do better, but I'm not very sure how, since SQL
doesn't actually provide any explicit names for the setop result.
Making up a name with no basis in the query probably isn't an
improvement, or at least not enough of one to justify a change.

			regards, tom lane

  29. Re: Runtime pruning problem

    Amit Langote <amitlangote09@gmail.com> — 2019-07-31T02:29:56Z

    On Wed, Jul 31, 2019 at 8:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
> > On Wed, 31 Jul 2019 at 10:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> The portion of this below the Append is fine, but I argue that
> >> the Vars above the Append should say "part", not "part_p1".
> >> In that way they'd look the same regardless of which partitions
> >> have been pruned or not.
>
> > That seems perfectly reasonable for Append / MergeAppend that are for
> > scanning partitioned tables. What do you propose we do for inheritance
> > and UNION ALLs?
>
> For inheritance, I don't believe there would be any change, precisely
> because we've historically used the parent rel as reference.

I may be missing something, but Vars above an Append/MergeAppend,
whether it's scanning a partitioned table or a regular inheritance
table, always refer to the first child subplan, which may or may not
be for the inheritance parent in its role as a child, not the Append
parent.

create table parent (a int);
alter table only parent add check (a = 1) no inherit;
create table child1 (a int check (a = 2)) inherits (parent);
create table child2 (a int check (a = 3)) inherits (parent);

explain (costs off, verbose) select * from parent where a > 1 order by 1;
              QUERY PLAN
───────────────────────────────────────
 Sort
   Output: child1.a
   Sort Key: child1.a
   ->  Append
         ->  Seq Scan on public.child1
               Output: child1.a
               Filter: (child1.a > 1)
         ->  Seq Scan on public.child2
               Output: child2.a
               Filter: (child2.a > 1)
(10 rows)

I think this is because we replace the original targetlist of such
nodes by a dummy one using set_dummy_tlist_references(), where all the
parent Vars are re-stamped with OUTER_VAR as varno.  When actually
printing the EXPLAIN VERBOSE output, ruleutils.c considers the first
child of Append as the OUTER referent, as set_deparse_planstate()
states:

    /*
     * We special-case Append and MergeAppend to pretend that the first child
     * plan is the OUTER referent; we have to interpret OUTER Vars in their
     * tlists according to one of the children, and the first one is the most
     * natural choice.

If I change set_append_references() to comment out the
set_dummy_tlist_references() call, I get this output:

explain (costs off, verbose) select * from parent where a > 1 order by 1;
              QUERY PLAN
───────────────────────────────────────
 Sort
   Output: a
   Sort Key: a
   ->  Append
         ->  Seq Scan on public.child1
               Output: child1.a
               Filter: (child1.a > 1)
         ->  Seq Scan on public.child2
               Output: child2.a
               Filter: (child2.a > 1)
(10 rows)

Not parent.a as I had expected.  That seems to be because parent's RTE
is considered unused in the plan.  One might say that the plan's
Append node belongs to that RTE, but then Append doesn't have any RT
index attached to it, so it escapes ExplainPreScanNode()'s walk of the
plan tree to collect the indexes of "used RTEs".  I changed
set_rtable_names() to get around that as follows:

@@ -3458,7 +3458,7 @@ set_rtable_names(deparse_namespace *dpns, List
*parent_namespaces,
         /* Just in case this takes an unreasonable amount of time ... */
         CHECK_FOR_INTERRUPTS();

-        if (rels_used && !bms_is_member(rtindex, rels_used))
+        if (rels_used && !bms_is_member(rtindex, rels_used) && !rte->inh)

and I get:

explain (costs off, verbose) select * from parent where a > 1 order by 1;
              QUERY PLAN
───────────────────────────────────────
 Sort
   Output: parent.a
   Sort Key: parent.a
   ->  Append
         ->  Seq Scan on public.child1
               Output: child1.a
               Filter: (child1.a > 1)
         ->  Seq Scan on public.child2
               Output: child2.a
               Filter: (child2.a > 1)
(10 rows)

> For setops we've traditionally used the left input as reference.
> Maybe we could do better, but I'm not very sure how, since SQL
> doesn't actually provide any explicit names for the setop result.
> Making up a name with no basis in the query probably isn't an
> improvement, or at least not enough of one to justify a change.

I too am not sure what we should about Appends of setops, but with the
above hacks, I get this:

explain (costs off, verbose) select * from child1 union all select *
from child2 order by 1;
              QUERY PLAN
───────────────────────────────────────
 Sort
   Output: "*SELECT* 1".a
   Sort Key: "*SELECT* 1".a
   ->  Append
         ->  Seq Scan on public.child1
               Output: child1.a
         ->  Seq Scan on public.child2
               Output: child2.a
(8 rows)

whereas currently it prints:

explain (costs off, verbose) select * from child1 union all select *
from child2 order by 1;
              QUERY PLAN
───────────────────────────────────────
 Sort
   Output: child1.a
   Sort Key: child1.a
   ->  Append
         ->  Seq Scan on public.child1
               Output: child1.a
         ->  Seq Scan on public.child2
               Output: child2.a
(8 rows)

Thanks,
Amit

  30. Re: Runtime pruning problem

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-09-12T14:11:53Z

    On 2019-Jul-30, Tom Lane wrote:

> I wrote:
> > This may be arguing for a change in ruleutils' existing behavior,
> > not sure.  But when dealing with traditional-style inheritance,
> > I've always thought that Vars above the Append were referring to
> > the parent rel in its capacity as the parent, not in its capacity
> > as the first child.  With new-style partitioning drawing a clear
> > distinction between the parent and all its children, it's easier
> > to understand the difference.
> 
> OK, so experimenting, I see that it is a change: [...]

> The portion of this below the Append is fine, but I argue that
> the Vars above the Append should say "part", not "part_p1".
> In that way they'd look the same regardless of which partitions
> have been pruned or not.

So is anyone working on a patch to use this approach?

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

  31. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-12T14:24:13Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> So is anyone working on a patch to use this approach?

It's on my to-do list, but I'm not sure how soon I'll get to it.

			regards, tom lane

  32. Re: Runtime pruning problem

    Michael Paquier <michael@paquier.xyz> — 2019-12-01T01:58:21Z

    On Thu, Sep 12, 2019 at 10:24:13AM -0400, Tom Lane wrote:
> It's on my to-do list, but I'm not sure how soon I'll get to it.

Seems like it is better to mark this CF entry as returned with
feedback then.
--
Michael

  33. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-01T02:43:35Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Sep 12, 2019 at 10:24:13AM -0400, Tom Lane wrote:
>> It's on my to-do list, but I'm not sure how soon I'll get to it.

> Seems like it is better to mark this CF entry as returned with
> feedback then.

Fair enough, but I did actually spend some time on the issue today.
Just to cross-link this thread to the latest, see

https://www.postgresql.org/message-id/12424.1575168015%40sss.pgh.pa.us

			regards, tom lane

  34. Re: Runtime pruning problem

    Michael Paquier <michael@paquier.xyz> — 2019-12-01T02:49:18Z

    On Sat, Nov 30, 2019 at 09:43:35PM -0500, Tom Lane wrote:
> Fair enough, but I did actually spend some time on the issue today.
> Just to cross-link this thread to the latest, see
> 
> https://www.postgresql.org/message-id/12424.1575168015%40sss.pgh.pa.us

Thanks, just saw the update.
--
Michael

  35. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-04T00:47:29Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jul-30, Tom Lane wrote:
>> The portion of this below the Append is fine, but I argue that
>> the Vars above the Append should say "part", not "part_p1".
>> In that way they'd look the same regardless of which partitions
>> have been pruned or not.

> So is anyone working on a patch to use this approach?

I spent some more time on this today, and successfully converted
ruleutils.c back to dealing only in Plan trees not PlanState trees.
The hard part of this turned out to be that in a Plan tree, it's
not so easy to identify subplans and initplans; the links that
simplify that in the existing ruleutils code get set up while
initializing the PlanState tree.  I had to do two things to make
it work:

* To cope with CTEScans and initPlans, ruleutils now needs access to the
PlannedStmt->subplans list, which can be set up along with the rtable.
I thought adding that as a separate argument wasn't very forward-looking,
so instead I changed the API of that function to pass the PlannedStmt.

* To cope with SubPlans, I changed the definition of the "ancestors"
list so that it includes SubPlans along with regular Plan nodes.
This is slightly squirrely, because SubPlan isn't a subclass of Plan,
but it seems to work well.  Notably, we don't have to search for
relevant SubPlan nodes in find_param_referent().  We'll just arrive
at them naturally while chasing up the ancestors list.

I don't think this is committable as it stands, because there are
a couple of undesirable changes in partition_prune.out.  Those test
cases are explaining queries in which the first child of a MergeAppend
gets pruned during executor start.  That results in ExplainPreScanNode
not seeing that node, so it deems the associated RTE to be unreferenced,
so select_rtable_names_for_explain doesn't assign that RTE an alias.
But then when we drill down for a referent for a Var above the
MergeAppend, we go to the first child of the MergeAppend (not the
MergeAppendState), ie exactly the RTE that was deemed unreferenced.
So we end up with no table alias to print.

That's not ruleutils.c's fault obviously: it did what it was told.
And it ties right into the question that's at the heart of this
discussion, ie what do we want to print for such Vars?  So I think
this patch is all right as a component of the full fix, but now we
have to move on to the main event.  I have some ideas about what
to do next, but they're not fully baked yet.

			regards, tom lane

  36. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-04T15:30:01Z

    I wrote:
>> This may be arguing for a change in ruleutils' existing behavior,
>> not sure.  But when dealing with traditional-style inheritance,
>> I've always thought that Vars above the Append were referring to
>> the parent rel in its capacity as the parent, not in its capacity
>> as the first child.  With new-style partitioning drawing a clear
>> distinction between the parent and all its children, it's easier
>> to understand the difference.

> OK, so experimenting, I see that it is a change: HEAD does

> regression=# explain verbose select * from part order by a;
>                                    QUERY PLAN                                   
> ---------------------------------------------------------------------------------
>  Sort  (cost=362.21..373.51 rows=4520 width=8)
>    Output: part_p1.a, part_p1.b
>    Sort Key: part_p1.a
>    ->  Append  (cost=0.00..87.80 rows=4520 width=8)
>          ->  Seq Scan on public.part_p1  (cost=0.00..32.60 rows=2260 width=8)
>                Output: part_p1.a, part_p1.b
>          ->  Seq Scan on public.part_p2_p1  (cost=0.00..32.60 rows=2260 width=8)
>                Output: part_p2_p1.a, part_p2_p1.b
> (8 rows)

> The portion of this below the Append is fine, but I argue that
> the Vars above the Append should say "part", not "part_p1".
> In that way they'd look the same regardless of which partitions
> have been pruned or not.

So I've been thinking about how to make this actually happen.
I do not think it's possible without adding more information
to Plan trees.  Which is not a show-stopper in itself --- there's
already various fields there that have no use except to support
EXPLAIN --- but it'd behoove us to minimize the amount of work
the planner spends to generate such new info.

I think it can be made to work with a design along these lines:

* Add the planner's AppendRelInfo list to the finished PlannedStmt.
We would have no need for the translated_vars list, only for the
recently-added reverse-lookup array, so we could reduce the cost
of copying plans by having setrefs.c zero out the translated_vars
fields, much as it does for unnecessary fields of RTEs.

* In Append and MergeAppend plan nodes, add a bitmapset field that
contains the relids of any inheritance parent rels formed by this
append operation.  (It has to be a set, not a single relid, because
a partitioned join would form two appendrels at the same plan node.
In general, partitioned joins break a lot of the simpler ideas
I'd had before this one...)  I think this is probably just the relids
of the path's parent RelOptInfo, so it's little or no extra cost to
calculate.

* In ExplainPreScanNode, treat relids mentioned in such fields as
referenced by the query, so that they'll be assigned aliases by
select_rtable_names_for_explain.  (Note that this will generally mean
that a partition root table gets its unmodified alias, and all child
rels will have "_N" added, rather than the current situation where the
first unpruned child gets the parent's unmodified alias.  This seems
good to me from a consistency standpoint, although it'll mean another
round of churn in the regression test results.)

* When ruleutils has to resolve a Var, and it descends through an
Append or MergeAppend that has this field nonempty, remember the
bitmapset of relevant relids as we continue recursing.  Once we've
finally located a base Var, if the passed-down set of inheritance
relids isn't empty, then use the AppendRelInfo data to try to map
the base Var's varno/varattno back up to any one of these relids.
If successful, print the name of the mapped-to table and column
instead of the base Var's name.

This design will correctly print references to the "same" Var
differently depending on where they appear in the plan tree, ie above
or below the Append that forms the appendrel.  I don't see any way we
can make that happen reliably without new plantree decoration --- in
particular, I don't think ruleutils can reverse-engineer which Appends
form which appendrels without any help.

An interesting point is what to do if we see more than one such append
node as we descend.  We should union the sets of relevant appendrel
relids, for sure, but now there is a possibility that more than one
appendrel can be matched while chasing back up the AppendRelInfo data.
I think that can only happen for an inheritance appendrel nested
inside a UNION ALL appendrel, so the question becomes whether we'd
rather report the inheritance root or whatever alias we're going to
assign for UNION appendrels.  Perhaps that choice should wait until
we've got some code to test these ideas with.

I haven't tried to code this yet, but will go do so if there aren't
objections to this sketch.

			regards, tom lane

  37. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-05T23:17:49Z

    I wrote:
> I haven't tried to code this yet, but will go do so if there aren't
> objections to this sketch.

OK, so here's a finished set of patches for this issue.

0001 is the same patch I posted on Tuesday; I kept it separate just
because it seemed like a largely separable set of changes.  (Note that
the undesirable regression test output changes are undone by 0002.)

0002 implements the map-vars-back-to-the-inheritance parent change
per my sketch.  Notice that relation aliases and Var names change
underneath Appends/MergeAppends, but Vars above one are (mostly)
printed the same as before.  On the whole I think this is a good
set of test output changes, reflecting a more predictable approach
to assigning aliases to inheritance children.  But somebody else
might see it differently I suppose.

Finally, 0003 is the remaining portion of David's patch to allow
deletion of all of an Append/MergeAppend's sub-plans during
executor startup pruning.

Thoughts?  I'd like to push this fairly soon, rather than waiting
for the next commitfest, because otherwise maintaining the
regression test diffs is likely to be painful.

			regards, tom lane

  38. Re: Runtime pruning problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-12T00:14:50Z

    I wrote:
> OK, so here's a finished set of patches for this issue.
> 0001 is the same patch I posted on Tuesday; I kept it separate just
> because it seemed like a largely separable set of changes.  (Note that
> the undesirable regression test output changes are undone by 0002.)
> 0002 implements the map-vars-back-to-the-inheritance parent change
> per my sketch.  Notice that relation aliases and Var names change
> underneath Appends/MergeAppends, but Vars above one are (mostly)
> printed the same as before.  On the whole I think this is a good
> set of test output changes, reflecting a more predictable approach
> to assigning aliases to inheritance children.  But somebody else
> might see it differently I suppose.
> Finally, 0003 is the remaining portion of David's patch to allow
> deletion of all of an Append/MergeAppend's sub-plans during
> executor startup pruning.

I pushed these, and the buildfarm immediately got a bad case of
the measles.  All the members using force_parallel_mode = regress
fail on the new regression test case added by 0003, with diffs
like this:

diff -U3 /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/partition_prune.out /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/partition_prune.out
--- /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/partition_prune.out	Thu Dec 12 00:40:04 2019
+++ /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/partition_prune.out	Thu Dec 12 00:45:44 2019
@@ -3169,10 +3169,12 @@
 --------------------------------------------
  Limit (actual rows=0 loops=1)
    Output: ma_test.a, ma_test.b
+   Worker 0: actual rows=0 loops=1
    ->  Merge Append (actual rows=0 loops=1)
          Sort Key: ma_test.b
+         Worker 0: actual rows=0 loops=1
          Subplans Removed: 3
-(5 rows)
+(7 rows)
 
 deallocate mt_q2;
 reset plan_cache_mode;

This looks to me like there's some other part of EXPLAIN that
needs to be updated for the possibility of zero child nodes, but
I didn't find out just where in a few minutes of searching.

As a stopgap to get back to green buildfarm, I removed this
specific test case, but we need to look at it closer.

			regards, tom lane