Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hHAnT-0004vu-P1 for pgsql-hackers@arkaria.postgresql.org; Thu, 18 Apr 2019 17:26:03 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hHAnS-0002dC-4T for pgsql-hackers@arkaria.postgresql.org; Thu, 18 Apr 2019 17:26:02 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hHAnR-0002d5-OY for pgsql-hackers@lists.postgresql.org; Thu, 18 Apr 2019 17:26:01 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hHAnP-0003va-7A for pgsql-hackers@lists.postgresql.org; Thu, 18 Apr 2019 17:26:00 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id x3IHPqYA017369; Thu, 18 Apr 2019 13:25:53 -0400 From: Tom Lane To: Amit Langote cc: David Rowley , Yuzuko Hosoya , PostgreSQL Hackers Subject: Re: Runtime pruning problem In-reply-to: References: <001001d4f44b$2a2cca50$7e865ef0$@lab.ntt.co.jp> <2018.1555473241@sss.pgh.pa.us> <2923.1555474214@sss.pgh.pa.us> Comments: In-reply-to Amit Langote message dated "Thu, 18 Apr 2019 14:20:32 +0900" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <17202.1555608045.0@sss.pgh.pa.us> Date: Thu, 18 Apr 2019 13:25:52 -0400 Message-ID: <17368.1555608352@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <17202.1555608045.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Amit Langote 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 contain= s > one element to cater for targetlist accesses. That is, Append will be > displayed with "Subplans Removed: All" and no child subplans listed belo= w > 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=3D# explain verbose select * from t1 where dt =3D current_date = + 400; QUERY PLAN = --------------------------------------------- Append (cost=3D0.00..198.42 rows=3D44 width=3D8) Subplans Removed: 4 (2 rows) which seems fine, but regression=3D# explain verbose select * from t1 where dt =3D 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="allow-full-pruning-of-append-wip.patch"; charset="us-ascii" Content-ID: <17202.1555608045.2@sss.pgh.pa.us> Content-Description: allow-full-pruning-of-append-wip.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/node= Append.c index f3be242..3cd8940 100644 --- a/src/backend/executor/nodeAppend.c +++ b/src/backend/executor/nodeAppend.c @@ -143,22 +143,13 @@ ExecInitAppend(Append *node, EState *estate, int efl= ags) list_length(node->appendplans)); = /* - * 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. + * If no subplans survive pruning, change as_whichplan to + * NO_MATCHING_SUBPLANS, to indicate that we don't really need to + * scan any subnodes. */ if (bms_is_empty(validsubplans)) - { appendstate->as_whichplan =3D NO_MATCHING_SUBPLANS; = - /* Mark the first as valid so that it's initialized below */ - validsubplans =3D bms_make_singleton(0); - } - nplans =3D bms_num_members(validsubplans); } else @@ -174,10 +165,7 @@ ExecInitAppend(Append *node, EState *estate, int efla= gs) * immediately, preventing later calls to ExecFindMatchingSubPlans. */ if (!prunestate->do_exec_prune) - { - Assert(nplans > 0); appendstate->as_valid_subplans =3D bms_add_range(NULL, 0, nplans - 1); - } } else { ------- =_aaaaaaaaaa0--