Обсуждение: Making the subquery alias optional in the FROM clause

This was discussed previously in [1], and there seemed to be general
consensus in favour of it, but no new patch emerged.

Attached is a patch that takes the approach of not generating an alias
at all, which seems to be neater and simpler, and less code than
trying to generate a unique alias.

It still generates an eref for the subquery RTE, which has a made-up
relation name, but that is marked as not visible on the
ParseNamespaceItem, so it doesn't conflict with anything else, need
not be unique, and cannot be used for qualified references to the
subquery's columns.

The only place that exposes the eref's made-up relation name is the
existing query deparsing code in ruleutils.c, which uniquifies it and
generates SQL spec-compliant output. For example:

CREATE OR REPLACE VIEW test_view AS
  SELECT *
    FROM (SELECT a, b FROM foo),
         (SELECT c, d FROM bar)
   WHERE a = c;

\sv test_view

CREATE OR REPLACE VIEW public.test_view AS
 SELECT subquery.a,
    subquery.b,
    subquery_1.c,
    subquery_1.d
   FROM ( SELECT foo.a,
            foo.b
           FROM foo) subquery,
    ( SELECT bar.c,
            bar.d
           FROM bar) subquery_1
  WHERE subquery.a = subquery_1.c

Regards,
Dean

[1] https://www.postgresql.org/message-id/flat/1487773980.3143.15.camel%40oopsware.de

On Mon, Jun 27, 2022 at 9:49 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> This was discussed previously in [1], and there seemed to be general
> consensus in favour of it, but no new patch emerged.
>
> Attached is a patch that takes the approach of not generating an alias
> at all, which seems to be neater and simpler, and less code than
> trying to generate a unique alias.
>
> It still generates an eref for the subquery RTE, which has a made-up
> relation name, but that is marked as not visible on the
> ParseNamespaceItem, so it doesn't conflict with anything else, need
> not be unique, and cannot be used for qualified references to the
> subquery's columns.
>
> The only place that exposes the eref's made-up relation name is the
> existing query deparsing code in ruleutils.c, which uniquifies it and
> generates SQL spec-compliant output. For example:
>
> CREATE OR REPLACE VIEW test_view AS
>   SELECT *
>     FROM (SELECT a, b FROM foo),
>          (SELECT c, d FROM bar)
>    WHERE a = c;
>
> \sv test_view
>
> CREATE OR REPLACE VIEW public.test_view AS
>  SELECT subquery.a,
>     subquery.b,
>     subquery_1.c,
>     subquery_1.d
>    FROM ( SELECT foo.a,
>             foo.b
>            FROM foo) subquery,
>     ( SELECT bar.c,
>             bar.d
>            FROM bar) subquery_1
>   WHERE subquery.a = subquery_1.c

It doesn't play that well if you have something called subquery though:

CREATE OR REPLACE VIEW test_view AS
  SELECT *
    FROM (SELECT a, b FROM foo),
         (SELECT c, d FROM bar), (select relname from pg_class limit
1)  as subquery
   WHERE a = c;

\sv test_view
CREATE OR REPLACE VIEW public.test_view AS
 SELECT subquery.a,
    subquery.b,
    subquery_1.c,
    subquery_1.d,
    subquery_2.relname
   FROM ( SELECT foo.a,
            foo.b
           FROM foo) subquery,
    ( SELECT bar.c,
            bar.d
           FROM bar) subquery_1,
    ( SELECT pg_class.relname
           FROM pg_class
         LIMIT 1) subquery_2
  WHERE subquery.a = subquery_1.c

While the output is a valid query, it's not nice that it's replacing a
user provided alias with another one (or force an alias if you have a
relation called subquery).  More generally, I'm -0.5 on the feature.
I prefer to force using SQL-compliant queries, and also not take bad
habits.

On Mon, 27 Jun 2022 at 16:12, Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> It doesn't play that well if you have something called subquery though:
>
> [example that changes a user-provided alias]
>
> While the output is a valid query, it's not nice that it's replacing a
> user provided alias with another one (or force an alias if you have a
> relation called subquery).

It's already the case that user-provided aliases can get replaced by
new ones in the query-deparsing code, e.g.:

CREATE OR REPLACE VIEW test_view AS
  SELECT x.a, y.b
    FROM foo AS x,
         (SELECT b FROM foo AS x) AS y;

\sv test_view

CREATE OR REPLACE VIEW public.test_view AS
 SELECT x.a,
    y.b
   FROM foo x,
    ( SELECT x_1.b
           FROM foo x_1) y

and similarly it may invent technically unnecessary aliases where
there were none before. The query-deparsing code has never been
alias-preserving, unless you take care to give everything a globally
unique alias.

Regards,
Dean

On Mon, 27 Jun 2022 at 19:43, David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Mon, Jun 27, 2022 at 11:25 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>
>> On Mon, 27 Jun 2022 at 16:12, Julien Rouhaud <rjuju123@gmail.com> wrote:
>> >
>> > It doesn't play that well if you have something called subquery though:
>> >
>> > [example that changes a user-provided alias]
>> >
>> > While the output is a valid query, it's not nice that it's replacing a
>> > user provided alias with another one (or force an alias if you have a
>> > relation called subquery).
>>
>> It's already the case that user-provided aliases can get replaced by
>> new ones in the query-deparsing code, e.g.:
>>
>
> Regardless, is there any reason to not just prefix our made-up aliases with "pg_" to make it perfectly clear they
weregenerated by the system and are basically implementation details as opposed to something that appeared in the
originallywritten query?
 
>
> I suppose, "because we've haven't until now, so why start" suffices...but still doing a rename/suffixing because of
queryrewriting and inventing one where we made it optional seem different enough to justify implementing something
different.
>

I think "pg_" would be a bad idea, since it's too easily confused with
things like system catalogs. The obvious precedent we have for a
made-up alias is "unnamed_join", so perhaps "unnamed_subquery" would
be better.

Regards,
Dean

Hi,

On Mon, Jun 27, 2022 at 12:03:20PM -0400, Isaac Morland wrote:
> On Mon, 27 Jun 2022 at 11:12, Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> > More generally, I'm -0.5 on the feature.
> > I prefer to force using SQL-compliant queries, and also not take bad
> > habits.
> >
>
> As to forcing SQL-complaint queries, that ship sailed a long time ago:
> Postgres allows but does not enforce the use of SQL-compliant queries, and
> many of its important features are extensions anyway, so forcing SQL
> compliant queries is out of the question (although I could see the utility
> of a mode where it warns or errors on non-compliant queries, at least in
> principle).

Sure, but it doesn't mean that we should support even more non-compliant syntax
without any restraint.  In this case, I don't see much benefit as it's not
solving performance problem or something like that.

> As to bad habits, I'm having trouble understanding. Why do you think
> leaving the alias off a subquery is a bad habit (assuming it were allowed)?

I think It's a bad habit because as far as I can see it's not supported on
mysql or sqlserver.

> If the name is never used, why are we required to supply it?

I'm not saying that I'm thrilled having to do so, but it's also not a huge
trouble.  And since it's required I have the habit to automatically put some
random alias if I'm writing some one shot query that indeed doesn't need to use
the alias.

But similarly, I many times relied on the fact that writable CTE are executed
even if not explicitly referenced.  So by the same argument shouldn't we allow
something like this?

WITH (INSERT INTO t SELECT * pending WHERE ts < now())
SELECT now() AS last_processing_time;

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> This was discussed previously in [1], and there seemed to be general
> consensus in favour of it, but no new patch emerged.

As I said in that thread, I'm not super enthused about this, but I was
clearly in the minority so I think it should go forward.

> Attached is a patch that takes the approach of not generating an alias
> at all, which seems to be neater and simpler, and less code than
> trying to generate a unique alias.

Hm.  Looking at the code surrounding what you touched, I'm reminded
that we allow JOIN nodes to not have an alias, and represent that
situation by rte->alias == NULL.  I wonder if it'd be better in the
long run to make alias-less subqueries work similarly, rather than
generating something that after-the-fact will be indistinguishable
from a user-written alias.  If that turns out to not work well,
I'd agree with "unnamed_subquery" as the inserted name.

Also, what about VALUES clauses?  It seems inconsistent to remove
this restriction for sub-SELECT but not VALUES.  Actually it looks
like your patch already does remove that restriction in gram.y,
but you didn't follow through elsewhere.

As far as the docs go, I think it's sufficient to mention the
inconsistency with SQL down at the bottom; we don't need a
redundant in-line explanation.

            regards, tom lane

On Tue, 5 Jul 2022 at 19:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> > This was discussed previously in [1], and there seemed to be general
> > consensus in favour of it, but no new patch emerged.
>
> As I said in that thread, I'm not super enthused about this, but I was
> clearly in the minority so I think it should go forward.
>

Cool. Thanks for looking.


> > Attached is a patch that takes the approach of not generating an alias
> > at all, which seems to be neater and simpler, and less code than
> > trying to generate a unique alias.
>
> Hm.  Looking at the code surrounding what you touched, I'm reminded
> that we allow JOIN nodes to not have an alias, and represent that
> situation by rte->alias == NULL.  I wonder if it'd be better in the
> long run to make alias-less subqueries work similarly,

That is what the patch does: transformRangeSubselect() passes a NULL
alias to addRangeTableEntryForSubquery(), which has been modified to
cope with that in a similar way to addRangeTableEntryForJoin() and
other addRangeTableEntryFor...() functions.

So for example, with the following query, this is what the output from
the parser looks like:

SELECT * FROM (SELECT 1);

query->rtable:
  rte:
    rtekind = RTE_SUBQUERY
    alias = NULL
    eref = { aliasname = "subquery", colnames = ... }


> rather than
> generating something that after-the-fact will be indistinguishable
> from a user-written alias.  If that turns out to not work well,
> I'd agree with "unnamed_subquery" as the inserted name.
>

The result is distinguishable from a user-written alias, because
rte->alias is NULL. I think the confusion is that when I suggested
using "unnamed_subquery", I was referring to rte->eref->aliasname, and
I still think it's a good idea to change that, for consistency with
unnamed joins.


> Also, what about VALUES clauses?  It seems inconsistent to remove
> this restriction for sub-SELECT but not VALUES.  Actually it looks
> like your patch already does remove that restriction in gram.y,
> but you didn't follow through elsewhere.
>

It does support unnamed VALUES clauses in the FROM list (there's a
regression test exercising that). It wasn't necessary to make any
additional code changes because addRangeTableEntryForValues() already
supported having a NULL alias, and it all just flowed through.

In fact, the grammar forces you to enclose a VALUES clause in the FROM
list in parentheses, so this ends up being an unnamed subquery in the
FROM list as well. For example:

SELECT * FROM (VALUES(1),(2),(3));

produces

query->rtable:
  rte:
    rtekind = RTE_SUBQUERY
    alias = NULL
    eref = { aliasname = "subquery", colnames = ... }
    subquery->rtable:
      rte:
        rtekind = RTE_VALUES
        alias = NULL
        eref = { aliasname = "*VALUES*", colnames = ... }

So it's not really any different from a normal subquery.


> As far as the docs go, I think it's sufficient to mention the
> inconsistency with SQL down at the bottom; we don't need a
> redundant in-line explanation.

OK, fair enough.

I'll post an update in a little while, but first, I found a bug, which
revealed a pre-existing bug in transformLockingClause(). I'll start a
new thread for that, since it'd be good to get that resolved
independently of this patch.

Regards,
Dean

On Wed, 6 Jul 2022 at 15:09, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> I'll post an update in a little while, but first, I found a bug, which
> revealed a pre-existing bug in transformLockingClause(). I'll start a
> new thread for that, since it'd be good to get that resolved
> independently of this patch.
>

Attached is an update with the following changes:

* Docs updated as suggested.
* transformLockingClause() updated to skip subquery and values rtes
without aliases.
* eref->aliasname changed to "unnamed_subquery" for subqueries without aliases.

Regards,
Dean

On Sat, 9 Jul 2022 at 12:24, Zhihong Yu <zyu@yugabyte.com> wrote:
>
>  It seems the code would be more readable if you keep the assignment in else block below:
>
> +                   else if (rte->rtekind == RTE_SUBQUERY ||
> +                            rte->rtekind == RTE_VALUES)
>                         continue;
> -                   rtename = rte->join_using_alias->aliasname;
>                 }
> -               else
> -                   rtename = rte->eref->aliasname;
>
> because rtename would be assigned in the `rte->rtekind == RTE_JOIN` case.
>

But then it would need 2 else blocks, one inside the rte->alias ==
NULL block, for when rtekind is not RTE_JOIN, RTE_SUBQUERY or
RTE_VALUES, and another after the block, for when rte->alias != NULL.
I find it more readable this way.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Attached is an update with the following changes:

> * Docs updated as suggested.
> * transformLockingClause() updated to skip subquery and values rtes
> without aliases.
> * eref->aliasname changed to "unnamed_subquery" for subqueries without aliases.

This looks good to me.  Marked RFC.

            regards, tom lane

Erwin Brandstetter <brsaweda@gmail.com> writes:
> On Mon, 2 Oct 2023 at 00:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> The only place that exposes the eref's made-up relation name is the
>> existing query deparsing code in ruleutils.c, which uniquifies it and
>> generates SQL spec-compliant output. For example:

> I ran into one other place: error messages.
> SELECT unnamed_subquery.a
> FROM (SELECT 1 AS a)
> ERROR:  There is an entry for table "unnamed_subquery", but it cannot be
> referenced from this part of the query.invalid reference to FROM-clause
> entry for table "unnamed_subquery"

Yeah, that's exposing more of the implementation than we really want.

> Notably, the same does not happen for "unnamed_subquery_1":
> SELECT unnamed_subquery_1.a
> FROM (SELECT 1 AS a), (SELECT 1 AS a)
> ERROR:  missing FROM-clause entry for table "unnamed_subquery_1"

Actually, that happens because "unnamed_subquery_1" *isn't* in the
parse tree.  As implemented, both RTEs are labeled "unnamed_subquery"
in the parser output, and it's ruleutils that de-duplicates them.

I'm inclined to think we should avoid letting "unnamed_subquery"
appear in the parse tree, too.  It might not be a good idea to
try to leave the eref field null, but could we set it to an
empty string instead, that is

-    eref = alias ? copyObject(alias) : makeAlias("unnamed_subquery", NIL);
+    eref = alias ? copyObject(alias) : makeAlias("", NIL);

and then let ruleutils replace that with "unnamed_subquery"?  This
would prevent accessing the subquery name in the way Erwin shows,
because we don't let you write an empty identifier in SQL:

regression=# select "".a from (select 1 as a);
ERROR:  zero-length delimited identifier at or near """"
LINE 1: select "".a from (select 1 as a);
               ^

However, there might then be some parser error messages that
refer to subquery "", so I'm not sure if this is totally
without surprises either.

            regards, tom lane

On Mon, 2 Oct 2023 at 01:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Erwin Brandstetter <brsaweda@gmail.com> writes:
> > On Mon, 2 Oct 2023 at 00:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >> The only place that exposes the eref's made-up relation name is the
> >> existing query deparsing code in ruleutils.c, which uniquifies it and
> >> generates SQL spec-compliant output. For example:
>
> > I ran into one other place: error messages.
> > SELECT unnamed_subquery.a
> > FROM (SELECT 1 AS a)
> > ERROR:  There is an entry for table "unnamed_subquery", but it cannot be
> > referenced from this part of the query.invalid reference to FROM-clause
> > entry for table "unnamed_subquery"
>
> Yeah, that's exposing more of the implementation than we really want.
>

Note that this isn't a new issue, specific to unnamed subqueries. The
same thing happens for unnamed joins:

create table foo(a int);
create table bar(a int);
select unnamed_join.a from foo join bar using (a);

ERROR:  invalid reference to FROM-clause entry for table "unnamed_join"
LINE 1: select unnamed_join.a from foo join bar using (a);
               ^
DETAIL:  There is an entry for table "unnamed_join", but it cannot be
referenced from this part of the query.


And there's a similar problem with VALUES RTEs:

insert into foo values (1),(2) returning "*VALUES*".a;

ERROR:  invalid reference to FROM-clause entry for table "*VALUES*"
LINE 1: insert into foo values (1),(2) returning "*VALUES*".a;
                                                 ^
DETAIL:  There is an entry for table "*VALUES*", but it cannot be
referenced from this part of the query.

> I'm inclined to think we should avoid letting "unnamed_subquery"
> appear in the parse tree, too.  It might not be a good idea to
> try to leave the eref field null, but could we set it to an
> empty string instead, that is
>
> -       eref = alias ? copyObject(alias) : makeAlias("unnamed_subquery", NIL);
> +       eref = alias ? copyObject(alias) : makeAlias("", NIL);
>
> and then let ruleutils replace that with "unnamed_subquery"?

Hmm, I think that there would be other side-effects if we did that --
at least doing it for VALUES RTEs would also require additional
changes to retain current EXPLAIN output. I think perhaps it would be
better to try for a more targeted fix of the parser error reporting.

In searchRangeTableForRel() we try to find any RTE that could possibly
match the RangeVar, but certain kinds of RTE don't naturally have
names, and if they also haven't been given aliases, then they can't
possibly match anywhere in the query (and thus it's misleading to
report that they can't be referred to from specific places).

So I think perhaps it's better to just have searchRangeTableForRel()
exclude these kinds of RTE, if they haven't been given an alias.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Mon, 2 Oct 2023 at 01:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, that's exposing more of the implementation than we really want.

> Note that this isn't a new issue, specific to unnamed subqueries. The
> same thing happens for unnamed joins:

True, and we've had few complaints about that.  Still, if we can
clean it up without too much effort, let's do so.

> So I think perhaps it's better to just have searchRangeTableForRel()
> exclude these kinds of RTE, if they haven't been given an alias.

Would we need a new flag in the ParseNamespaceItem data structure,
or will the existing data serve?  I see how to do this if we add
a "doesn't really have a name" flag, but it's not clear to me that
we can reliably identify them otherwise.  Maybe a test involving
the rtekind and whether the "alias" field is set would do, but
that way seems a bit ugly.

            regards, tom lane