Re: Alias of VALUES RTE in explain plan

On Fri, Oct 25, 2024 at 10:35 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Yasir <yasir(dot)hussain(dot)shah(at)gmail(dot)com> writes:
> > I have fixed the code to produce desired output by adding a few lines in
> > pull_up_simple_subquery().
> > Attached patch is divided in 2 files:
> > - 001-Fix-Alias-VALUES-RTE.patch contains the actual fix.
> > - 002-Fix-Alias-VALUES-RTE.patch contains the expected output changes
> > against the actual fix.
>
> I was initially skeptical about this, because we've been printing
> "*VALUES*" for a decade or more and there have been few complaints.
> So I wondered if the change would annoy more people than liked it.
> However, after looking at the output for awhile, it is nice that the
> columns of the VALUES are referenced with their user-given names
> instead of "columnN". I think that's enough of an improvement that
> it'll win people over.
>

Hopefully, yes.

> However ... I don't like this implementation, not even a little
> bit. Table/column aliases are assigned by the parser, and the
> planner has no business overriding them. Quite aside from being
>

Totally agreed.

> a violation of system structural principles, there are practical
> reasons not to do it like that:
>
> 1. We'd see different results when considering plan trees than
> unplanned query trees.
>
> 2. At the place where you put this, some planning transformations have
> already been done, and that affects the results. That means that
> future extensions or restructuring of the planner might change the
> results, which seems undesirable.
>
> I think the right way to make this happen is for the parser to
> do it, which it can do by passing down the outer query level's
> Alias to addRangeTableEntryForValues. There's a few layers of
> subroutine calls between, but we can minimize the pain by adding
> a ParseState field to carry the Alias. See attached.
>

Actually, I fixed this problem using two approaches. One at the parser
side, 2nd at the planner.
The one I submitted was the latter one. The first way (attached partially)
I fixed the problem is almost similar to your approach.
Obviously, yours better manages the parent alias.
Why I submitted the 2nd solution was because I wanted to make as few
changes in the code as I could.

> My point 2 is illustrated by the fact that my patch produces
> different results in a few cases than yours does --- look at
> groupingsets.out in particular. I think that's fine, and
> the changes that yours makes and mine doesn't look a little
> unprincipled. For example, in the tests involving the "gstest1"
> view, if somebody wants nice labels on that view's VALUES columns
> then the right place to apply those labels is within the view.
> Letting a subsequent call of the view inject labels seems pretty
> action-at-a-distance-y.
>

> regards, tom lane
>
>