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
On Thu, Nov 7, 2024 at 4:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > 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. Hmm, OK, that's useful. But I guess I'm still puzzled about the theory here. A name like *VALUES* doesn't seem like it was created with the idea of referring to it from some other part of your query. I do take your point that it works and somebody's probably relying on it, but I don't think you'd pick a name that requires quoting if you were trying to make it easy to use that name in the query. You might possibly also try to generate names that are easy for users to guess, and distinct. Since none of that was done, it seems like it was just intended for display. But EXPLAIN already has its own logic to decide on what names it will display in the output, which may be different from anything that was entered by the user or invented by the parser (set_rtable_names()). So the whole thing just seems like a really strange system. I find it hard to avoid the conclusion that it's just a historical accident -- somebody did something 20 or 30 years ago to make it all work, before we had all the infrastructure that we do today, and then none of those decisions ever got revisited due either to inertia or backward compatibility concerns. Do you see it differently? > 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. Yeah, I don't really want to be the one to break somebody's query that explicitly references "*VALUES*" or whatever. At least not without a better reason than I currently have. If this were just a display artifact I think finding some way to clean it up would be pretty worthwhile, but I would need a better reason to break working SQL. -- Robert Haas EDB: http://www.enterprisedb.com
On 11/8/24 20:33, Robert Haas wrote: > On Thu, Nov 7, 2024 at 4:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 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. > > Yeah, I don't really want to be the one to break somebody's query that > explicitly references "*VALUES*" or whatever. At least not without a > better reason than I currently have. If this were just a display > artifact I think finding some way to clean it up would be pretty > worthwhile, but I would need a better reason to break working SQL. Thanks for this topic! Having run into this years ago, I was confused by eref and alias fields. I frequently use eref during debugging. Also, knowing the naming convention makes it much easier to resolve issues with only an explanation when the user can't provide any other information. I wonder if other people who work with EXPLAIN a lot already have some sort of habit here. I agree that the naming convention can float, but please let it be stable and predictable. Also, I'm not sure how other extension developers operate, but in a handful of mine, I use the fact that eref always contains a name - the relational model requires a name for each (even intermediate) table and column, doesn't it? Also, do not forget that these names can be used in pg_hint_plan hints - one more reason to make it stable. -- regards, Andrei Lepikhov
[ this thread seems to have stalled out, but we need to resolve it ]
Robert Haas <robertmhaas@gmail.com> writes:
>>> 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.
> Hmm, OK, that's useful. But I guess I'm still puzzled about the theory
> here. A name like *VALUES* doesn't seem like it was created with the
> idea of referring to it from some other part of your query. I do take
> your point that it works and somebody's probably relying on it, but I
> don't think you'd pick a name that requires quoting if you were trying
> to make it easy to use that name in the query.
As you say, some of this is lost in the mists of time; but I think the
idea was specifically that these names should *not* be easy to type,
because we don't want them to conflict with any relation alias that
the user is likely to pick. I'm fairly sure that the SQL spec
actually has verbiage to the effect of "the implementation shall
select a name that does not conflict with any other name in the query".
> You might possibly also
> try to generate names that are easy for users to guess, and distinct.
Yeah, per spec they should be distinct; but somebody didn't bother
with that early on, and now we've ended up in a situation where
ruleutils.c does it instead. I'm not sure that that's terribly evil.
In particular, in a situation where we're trying to show a plan for
a query with inlined views, EXPLAIN would probably have to have code
to unique-ify the names anyway --- there's no way we're going to make
these nonce names globally unique, so the view(s) might contain names
that conflict with each other or the outer query.
>> ... We're working with a ton of history here,
>> and I'm not really convinced that change will be change for the
>> better.
> Yeah, I don't really want to be the one to break somebody's query that
> explicitly references "*VALUES*" or whatever. At least not without a
> better reason than I currently have. If this were just a display
> artifact I think finding some way to clean it up would be pretty
> worthwhile, but I would need a better reason to break working SQL.
So it seems like we're coming to the conclusion that we don't want
to change things here?
The reason I want to push for a conclusion is that the discussion
about "*VALUES*" over at [1] is on hold pending some decision here.
The v3 patch in that thread is set up in such a way that it improves
EXPLAIN output without breaking any existing SQL, and that's what
I'd use if we refrain from changing things here. But it's a tad
inconsistent, so if we did decide it was okay to break some edge
cases, I'd reconsider.
The reason that patch can (mostly) assign some other names to
"*VALUES*" without breaking things is that we treat a VALUES
clause as an implicit sub-select:
SELECT * FROM (VALUES (1,2),...) v;
is parsed as though it were more or less
SELECT * FROM (SELECT * FROM VALUES (1,2),... AS "*VALUES*") AS v;
and the "*VALUES*" alias is not referenceable except within that
implicit sub-SELECT. The only place anybody notices it is in
EXPLAIN, which has to print the base relation alias because "v"
is usually gone due to flattening. So we can change the base
relation alias to "v" without breaking anything in the original
query (except in some very weird edge cases), and then EXPLAIN
will talk about "v" not "*VALUES*".
Perhaps a similar idea could be applied to the other cases where
we invent aliases, but it's less obvious how. For instance in
SELECT ... UNION SELECT ...
there's no obvious place where we could get names for the
two sub-SELECTs.
regards, tom lane
[1] https://www.postgresql.org/message-id/flat/CAExHW5sh28_gwQP4%3DX4i4kMsAYaoSi3tsNse3LaTihV%3DeWuTcA%40mail.gmail.com