Re: plan shape work - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Sep 14, 2025 at 7:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... 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?)

> Yes, I wondered if it should actually look more like this:
> Degenerate Scan on blah
> Degenerate Join on blah, blah, blah
> Degenerate Aggregate
> MinMaxAggregate Result
> So getting rid of Replaces: altogether.

Yeah, that would be pretty tempting if we were working in a green
field.  I think it might be too much change though.  Also, from a
developer's standpoint it's better if what EXPLAIN prints agrees
with what the node types are internally.

>> +               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.

> This is the case I know about where that can be reached.

> robert.haas=# explain select 1 as a, 2 as b having false;
>                 QUERY PLAN
> ------------------------------------------
>  Result  (cost=0.00..0.01 rows=1 width=8)
>    One-Time Filter: false
>    Replaces: Aggregate
> (3 rows)

Hmm, okay.  "Replaces: Aggregate" doesn't seem like a great
explanation, but I don't have a better idea offhand.
In any case I'm not sure this result_type value is going to
have a long shelf-life, so arguing about how to spell it
may not be a productive use of time.

I do suggest adding the above as a regression test.

>> 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.

> Counterexample:
> robert.haas=# explain verbose select * from pgbench_accounts where 0 = 1;
>                 QUERY PLAN
> ------------------------------------------
>  Result  (cost=0.00..0.00 rows=0 width=0)
>    Output: aid, bid, abalance, filler
>    One-Time Filter: false
>    Replaces: Scan on pgbench_accounts
> (4 rows)

Uh ... this example does not involve an RTE_RESULT does it?
I see a regular RTE_RELATION RTE for pgbench_accounts, which
your code duly prints.

Looking at the code, there are just three places (all in
prepjointree.c) that create RTE_RESULT RTEs.  Two of them
are building the RTE from scratch, and they both do
    rte->eref = makeAlias("*RESULT*", NIL);
However the third one (pull_up_constant_function) is changing
a pulled-up RTE_FUNCTION into RTE_RESULT, and it doesn't do
anything to the RTE's eref.  While that makes your argument
nominally true, I'd be inclined to argue that this was an oversight
and it should have changed the alias/eref fields to look like other
RTE_RESULTs.  (I've not investigated, but I wonder what your
patch prints for such cases.)

Bottom line remains that I don't think the comment para I quoted
is adding any useful info.  The first half of that comment block
was sufficient.

>> In general, I wonder if it'd be better for the callers of
>> make_xxx_result to pass in the result_type to use.

> That was my first thought, but after experimentation I think it sucks,

Hmph.  Okay, if you tried it and it's bad, I'll accept your opinion.

>> 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.

> Upper rels are created by fetch_upper_rel(). The third argument
> becomes the relids set. Most call sites pass that argument as NULL:

Okay, but "most" is not "all".  I suggest that that comment is just
too specific in the first place.  Even if it's 100% correct today,
it's likely to be falsified in future and no one will remember to
update it.  Something like this might have a longer shelf life:

+ * 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 contains the relids of the planner relation that
+ * the Result represents.

            regards, tom lane