Thread: magical eref alias names

Hi,

When a query references a normal relation, the RangeTblEntry we
construct sets rte->eref->aliasname to either the user-specified alias
name, if there is one, or otherwise to the table name. But if there's
neither a user-specified alias name nor a table name, then we make up
a name. I have two related questions about this. First, why do we do
it at all? Second, why do we assign the names as we do?

Let me start with the second question. After some surveying of the
source code, here's a (possibly-incomplete) list of eref names that we
fabricate: old, new, *SELECT*, ANY_subquery, *MERGE*, *RESULT*,
*SELECT*, excluded, unnamed_subquery, unnamed_join, *GROUP*, *TLOCRN*,
*TROCRN*, *SELECT* %d where %d is an integer, *VALUES*, xmltable,
json_table. Of these, old, new, and excluded seem to make total sense
-- in certain contexts, the user can actually use those names in SQL
expressions to refer to stuff. But the rest, to my knowledge, can't be
referenced within the query, so I suppose they're just for display
purposes. But that seems like an odd choice, because these artificial
names are (1) not unique, which means that if they are referenced
anywhere such references are ambiguous; (2) not consistent in terms of
capitalization and punctuation, which is questionable for something
whose primary purpose is to inform the user; and (3) sometimes
incomprehensible -- I can make nothing of *TLOCRN* or *TROCRN*, even
after looking at the relevant source code, and I wouldn't know what
the distinction is between *SELECT* and *SELECT* %d without looking at
the source code.

And that brings me to the first question, which is why we're even
making up these names at all (with the exceptions of new, old, and
excluded, which serve a clear purpose). If we needed them to identify
things, that would make sense, but since they're neither unique nor
comprehensible, they're not really any good for that. And it seems
downright confusing at best, and a source of bugs at worst, to mix
together user-supplied names and system-generated names in such a way
that the one can't be easily distinguished from the other. We even
have regression tests verifying that if the user explicitly enters
unnamed_join or unnamed_subquery as an alias name, it doesn't break
anything due to confusion with identical alias names that might be
generated internally, which seems like good evidence for the
proposition that I'm not the first person to worry about this being
bug-prone. Granted, there are some cases where these names make their
way into EXPLAIN output. For example, this query from the regression
tests:

select * from int4_tbl o where (f1, f1) in
  (select f1, generate_series(1,50) / 10 g from int4_tbl i group by f1);

produces EXPLAIN output that includes this:

         ->  Subquery Scan on "ANY_subquery"
               Output: "ANY_subquery".f1, "ANY_subquery".g
               Filter: ("ANY_subquery".f1 = "ANY_subquery".g)

However, that seems to be a minority position. Many of the
system-generated eref names do not appear in EXPLAIN output, at least
not in our regression tests. Furthermore, EXPLAIN itself does
post-processing of the relations that appear in the output to set the
final names that are displaced (see set_rtable_names()), so if we
didn't insert names at parse time, we'd still have an opportunity to
make up something for display purposes. You could argue that the
results would be worse, but the current results aren't especially
amazing so I'm not sure I believe that. Perhaps with some elbow grease
they would even be better.

So overall I'm just confused here. Is this just a bunch of old cruft
that has never been cleaned up or standardized, or does it serve some
valuable purpose that is not obvious to me?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 06.11.24 20:06, Robert Haas wrote:
> I can make nothing of*TLOCRN* or*TROCRN*, even
> after looking at the relevant source code,

These are from the SQL standard text.  So they are more guidance to the 
implementer than anything else.  I think something had to be put there, 
because erefs are required.  I'm also interested in the discussion you 
raise about that.

(I think they are meant to be something like "table {left|right} operand 
cycle reference name".)

On Thu, Nov 7, 2024 at 2:05 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 06.11.24 20:06, Robert Haas wrote:
> > I can make nothing of*TLOCRN* or*TROCRN*, even
> > after looking at the relevant source code,
>
> These are from the SQL standard text.  So they are more guidance to the
> implementer than anything else.  I think something had to be put there,
> because erefs are required.  I'm also interested in the discussion you
> raise about that.
>
> (I think they are meant to be something like "table {left|right} operand
> cycle reference name".)

Interesting. I guess we could maybe document that a bit better, but
it's not the main thing to focus on.

I tried replacing all of those fake RTE entry names with NULL and
fixing whatever broke in the regression tests. I attach the result. It
seems like this is basically a problem that applies to subqueries
rather than any other form of RTE, and mostly subqueries that are
named *SELECT* or *SELECT* %d. The main place where the names are
user-visible in EXPLAIN output. With this patch, the name displayed by
EXPLAIN changed to unnamed_subquery, but that's what we already use
for subqueries in some cases, so I don't see a problem with it.
There's one case where *SELECT* 2 actually showed up in an error
message. I chose to add a second variant of the message rather than
displaying the name as unnamed_subquery, but either could be done:

-DETAIL:  There is a column named "q2" in table "*SELECT* 2", but it
cannot be referenced from this part of the query.
+DETAIL:  There is a column named "q2", but it cannot be referenced
from this part of the query.

It's quite possible that this patch isn't completely correct and it's
also possible that I'm missing some deeper problem with this idea that
just isn't exercised by the regression tests. But overall I find these
results fairly encouraging -- it just wasn't very hard to make the
regression tests pass.

I did a bit of historical checking at the comment that eref->aliasname
is required to be present was added in commit
3a94e789f5c9537d804210be3cb26f7fb08e3b9e, Tom Lane, 2000. I can't
immediately tell what the reasoning was. Copying Tom in case he has
thoughts.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> It's quite possible that this patch isn't completely correct and it's
> also possible that I'm missing some deeper problem with this idea that
> just isn't exercised by the regression tests. But overall I find these
> results fairly encouraging -- it just wasn't very hard to make the
> regression tests pass.

My guess is that there are more places that assume eref->aliasname
isn't NULL than you've caught here; and some of them probably are in
code we don't control.  And you didn't even update the comments for
struct Alias.

Is there some strong reason to insist on making that core-dump-risking
change, rather than simply assigning the now-one-size-fits-all alias
when creating Alias nodes?

> I did a bit of historical checking at the comment that eref->aliasname
> is required to be present was added in commit
> 3a94e789f5c9537d804210be3cb26f7fb08e3b9e, Tom Lane, 2000. I can't
> immediately tell what the reasoning was. Copying Tom in case he has
> thoughts.

Well, that was 24 years ago ... but the commit message seems to only
be about the requirement that user-written sub-SELECT-in-FROM have
an alias.  Which is a requirement we've since dropped.

When it comes to system-supplied aliases, I'd argue that the tradeoff
is about the same: forcing somebody or something that has a clue what
the relation is to define an alias, versus falling back to some generic
alias.  If we're okay with generic aliases for user-written sub-SELECT
then it's not much of a step to generic aliases for system-defined
relations.  I won't argue hard about that --- certainly things like
"*VALUE*" aren't very pretty.  But I'd really rather not switch to
"eref->aliasname can be NULL": that's introducing a coding gotcha for
zero benefit that I can detect.

            regards, tom lane

On Thu, Nov 7, 2024 at 4:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Is there some strong reason to insist on making that core-dump-risking
> change, rather than simply assigning the now-one-size-fits-all alias
> when creating Alias nodes?

What I'm unhappy about is not being able to tell the difference
between a name that was invented by or at least meaningful to the user
and one that isn't. Filling in unnamed_subquery everywhere doesn't
accomplish that because the user could in theory supply that name;
even if that were no issue, I do not want to have code like:

if (strcmp(aliasname, "unnamed_subquery") == 0 || (strncmp(aliasname,
"unnamed_subquery_", strlen("unnamed_subquery_") && something
complicated with strtol to see if the rest of the name is an integer))
   ... looks system generated ...
else
   ... looks user generated ...

I would be more sympathetic to the idea of system-generated aliases if
they were generated in a way that made it likely that they would be
meaningful to the user. In fact, if they were generated in such a way
that they would be unique, that would actually be fantastic and I
would definitely not be arguing for removing them. But I think what we
have right now is a mess.

--
Robert Haas
EDB: http://www.enterprisedb.com

BTW, one aspect of this proposal that needs to be discussed is that
it can break existing SQL.  An example discussed nearby[1] is

regression=# select * from (values (1,7), (3,4) order by "*VALUES*".column2);
 column1 | column2 
---------+---------
       3 |       4
       1 |       7
(2 rows)

We concluded in the other thread that we didn't want to risk breaking
such code.  I concede that this example isn't too compelling on its
own, but I wonder if there's other cases that are more likely to be
in common use.

(If we do decide it's okay to break this, my opinion about what to
do in the other thread would change.)

            regards, tom lane

[1] https://www.postgresql.org/message-id/251197.1730222362%40sss.pgh.pa.us

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Nov 7, 2024 at 4:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Is there some strong reason to insist on making that core-dump-risking
>> change, rather than simply assigning the now-one-size-fits-all alias
>> when creating Alias nodes?

> What I'm unhappy about is not being able to tell the difference
> between a name that was invented by or at least meaningful to the user
> and one that isn't.

You can already tell that, by looking to see whether
RTE->alias->aliasname exists.  eref is meant to be the resolved
name-to-use not the user's original input.

> I would be more sympathetic to the idea of system-generated aliases if
> they were generated in a way that made it likely that they would be
> meaningful to the user. In fact, if they were generated in such a way
> that they would be unique, that would actually be fantastic and I
> would definitely not be arguing for removing them.

The trick there is to keep them predictable, because as I mentioned in
my previous response, there may be people depending on knowing what
name will be assigned.  We're working with a ton of history here,
and I'm not really convinced that change will be change for the
better.

            regards, tom lane