On Fri, Oct 25, 2024 at 10:35 PM Tom Lane <[email protected]> wrote:
> Yasir <[email protected]> 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
>
>
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 6593fd7d81..447e406255 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -932,7 +932,7 @@ interpret_AS_clause(Oid languageOid, const char *languageName,
pstate->p_sourcetext = queryString;
sql_fn_parser_setup(pstate, pinfo);
- q = transformStmt(pstate, stmt);
+ q = transformStmt(pstate, stmt, NULL);
if (q->commandType == CMD_UTILITY)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -951,7 +951,7 @@ interpret_AS_clause(Oid languageOid, const char *languageName,
pstate->p_sourcetext = queryString;
sql_fn_parser_setup(pstate, pinfo);
- q = transformStmt(pstate, sql_body_in);
+ q = transformStmt(pstate, sql_body_in, NULL);
if (q->commandType == CMD_UTILITY)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 28fed9d87f..827b560abe 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -64,7 +64,7 @@ static OnConflictExpr *transformOnConflictClause(ParseState *pstate,
OnConflictClause *onConflictClause);
static int count_rowexpr_columns(ParseState *pstate, Node *expr);
static Query *transformSelectStmt(ParseState *pstate, SelectStmt *stmt);
-static Query *transformValuesClause(ParseState *pstate, SelectStmt *stmt);
+static Query *transformValuesClause(ParseState *pstate, SelectStmt *stmt, Alias *alias);
static Query *transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt);
static Node *transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
bool isTopLevel, List **targetlist);
@@ -220,6 +220,7 @@ parse_analyze_withcb(RawStmt *parseTree, const char *sourceText,
Query *
parse_sub_analyze(Node *parseTree, ParseState *parentParseState,
CommonTableExpr *parentCTE,
+ Alias *alias,
bool locked_from_parent,
bool resolve_unknowns)
{
@@ -230,7 +231,7 @@ parse_sub_analyze(Node *parseTree, ParseState *parentParseState,
pstate->p_locked_from_parent = locked_from_parent;
pstate->p_resolve_unknowns = resolve_unknowns;
- query = transformStmt(pstate, parseTree);
+ query = transformStmt(pstate, parseTree, alias);
free_parsestate(pstate);
@@ -300,7 +301,7 @@ transformOptionalSelectInto(ParseState *pstate, Node *parseTree)
}
}
- return transformStmt(pstate, parseTree);
+ return transformStmt(pstate, parseTree, NULL);
}
/*
@@ -308,7 +309,7 @@ transformOptionalSelectInto(ParseState *pstate, Node *parseTree)
* recursively transform a Parse tree into a Query tree.
*/
Query *
-transformStmt(ParseState *pstate, Node *parseTree)
+transformStmt(ParseState *pstate, Node *parseTree, Alias *alias)
{
Query *result;
@@ -363,7 +364,7 @@ transformStmt(ParseState *pstate, Node *parseTree)
SelectStmt *n = (SelectStmt *) parseTree;
if (n->valuesLists)
- result = transformValuesClause(pstate, n);
+ result = transformValuesClause(pstate, n, alias);
else if (n->op == SETOP_NONE)
result = transformSelectStmt(pstate, n);
else
@@ -717,7 +718,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
sub_pstate->p_namespace = sub_namespace;
sub_pstate->p_resolve_unknowns = false;
- selectQuery = transformStmt(sub_pstate, stmt->selectStmt);
+ selectQuery = transformStmt(sub_pstate, stmt->selectStmt, NULL);
free_parsestate(sub_pstate);
@@ -1477,7 +1478,7 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
* SELECT * FROM (VALUES ...) AS "*VALUES*"
*/
static Query *
-transformValuesClause(ParseState *pstate, SelectStmt *stmt)
+transformValuesClause(ParseState *pstate, SelectStmt *stmt, Alias *alias)
{
Query *qry = makeNode(Query);
List *exprsLists = NIL;
@@ -1638,7 +1639,7 @@ transformValuesClause(ParseState *pstate, SelectStmt *stmt)
*/
nsitem = addRangeTableEntryForValues(pstate, exprsLists,
coltypes, coltypmods, colcollations,
- NULL, lateral, true);
+ alias, lateral, true);
addNSItemToQuery(pstate, nsitem, true, true, true);
/*
@@ -2074,7 +2075,7 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
* of this sub-query, because they are not in the toplevel pstate's
* namespace list.
*/
- selectQuery = parse_sub_analyze((Node *) stmt, pstate,
+ selectQuery = parse_sub_analyze((Node *) stmt, pstate, NULL,
NULL, false, false);
/*
@@ -2887,7 +2888,7 @@ transformDeclareCursorStmt(ParseState *pstate, DeclareCursorStmt *stmt)
"ASENSITIVE", "INSENSITIVE")));
/* Transform contained query, not allowing SELECT INTO */
- query = transformStmt(pstate, stmt->query);
+ query = transformStmt(pstate, stmt->query, NULL);
stmt->query = (Node *) query;
/* Grammar should not have allowed anything but SELECT */
@@ -3016,7 +3017,7 @@ transformCreateTableAsStmt(ParseState *pstate, CreateTableAsStmt *stmt)
Query *query;
/* transform contained query, not allowing SELECT INTO */
- query = transformStmt(pstate, stmt->query);
+ query = transformStmt(pstate, stmt->query, NULL);
stmt->query = (Node *) query;
/* additional work needed for CREATE MATERIALIZED VIEW */
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 8118036495..42c1786f69 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -429,7 +429,7 @@ transformRangeSubselect(ParseState *pstate, RangeSubselect *r)
* have an alias, it can't be explicitly selected for locking, but locking
* might still be required (if there is an all-tables locking clause).
*/
- query = parse_sub_analyze(r->subquery, pstate, NULL,
+ query = parse_sub_analyze(r->subquery, pstate, NULL, r->alias,
isLockedRefname(pstate,
r->alias == NULL ? NULL :
r->alias->aliasname),
diff --git a/src/backend/parser/parse_cte.c b/src/backend/parser/parse_cte.c
index 6826d4f36a..e05e82b912 100644
--- a/src/backend/parser/parse_cte.c
+++ b/src/backend/parser/parse_cte.c
@@ -312,7 +312,7 @@ analyzeCTE(ParseState *pstate, CommonTableExpr *cte)
}
/* Now we can get on with analyzing the CTE's query */
- query = parse_sub_analyze(cte->ctequery, pstate, cte, false, true);
+ query = parse_sub_analyze(cte->ctequery, pstate, cte, NULL, false, true);
cte->ctequery = (Node *) query;
/*
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index aba3546ed1..5db4779200 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1880,7 +1880,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
/*
* OK, let's transform the sub-SELECT.
*/
- qtree = parse_sub_analyze(sublink->subselect, pstate, NULL, false, true);
+ qtree = parse_sub_analyze(sublink->subselect, pstate, NULL, NULL, false, true);
/*
* Check that we got a SELECT. Anything else should be impossible given
@@ -3748,7 +3748,7 @@ transformJsonArrayQueryConstructor(ParseState *pstate,
/* Transform query only for counting target list entries. */
qpstate = make_parsestate(pstate);
- query = transformStmt(qpstate, ctor->query);
+ query = transformStmt(qpstate, ctor->query, NULL);
if (count_nonjunk_tlist_entries(query->targetList) != 1)
ereport(ERROR,
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 639cfa443e..3ac1537522 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3085,7 +3085,7 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString,
addNSItemToQuery(sub_pstate, newnsitem, false, true, false);
/* Transform the rule action statement */
- top_subqry = transformStmt(sub_pstate, action);
+ top_subqry = transformStmt(sub_pstate, action, NULL);
/*
* We cannot support utility-statement actions (eg NOTIFY) with
diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h
index 28b66fccb4..66477dc36f 100644
--- a/src/include/parser/analyze.h
+++ b/src/include/parser/analyze.h
@@ -36,6 +36,7 @@ extern Query *parse_analyze_withcb(RawStmt *parseTree, const char *sourceText,
extern Query *parse_sub_analyze(Node *parseTree, ParseState *parentParseState,
CommonTableExpr *parentCTE,
+ Alias *alias,
bool locked_from_parent,
bool resolve_unknowns);
@@ -47,7 +48,7 @@ extern List *transformUpdateTargetList(ParseState *pstate,
extern List *transformReturningList(ParseState *pstate, List *returningList,
ParseExprKind exprKind);
extern Query *transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree);
-extern Query *transformStmt(ParseState *pstate, Node *parseTree);
+extern Query *transformStmt(ParseState *pstate, Node *parseTree, Alias *alias);
extern bool stmt_requires_parse_analysis(RawStmt *parseTree);
extern bool analyze_requires_snapshot(RawStmt *parseTree);
