Runtime pruning problem

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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