Re: Runtime pruning problem

From: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, 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-22 13:12:16
Message-ID: CAKJS1f_020O9bX-c1cGcDM2L5DU44hRQ3k_0MXBBqKxAQ=X5bg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, 19 Apr 2019 at 05:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Attachment Content-Type Size
wip_improve_runtime_pruning_explain_output.patch application/octet-stream 37.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2019-04-22 13:19:44 Re: Unhappy about API changes in the no-fsm-for-small-rels patch
Previous Message Kuntal Ghosh 2019-04-22 12:51:00 Re: Regression test PANICs with master-standby setup on same machine