Amit Langote <langote_amit...@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
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.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 eflags)
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 = NO_MATCHING_SUBPLANS;
- /* Mark the first as valid so that it's initialized below */
- validsubplans = bms_make_singleton(0);
- }
-
nplans = bms_num_members(validsubplans);
}
else
@@ -174,10 +165,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
* immediately, preventing later calls to ExecFindMatchingSubPlans.
*/
if (!prunestate->do_exec_prune)
- {
- Assert(nplans > 0);
appendstate->as_valid_subplans = bms_add_range(NULL, 0, nplans - 1);
- }
}
else
{
