I wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> But, I also can't commit either v4-0003 or v5-0003 or any variant
>> thereof until we agree on what to do about 0001, and you're the
>> holdout there.
> Yeah, I owe you a review, hope to get to it over the weekend.
I'm running out of weekend, but I found time to look at v5-0001,
so here are some comments. In general, it's in pretty good shape
and these are nitpicks.
* I think your placement of the show_result_replacement_info call
site suffers from add-at-the-end syndrome. It should certainly go
before the show_instrumentation_count call: IMO we expect stuff
added by EXPLAIN ANALYZE to appear after stuff that's there in plain
EXPLAIN. But I'd really argue that from a user's standpoint this
information is part of the fundamental plan structure and so it
deserves more prominence. I'd lean to putting it first in the
T_Result case, before the "One-Time Filter". (Thought experiment:
if we'd had this EXPLAIN field from day one, where do you think
it would have been placed?)
* Even more nitpicky:
+ if (plan->lefttree == NULL)
+ show_result_replacement_info(castNode(Result,
plan), es);
I think show_result_replacement_info should have responsibility for
deciding whether to print anything in the lefttree == NULL case.
(This will affect the Asserts in show_result_replacement_info,
but those seem a little odd anyway.)
+ case RESULT_TYPE_UPPER:
+ /* a small white lie */
+ replacement_type = "Aggregate";
+ break;
I find this unconvincing: is it really an aggregate? It doesn't
help that this case doesn't seem to be reached anywhere in the
regression tests.
In general I suspect that we'll have to refine RESULT_TYPE_UPPER
in the future. I don't think this fear needs to block committing
of what you have though.
+ /* Work out what reference name to use and added it the string. */
The grammar police will be after you.
+ * (Arguably, we should instead display the RTE name in some other way in
+ * such cases, but in typical cases the RTE name is *RESULT* and printing
+ * "Result on *RESULT*" or similar doesn't seem especially useful, so for
+ * now we don't print anything at all.)
Right offhand, I think that RTE_RESULT *always* has the name *RESULT*,
so the "typical" bit seems misleading. Personally I'd drop this
para altogether.
+ /*
+ * We're replacing either a scan or a join, according to the number of
+ * rels in the relids set.
+ */
+ if (nrels == 0)
+ ExplainPropertyText("Replaces", replacement_type, es);
+ else
+ {
+ char *s = psprintf("%s on %s", replacement_type, buf.data);
+
+ ExplainPropertyText("Replaces", s, es);
+ }
This comment seems to neither have anything to do with the logic,
or to be adding anything. But do you need nrels at all? I'd
be inclined to check for "buf.len > 0" to see if you want to
insert the "on ..." bit.
+ * make_simple_result
+ * Build a Result plan node that returns a single row (or possibly no
rows,
+ * if the one-time filtered defined by resconstantqual returns false)
I don't love the name "make_simple_result", as the cases this handles
are frequently far from simple. I don't have a clearly-better idea
offhand though. Maybe "make_one_row_result"? In any case, "one-time
filtered" needs help.
In general, I wonder if it'd be better for the callers of
make_xxx_result to pass in the result_type to use. Those
functions have such a narrow view of the available info
that I'm dubious that they can get it right. Especially
if/when we decide that RESULT_TYPE_UPPER needs subdivision.
+ * relids identifies the relation for which this Result node is generating the
+ * tuples. When subplan is not NULL, it should be empty: this node is not
+ * generating anything in that case, just acting on tuples generated by the
+ * subplan. Otherwise, it may contain a single RTI (as when this Result node
+ * is substituted for a scan); multiple RTIs (as when this Result node is
+ * substituted for a join); or no RTIs at all (as when this Result node is
+ * substituted for an upper rel).
I doubt this claim that the relid set will be empty for an upper rel.
I think it's more likely that it will include all the rels for the
query.
regards, tom lane
