Re: Alias of VALUES RTE in explain plan

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Nov 2, 2024 at 1:09 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> It seems sufficient to avoid alias pushdown when there's an ORDER BY
>> inside the VALUES subquery. We disallow a locking clause, and
>> while there can be LIMIT/OFFSET, those aren't allowed to reference the
>> VALUES output anyway. I added some test cases to show that this is
>> enough to make view-dumping behave sanely.

> ... The correctness of that rejiggering depends crucially on what
> will happen at plan time and then at EXPLAIN/ruleutils time, but the
> rules for what will happen at those later times are pretty darn
> complicated, so I feel like this is creating an awful lot of action at
> a distance.

I'm not seeing where there's a correctness issue here? The parser
is charged with assigning aliases to RTEs that lack one, and with
this patch that's still true. It's just assigning a different alias
than it did before. There is no question of whether the alias can
"leak" out of the implicit sub-select; it cannot, by SQL's scoping
rules. We have to avoid changing anything if there are clauses
inside the implicit sub-select that could reference the old choice
of alias, but the patch does that. (Or we could decide to simplify
things at the cost of breaking such SQL code, since there probably
is none in the field. It's still not clear to me which choice is
better.)

Yes, changing the parser to get an effect in ruleutils.c is indeed
action-at-a-distance-y, but the same can be said of an awful lot
of the behavior in this area. If we were to do this differently,
we'd be breaking existing conventions like "the parser is what
initially assigns aliases", and we'd be much more likely to create
new bugs that way (cf. my criticisms upthread of the v1 patch).
So I'm not really seeing another way.

We could just reject this patch series on the grounds of "it's
not a bug and we're not going to change the behavior". But I do
think the proposed new output looks nicer.

> If were able (and I suspect we're not, but hypothetically) to in
> effect pull up the subquery at parse time, so that to the planner and
> executor it doesn't even exist, then I think that would be perfectly
> fine, because then we would have strong reasons for believing that no
> later decision can turn our parse-time decision into a problem. But to
> leave that subquery there and guess that it's going to disappear
> before we get to EXPLAIN doesn't seem nearly as safe. It seems pretty
> easy to either miss things (like the ORDER BY case) or even to have
> future planner changes break stuff.

I completely fail to understand what you think might break. The
planner is not especially interested in aliases, and ruleutils already
has sufficient defenses against cases like duplicate aliases --- it
must, because such cases can arise anyway.

regards, tom lane