Re: Runtime pruning problem

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Yuzuko Hosoya <hosoya(dot)yuzuko(at)lab(dot)ntt(dot)co(dot)jp>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Runtime pruning problem
Date: 2019-04-18 17:25:52
Message-ID: 17368.1555608352@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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

Attachment Content-Type Size
allow-full-pruning-of-append-wip.patch text/x-diff 1.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2019-04-18 18:05:40 Re: block-level incremental backup
Previous Message Andy Fan 2019-04-18 17:02:39 Re: Question about the holdable cursor