Thread: BUG #18589: pg_get_viewdef returns wrong query
The following bug has been logged on the website: Bug reference: 18589 Logged by: Quynh Tran Email address: tlhquynh@gmail.com PostgreSQL version: 16.3 Operating system: cloud.google.com/container-optimized-os Description: Hi, In PostgreSQL 16.3, after I created a view by a statement like "create view kvview as select key as value, value as key from kv order by value", I retrieved a different definition from information_schema.views "SELECT key AS value, value AS key FROM kv ORDER BY key". I expect ORDER BY column be value. I tried the same in PostgreSQL 14.12 and 15.7 and got correct equivalent definitions. "SELECT keyvalue.key AS value, keyvalue.value AS key FROM keyvalue ORDER BY keyvalue.key" So this seems to be a regression. Below are steps to reproduce in PostgreSQL 16.3: > create table kv (key int primary key, value text); Query OK, 0 rows affected (5.27 sec) > insert into kv values (1, 'z'), (2, 'z'), (3, 'y'); Query OK, 3 rows affected (0.40 sec) > select key as value, value as key from kv order by value; -- This is what we want to see. +-------+-----+ | value | key | +-------+-----+ | 1 | z | | 2 | z | | 3 | y | +-------+-----+ 3 rows in set (4.04 msecs) > create view kvview as select key as value, value as key from kv order by value; -- Create view with same definition as the query above. Query OK, 0 rows affected (11.00 sec) > select * from kvview; -- View also has correct result. +-------+-----+ | value | key | +-------+-----+ | 1 | z | | 2 | z | | 3 | y | +-------+-----+ 3 rows in set (4.05 msecs) > select table_name, view_definition from information_schema.views; -- But information_schema displays the wrong view definition! +------------+------------------------------------------------------------+ | table_name | view_definition | +------------+------------------------------------------------------------+ | kvview | SELECT key AS value, value AS key FROM kv ORDER BY key | +------------+------------------------------------------------------------+ 1 rows in set (11.67 msecs) > select key as value, value as key from kv order by key; -- The view definition would give the wrong result if that were what was actually executed. +-------+-----+ | value | key | +-------+-----+ | 3 | y | | 1 | z | | 2 | z | +-------+-----+ 3 rows in set (16.76 msecs)
On Mon, Aug 26, 2024 at 7:22 PM PG Bug reporting form <noreply@postgresql.org> wrote: > In PostgreSQL 16.3, after I created a view by a statement like > "create view kvview as select key as value, value as key from kv order by > value", > I retrieved a different definition from information_schema.views > "SELECT key AS value, value AS key FROM kv ORDER BY key". > I expect ORDER BY column be value. > > I tried the same in PostgreSQL 14.12 and 15.7 and got correct equivalent > definitions. > "SELECT keyvalue.key AS value, keyvalue.value AS key FROM keyvalue ORDER BY > keyvalue.key" > > So this seems to be a regression. This is broken starting from 1b4d280ea. The "new" and "old" entries in a view's rangetable were removed, which results in the varprefix flag being set to false in this case, as there is now only one rtable entry. Thanks Richard
Richard Guo <guofenglinux@gmail.com> writes:
> This is broken starting from 1b4d280ea. The "new" and "old" entries
> in a view's rangetable were removed, which results in the varprefix
> flag being set to false in this case, as there is now only one rtable
> entry.
Yeah. Interesting that that old behavior accidentally hid this
ambiguity. I wonder whether there is a way to provoke mis-deparsing
even before 1b4d280ea.
The simplest fix would be to force varprefix on whenever deparsing
an ORDER BY list, as in the first patch attached. However, that
turns out to cause quite a lot of changes in regression test outputs
(which I didn't bother to include in the patch), which is unsurprising
given that it's reverting some of the 1b4d280ea output changes.
I don't like that too much, first because it seems like kind of a lot
of behavior churn for a stable branch, and second because it's just
plain ugly that the same variable is printed in different ways
depending on where it is in the query.
The second patch attached is what I think we should really do.
What it does is to prefix Vars only in the actually-troublesome
case where there is a conflicting SELECT-list entry, so that you
get the clutter only when doing something that's fairly ill-advised.
To do that, get_variable needs access to the SELECT list. We already
have that available in deparse_context, but the field name is
"windowTList" which seems rather misleading if it's to be used for
other purposes too. So I renamed and relocated that field to make
things less confusing. The other debatable point in the patch is
how to tell get_variable that we're considering an ORDER BY item.
I chose to re-use the special_exprkind field that's already there,
but I'm not totally convinced that that's a better solution than
adding another bool field. The problem is that we also use
get_rule_sortgroupclause for GROUP BY, so that it's temporarily hiding
the EXPR_KIND_GROUP_BY setting. That doesn't break anything as long
as we only change it while deparsing a Var, but it seems kind of
fragile. (On the whole, I don't think special_exprkind is all that
well thought out: aside from having no useful documentation, this
experience shows that it's not nearly as extensible as the author
probably believed. I wonder if we should replace it with
"bool in_group_by" or the like.)
With the second patch, there are no changes in existing regression
test outputs, so I added a new test.
Thoughts?
regards, tom lane
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 00eda1b34c..14d7ed9cd1 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -6306,20 +6306,32 @@ get_rule_sortgroupclause(Index ref, List *tlist, bool force_colno,
* Use column-number form if requested by caller. Otherwise, if
* expression is a constant, force it to be dumped with an explicit cast
* as decoration --- this is because a simple integer constant is
- * ambiguous (and will be misinterpreted by findTargetlistEntry()) if we
- * dump it without any decoration. If it's anything more complex than a
- * simple Var, then force extra parens around it, to ensure it can't be
- * misinterpreted as a cube() or rollup() construct.
+ * ambiguous (and will be misinterpreted by findTargetlistEntrySQL92()) if
+ * we dump it without any decoration. Similarly, if it's just a Var,
+ * there is risk of misinterpretation if the column name is reassigned in
+ * the SELECT list, so we need to force table qualification. And, if it's
+ * anything more complex than a simple Var, then force extra parens around
+ * it, to ensure it can't be misinterpreted as a cube() or rollup()
+ * construct.
*/
if (force_colno)
{
Assert(!tle->resjunk);
appendStringInfo(buf, "%d", tle->resno);
}
- else if (expr && IsA(expr, Const))
+ else if (!expr)
+ /* do nothing, probably can't happen */ ;
+ else if (IsA(expr, Const))
get_const_expr((Const *) expr, context, 1);
- else if (!expr || IsA(expr, Var))
+ else if (IsA(expr, Var))
+ {
+ /* Force table-qualification by setting varprefix to true */
+ bool save_varprefix = context->varprefix;
+
+ context->varprefix = true;
get_rule_expr(expr, context, true);
+ context->varprefix = save_varprefix;
+ }
else
{
/*
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 00eda1b34c..7e06e03b92 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -114,8 +114,8 @@ typedef struct
{
StringInfo buf; /* output buffer to append to */
List *namespaces; /* List of deparse_namespace nodes */
+ List *targetList; /* Current query level's SELECT targetlist */
List *windowClause; /* Current query level's WINDOW clause */
- List *windowTList; /* targetlist for resolving WINDOW clause */
int prettyFlags; /* enabling of pretty-print functions */
int wrapColumn; /* max line length, or -1 for no limit */
int indentLevel; /* current indent level for pretty-print */
@@ -1094,8 +1094,8 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
/* Set up context with one-deep namespace stack */
context.buf = &buf;
context.namespaces = list_make1(&dpns);
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = true;
context.prettyFlags = GET_PRETTY_FLAGS(pretty);
context.wrapColumn = WRAP_COLUMN_DEFAULT;
@@ -3632,8 +3632,8 @@ deparse_expression_pretty(Node *expr, List *dpcontext,
initStringInfo(&buf);
context.buf = &buf;
context.namespaces = dpcontext;
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = forceprefix;
context.prettyFlags = prettyFlags;
context.wrapColumn = WRAP_COLUMN_DEFAULT;
@@ -5283,8 +5283,8 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
context.buf = buf;
context.namespaces = list_make1(&dpns);
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = (list_length(query->rtable) != 1);
context.prettyFlags = prettyFlags;
context.wrapColumn = WRAP_COLUMN_DEFAULT;
@@ -5451,8 +5451,8 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
context.buf = buf;
context.namespaces = lcons(&dpns, list_copy(parentnamespace));
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = (parentnamespace != NIL ||
list_length(query->rtable) != 1);
context.prettyFlags = prettyFlags;
@@ -5691,19 +5691,21 @@ get_select_query_def(Query *query, deparse_context *context,
TupleDesc resultDesc, bool colNamesVisible)
{
StringInfo buf = context->buf;
+ List *save_targetlist;
List *save_windowclause;
- List *save_windowtlist;
bool force_colno;
ListCell *l;
/* Insert the WITH clause if given */
get_with_clause(query, context);
+ /* We may need to consult the SELECT targetlist */
+ save_targetlist = context->targetList;
+ context->targetList = query->targetList;
+
/* Set up context for possible window functions */
save_windowclause = context->windowClause;
context->windowClause = query->windowClause;
- save_windowtlist = context->windowTList;
- context->windowTList = query->targetList;
/*
* If the Query node has a setOperations tree, then it's the top level of
@@ -5809,8 +5811,8 @@ get_select_query_def(Query *query, deparse_context *context,
}
}
+ context->targetList = save_targetlist;
context->windowClause = save_windowclause;
- context->windowTList = save_windowtlist;
}
/*
@@ -6306,20 +6308,37 @@ get_rule_sortgroupclause(Index ref, List *tlist, bool force_colno,
* Use column-number form if requested by caller. Otherwise, if
* expression is a constant, force it to be dumped with an explicit cast
* as decoration --- this is because a simple integer constant is
- * ambiguous (and will be misinterpreted by findTargetlistEntry()) if we
- * dump it without any decoration. If it's anything more complex than a
- * simple Var, then force extra parens around it, to ensure it can't be
- * misinterpreted as a cube() or rollup() construct.
+ * ambiguous (and will be misinterpreted by findTargetlistEntrySQL92()) if
+ * we dump it without any decoration. Similarly, if it's just a Var,
+ * there is risk of misinterpretation if the column name is reassigned in
+ * the SELECT list, so we may need to force table qualification. And, if
+ * it's anything more complex than a simple Var, then force extra parens
+ * around it, to ensure it can't be misinterpreted as a cube() or rollup()
+ * construct.
*/
if (force_colno)
{
Assert(!tle->resjunk);
appendStringInfo(buf, "%d", tle->resno);
}
- else if (expr && IsA(expr, Const))
+ else if (!expr)
+ /* do nothing, probably can't happen */ ;
+ else if (IsA(expr, Const))
get_const_expr((Const *) expr, context, 1);
- else if (!expr || IsA(expr, Var))
- get_rule_expr(expr, context, true);
+ else if (IsA(expr, Var))
+ {
+ /*
+ * Tell get_variable to check for name conflict. Note that we may be
+ * overriding special_exprkind = EXPR_KIND_GROUP_BY here, but that's
+ * okay because we only need to handle that case when considering a
+ * function.
+ */
+ ParseExprKind save_exprkind = context->special_exprkind;
+
+ context->special_exprkind = EXPR_KIND_ORDER_BY;
+ (void) get_variable((Var *) expr, 0, false, context);
+ context->special_exprkind = save_exprkind;
+ }
else
{
/*
@@ -7307,6 +7326,7 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
deparse_columns *colinfo;
char *refname;
char *attname;
+ bool need_prefix;
/* Find appropriate nesting depth */
netlevelsup = var->varlevelsup + levelsup;
@@ -7502,7 +7522,30 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
attname = get_rte_attribute_name(rte, attnum);
}
- if (refname && (context->varprefix || attname == NULL))
+ need_prefix = (context->varprefix || attname == NULL);
+
+ /*
+ * If we're considering a plain Var in an ORDER BY or GROUP BY list, we
+ * may need to use a table-name prefix to prevent findTargetlistEntrySQL92
+ * from misinterpreting the name as an output-column name. To avoid
+ * cluttering the output with unnecessary prefixes, do so only if there is
+ * a name match to a SELECT tlist item that is different from the Var.
+ */
+ if (!need_prefix && context->special_exprkind == EXPR_KIND_ORDER_BY)
+ {
+ foreach_node(TargetEntry, tle, context->targetList)
+ {
+ if (!tle->resjunk && tle->resname &&
+ strcmp(tle->resname, attname) == 0 &&
+ !equal(var, tle->expr))
+ {
+ need_prefix = true;
+ break;
+ }
+ }
+ }
+
+ if (refname && need_prefix)
{
appendStringInfoString(buf, quote_identifier(refname));
appendStringInfoChar(buf, '.');
@@ -10713,7 +10756,7 @@ get_windowfunc_expr_helper(WindowFunc *wfunc, deparse_context *context,
if (wc->name)
appendStringInfoString(buf, quote_identifier(wc->name));
else
- get_rule_windowspec(wc, context->windowTList, context);
+ get_rule_windowspec(wc, context->targetList, context);
break;
}
}
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index f3f8c7b5a2..7b42911d2f 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -824,6 +824,22 @@ View definition:
FROM temp_view_test.tx1 tx1_1
WHERE tx1.y1 = tx1_1.f1));
+-- Test correct deparsing of ORDER BY when there is a name conflict
+create view aliased_order_by as
+select x1 as x2, x2 as x1 from tt1
+ order by x2;
+\d+ aliased_order_by
+ View "testviewschm2.aliased_order_by"
+ Column | Type | Collation | Nullable | Default | Storage | Description
+--------+---------+-----------+----------+---------+---------+-------------
+ x2 | integer | | | | plain |
+ x1 | integer | | | | plain |
+View definition:
+ SELECT x1 AS x2,
+ x2 AS x1
+ FROM tt1
+ ORDER BY tt1.x1;
+
-- Test aliasing of joins
create view view_of_joins as
select * from
@@ -2248,7 +2264,7 @@ drop cascades to view aliased_view_2
drop cascades to view aliased_view_3
drop cascades to view aliased_view_4
DROP SCHEMA testviewschm2 CASCADE;
-NOTICE: drop cascades to 79 other objects
+NOTICE: drop cascades to 80 other objects
DETAIL: drop cascades to table t1
drop cascades to view temporal1
drop cascades to view temporal2
@@ -2275,6 +2291,7 @@ drop cascades to view mysecview9
drop cascades to view unspecified_types
drop cascades to table tt1
drop cascades to table tx1
+drop cascades to view aliased_order_by
drop cascades to view view_of_joins
drop cascades to table tbl1a
drop cascades to view view_of_joins_2a
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 3a78be1b0c..7030744881 100644
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -373,6 +373,14 @@ ALTER TABLE tmp1 RENAME TO tx1;
\d+ aliased_view_3
\d+ aliased_view_4
+-- Test correct deparsing of ORDER BY when there is a name conflict
+
+create view aliased_order_by as
+select x1 as x2, x2 as x1 from tt1
+ order by x2;
+
+\d+ aliased_order_by
+
-- Test aliasing of joins
create view view_of_joins as
On Tue, Aug 27, 2024 at 1:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The second patch attached is what I think we should really do.
> What it does is to prefix Vars only in the actually-troublesome
> case where there is a conflicting SELECT-list entry, so that you
> get the clutter only when doing something that's fairly ill-advised.
I also think this is a better way.
> To do that, get_variable needs access to the SELECT list. We already
> have that available in deparse_context, but the field name is
> "windowTList" which seems rather misleading if it's to be used for
> other purposes too. So I renamed and relocated that field to make
> things less confusing.
+1 to this part.
> The other debatable point in the patch is
> how to tell get_variable that we're considering an ORDER BY item.
> I chose to re-use the special_exprkind field that's already there,
> but I'm not totally convinced that that's a better solution than
> adding another bool field. The problem is that we also use
> get_rule_sortgroupclause for GROUP BY, so that it's temporarily hiding
> the EXPR_KIND_GROUP_BY setting.
It seems to me that this might result in different behavior from
previous when deparsing Vars in a GROUP BY list, due to overriding
special_exprkind from EXPR_KIND_GROUP_BY to EXPR_KIND_ORDER_BY.
For example, with this patch:
create table t (x1 int, x2 int);
create view v as select x1 as x2, x2 as x1 from t group by x1, x2;
select pg_get_viewdef('v', true);
pg_get_viewdef
------------------------
SELECT x1 AS x2, +
x2 AS x1 +
FROM t +
GROUP BY t.x1, t.x2;
(1 row)
It seems that the prefixes in the GROUP BY Vars are unnecessary.
> (On the whole, I don't think special_exprkind is all that
> well thought out: aside from having no useful documentation, this
> experience shows that it's not nearly as extensible as the author
> probably believed. I wonder if we should replace it with
> "bool in_group_by" or the like.)
From my brief 2-minute look at how special_exprkind is used, it seems
that special_exprkind is either EXPR_KIND_GROUP_BY or EXPR_KIND_NONE.
Since it doesn't seem to be extensible as expected, I agree that maybe
a bool field is more straightforward. Then we can have another bool
field for ORDER BY, avoiding the need to override the in-group-by
setting.
Thanks
Richard
On Tue, Aug 27, 2024 at 4:46 PM Richard Guo <guofenglinux@gmail.com> wrote: > From my brief 2-minute look at how special_exprkind is used, it seems > that special_exprkind is either EXPR_KIND_GROUP_BY or EXPR_KIND_NONE. > Since it doesn't seem to be extensible as expected, I agree that maybe > a bool field is more straightforward. Then we can have another bool > field for ORDER BY, avoiding the need to override the in-group-by > setting. Alternatively, can we set special_exprkind = EXPR_KIND_ORDER_BY in get_rule_orderby? I’m looking for a approach that is parallel to how we set special_exprkind = EXPR_KIND_GROUP_BY in get_basic_select_query. Thanks Richard
Richard Guo <guofenglinux@gmail.com> writes:
> Alternatively, can we set special_exprkind = EXPR_KIND_ORDER_BY in
> get_rule_orderby? I’m looking for a approach that is parallel to how
> we set special_exprkind = EXPR_KIND_GROUP_BY in get_basic_select_query.
I don't want to do that because it would result in prefixing Vars
within grouping/ordering expressions, which is unnecessary. Only
a bare Var requires this treatment.
regards, tom lane
Richard Guo <guofenglinux@gmail.com> writes:
> On Tue, Aug 27, 2024 at 1:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The other debatable point in the patch is
>> how to tell get_variable that we're considering an ORDER BY item.
> It seems to me that this might result in different behavior from
> previous when deparsing Vars in a GROUP BY list, due to overriding
> special_exprkind from EXPR_KIND_GROUP_BY to EXPR_KIND_ORDER_BY.
Yeah, I was uncertain whether to bother with that detail. It's true
that there's no bug in GROUP BY because the parser prefers SQL99
semantics in that case, but maybe we shouldn't have this code assuming
that? And it wasn't easy to do with the special_exprkind approach.
But it is pretty easy with the two-bools approach, so I fixed that in
the attached v2.
regards, tom lane
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 00eda1b34c..2a624a9b9c 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -50,7 +50,6 @@
#include "optimizer/optimizer.h"
#include "parser/parse_agg.h"
#include "parser/parse_func.h"
-#include "parser/parse_node.h"
#include "parser/parse_oper.h"
#include "parser/parse_relation.h"
#include "parser/parser.h"
@@ -114,14 +113,14 @@ typedef struct
{
StringInfo buf; /* output buffer to append to */
List *namespaces; /* List of deparse_namespace nodes */
+ List *targetList; /* Current query level's SELECT targetlist */
List *windowClause; /* Current query level's WINDOW clause */
- List *windowTList; /* targetlist for resolving WINDOW clause */
int prettyFlags; /* enabling of pretty-print functions */
int wrapColumn; /* max line length, or -1 for no limit */
int indentLevel; /* current indent level for pretty-print */
bool varprefix; /* true to print prefixes on Vars */
- ParseExprKind special_exprkind; /* set only for exprkinds needing special
- * handling */
+ bool in_group_by; /* deparsing GROUP BY clause? */
+ bool var_in_order_by; /* deparsing simple Var in ORDER BY? */
Bitmapset *appendparents; /* if not null, map child Vars of these relids
* back to the parent rel */
} deparse_context;
@@ -515,7 +514,7 @@ static char *generate_qualified_relation_name(Oid relid);
static char *generate_function_name(Oid funcid, int nargs,
List *argnames, Oid *argtypes,
bool has_variadic, bool *use_variadic_p,
- ParseExprKind special_exprkind);
+ bool in_group_by);
static char *generate_operator_name(Oid operid, Oid arg1, Oid arg2);
static void add_cast_to(StringInfo buf, Oid typid);
static char *generate_qualified_type_name(Oid typid);
@@ -1094,13 +1093,14 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
/* Set up context with one-deep namespace stack */
context.buf = &buf;
context.namespaces = list_make1(&dpns);
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = true;
context.prettyFlags = GET_PRETTY_FLAGS(pretty);
context.wrapColumn = WRAP_COLUMN_DEFAULT;
context.indentLevel = PRETTYINDENT_STD;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.in_group_by = false;
+ context.var_in_order_by = false;
context.appendparents = NULL;
get_rule_expr(qual, &context, false);
@@ -1111,7 +1111,7 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
appendStringInfo(&buf, "EXECUTE FUNCTION %s(",
generate_function_name(trigrec->tgfoid, 0,
NIL, NULL,
- false, NULL, EXPR_KIND_NONE));
+ false, NULL, false));
if (trigrec->tgnargs > 0)
{
@@ -2992,7 +2992,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
appendStringInfo(&buf, " SUPPORT %s",
generate_function_name(proc->prosupport, 1,
NIL, argtypes,
- false, NULL, EXPR_KIND_NONE));
+ false, NULL, false));
}
if (oldlen != buf.len)
@@ -3632,13 +3632,14 @@ deparse_expression_pretty(Node *expr, List *dpcontext,
initStringInfo(&buf);
context.buf = &buf;
context.namespaces = dpcontext;
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = forceprefix;
context.prettyFlags = prettyFlags;
context.wrapColumn = WRAP_COLUMN_DEFAULT;
context.indentLevel = startIndent;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.in_group_by = false;
+ context.var_in_order_by = false;
context.appendparents = NULL;
get_rule_expr(expr, &context, showimplicit);
@@ -5283,13 +5284,14 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
context.buf = buf;
context.namespaces = list_make1(&dpns);
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = (list_length(query->rtable) != 1);
context.prettyFlags = prettyFlags;
context.wrapColumn = WRAP_COLUMN_DEFAULT;
context.indentLevel = PRETTYINDENT_STD;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.in_group_by = false;
+ context.var_in_order_by = false;
context.appendparents = NULL;
set_deparse_for_query(&dpns, query, NIL);
@@ -5451,14 +5453,15 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
context.buf = buf;
context.namespaces = lcons(&dpns, list_copy(parentnamespace));
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = (parentnamespace != NIL ||
list_length(query->rtable) != 1);
context.prettyFlags = prettyFlags;
context.wrapColumn = wrapColumn;
context.indentLevel = startIndent;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.in_group_by = false;
+ context.var_in_order_by = false;
context.appendparents = NULL;
set_deparse_for_query(&dpns, query, parentnamespace);
@@ -5691,19 +5694,21 @@ get_select_query_def(Query *query, deparse_context *context,
TupleDesc resultDesc, bool colNamesVisible)
{
StringInfo buf = context->buf;
+ List *save_targetlist;
List *save_windowclause;
- List *save_windowtlist;
bool force_colno;
ListCell *l;
/* Insert the WITH clause if given */
get_with_clause(query, context);
+ /* We may need to consult the SELECT targetlist */
+ save_targetlist = context->targetList;
+ context->targetList = query->targetList;
+
/* Set up context for possible window functions */
save_windowclause = context->windowClause;
context->windowClause = query->windowClause;
- save_windowtlist = context->windowTList;
- context->windowTList = query->targetList;
/*
* If the Query node has a setOperations tree, then it's the top level of
@@ -5809,8 +5814,8 @@ get_select_query_def(Query *query, deparse_context *context,
}
}
+ context->targetList = save_targetlist;
context->windowClause = save_windowclause;
- context->windowTList = save_windowtlist;
}
/*
@@ -5961,15 +5966,15 @@ get_basic_select_query(Query *query, deparse_context *context,
/* Add the GROUP BY clause if given */
if (query->groupClause != NULL || query->groupingSets != NULL)
{
- ParseExprKind save_exprkind;
+ bool save_in_group_by;
appendContextKeyword(context, " GROUP BY ",
-PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
if (query->groupDistinct)
appendStringInfoString(buf, "DISTINCT ");
- save_exprkind = context->special_exprkind;
- context->special_exprkind = EXPR_KIND_GROUP_BY;
+ save_in_group_by = context->in_group_by;
+ context->in_group_by = true;
if (query->groupingSets == NIL)
{
@@ -5997,7 +6002,7 @@ get_basic_select_query(Query *query, deparse_context *context,
}
}
- context->special_exprkind = save_exprkind;
+ context->in_group_by = save_in_group_by;
}
/* Add the HAVING clause if given */
@@ -6306,20 +6311,32 @@ get_rule_sortgroupclause(Index ref, List *tlist, bool force_colno,
* Use column-number form if requested by caller. Otherwise, if
* expression is a constant, force it to be dumped with an explicit cast
* as decoration --- this is because a simple integer constant is
- * ambiguous (and will be misinterpreted by findTargetlistEntry()) if we
- * dump it without any decoration. If it's anything more complex than a
- * simple Var, then force extra parens around it, to ensure it can't be
- * misinterpreted as a cube() or rollup() construct.
+ * ambiguous (and will be misinterpreted by findTargetlistEntrySQL92()) if
+ * we dump it without any decoration. Similarly, if it's just a Var,
+ * there is risk of misinterpretation if the column name is reassigned in
+ * the SELECT list, so we may need to force table qualification. And, if
+ * it's anything more complex than a simple Var, then force extra parens
+ * around it, to ensure it can't be misinterpreted as a cube() or rollup()
+ * construct.
*/
if (force_colno)
{
Assert(!tle->resjunk);
appendStringInfo(buf, "%d", tle->resno);
}
- else if (expr && IsA(expr, Const))
+ else if (!expr)
+ /* do nothing, probably can't happen */ ;
+ else if (IsA(expr, Const))
get_const_expr((Const *) expr, context, 1);
- else if (!expr || IsA(expr, Var))
- get_rule_expr(expr, context, true);
+ else if (IsA(expr, Var))
+ {
+ /* Tell get_variable to check for name conflict */
+ bool save_var_in_order_by = context->var_in_order_by;
+
+ context->var_in_order_by = true;
+ (void) get_variable((Var *) expr, 0, false, context);
+ context->var_in_order_by = save_var_in_order_by;
+ }
else
{
/*
@@ -7307,6 +7324,7 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
deparse_columns *colinfo;
char *refname;
char *attname;
+ bool need_prefix;
/* Find appropriate nesting depth */
netlevelsup = var->varlevelsup + levelsup;
@@ -7502,7 +7520,31 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
attname = get_rte_attribute_name(rte, attnum);
}
- if (refname && (context->varprefix || attname == NULL))
+ need_prefix = (context->varprefix || attname == NULL);
+
+ /*
+ * If we're considering a plain Var in an ORDER BY (but not GROUP BY)
+ * clause, we may need to add a table-name prefix to prevent
+ * findTargetlistEntrySQL92 from misinterpreting the name as an
+ * output-column name. To avoid cluttering the output with unnecessary
+ * prefixes, do so only if there is a name match to a SELECT tlist item
+ * that is different from the Var.
+ */
+ if (context->var_in_order_by && !context->in_group_by && !need_prefix)
+ {
+ foreach_node(TargetEntry, tle, context->targetList)
+ {
+ if (!tle->resjunk && tle->resname &&
+ strcmp(tle->resname, attname) == 0 &&
+ !equal(var, tle->expr))
+ {
+ need_prefix = true;
+ break;
+ }
+ }
+ }
+
+ if (refname && need_prefix)
{
appendStringInfoString(buf, quote_identifier(refname));
appendStringInfoChar(buf, '.');
@@ -10463,7 +10505,7 @@ get_func_expr(FuncExpr *expr, deparse_context *context,
argnames, argtypes,
expr->funcvariadic,
&use_variadic,
- context->special_exprkind));
+ context->in_group_by));
nargs = 0;
foreach(l, expr->args)
{
@@ -10533,7 +10575,7 @@ get_agg_expr_helper(Aggref *aggref, deparse_context *context,
funcname = generate_function_name(aggref->aggfnoid, nargs, NIL,
argtypes, aggref->aggvariadic,
&use_variadic,
- context->special_exprkind);
+ context->in_group_by);
/* Print the aggregate name, schema-qualified if needed */
appendStringInfo(buf, "%s(%s", funcname,
@@ -10674,7 +10716,7 @@ get_windowfunc_expr_helper(WindowFunc *wfunc, deparse_context *context,
if (!funcname)
funcname = generate_function_name(wfunc->winfnoid, nargs, argnames,
argtypes, false, NULL,
- context->special_exprkind);
+ context->in_group_by);
appendStringInfo(buf, "%s(", funcname);
@@ -10713,7 +10755,7 @@ get_windowfunc_expr_helper(WindowFunc *wfunc, deparse_context *context,
if (wc->name)
appendStringInfoString(buf, quote_identifier(wc->name));
else
- get_rule_windowspec(wc, context->windowTList, context);
+ get_rule_windowspec(wc, context->targetList, context);
break;
}
}
@@ -12420,7 +12462,7 @@ get_tablesample_def(TableSampleClause *tablesample, deparse_context *context)
appendStringInfo(buf, " TABLESAMPLE %s (",
generate_function_name(tablesample->tsmhandler, 1,
NIL, argtypes,
- false, NULL, EXPR_KIND_NONE));
+ false, NULL, false));
nargs = 0;
foreach(l, tablesample->args)
@@ -12840,12 +12882,14 @@ generate_qualified_relation_name(Oid relid)
* the output. For non-FuncExpr cases, has_variadic should be false and
* use_variadic_p can be NULL.
*
+ * in_group_by must be true if we're deparsing a GROUP BY clause.
+ *
* The result includes all necessary quoting and schema-prefixing.
*/
static char *
generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
bool has_variadic, bool *use_variadic_p,
- ParseExprKind special_exprkind)
+ bool in_group_by)
{
char *result;
HeapTuple proctup;
@@ -12870,9 +12914,9 @@ generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
/*
* Due to parser hacks to avoid needing to reserve CUBE, we need to force
- * qualification in some special cases.
+ * qualification of some function names within GROUP BY.
*/
- if (special_exprkind == EXPR_KIND_GROUP_BY)
+ if (in_group_by)
{
if (strcmp(proname, "cube") == 0 || strcmp(proname, "rollup") == 0)
force_qualify = true;
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index f3f8c7b5a2..7b42911d2f 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -824,6 +824,22 @@ View definition:
FROM temp_view_test.tx1 tx1_1
WHERE tx1.y1 = tx1_1.f1));
+-- Test correct deparsing of ORDER BY when there is a name conflict
+create view aliased_order_by as
+select x1 as x2, x2 as x1 from tt1
+ order by x2;
+\d+ aliased_order_by
+ View "testviewschm2.aliased_order_by"
+ Column | Type | Collation | Nullable | Default | Storage | Description
+--------+---------+-----------+----------+---------+---------+-------------
+ x2 | integer | | | | plain |
+ x1 | integer | | | | plain |
+View definition:
+ SELECT x1 AS x2,
+ x2 AS x1
+ FROM tt1
+ ORDER BY tt1.x1;
+
-- Test aliasing of joins
create view view_of_joins as
select * from
@@ -2248,7 +2264,7 @@ drop cascades to view aliased_view_2
drop cascades to view aliased_view_3
drop cascades to view aliased_view_4
DROP SCHEMA testviewschm2 CASCADE;
-NOTICE: drop cascades to 79 other objects
+NOTICE: drop cascades to 80 other objects
DETAIL: drop cascades to table t1
drop cascades to view temporal1
drop cascades to view temporal2
@@ -2275,6 +2291,7 @@ drop cascades to view mysecview9
drop cascades to view unspecified_types
drop cascades to table tt1
drop cascades to table tx1
+drop cascades to view aliased_order_by
drop cascades to view view_of_joins
drop cascades to table tbl1a
drop cascades to view view_of_joins_2a
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 3a78be1b0c..7030744881 100644
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -373,6 +373,14 @@ ALTER TABLE tmp1 RENAME TO tx1;
\d+ aliased_view_3
\d+ aliased_view_4
+-- Test correct deparsing of ORDER BY when there is a name conflict
+
+create view aliased_order_by as
+select x1 as x2, x2 as x1 from tt1
+ order by x2;
+
+\d+ aliased_order_by
+
-- Test aliasing of joins
create view view_of_joins as
I wrote:
> Yeah, I was uncertain whether to bother with that detail. It's true
> that there's no bug in GROUP BY because the parser prefers SQL99
> semantics in that case, but maybe we shouldn't have this code assuming
> that? And it wasn't easy to do with the special_exprkind approach.
> But it is pretty easy with the two-bools approach, so I fixed that in
> the attached v2.
I was just about to commit v2 when I realized that it's wrong, because
what we need to be checking against is the output column name(s) that
get_target_list() will print, and that's not necessarily tle->resname.
You can in fact make v2 misbehave with ALTER VIEW RENAME COLUMN, as
shown by the added test cases in attached v3.
That complicates matters greatly because it means that we also need
access to the current get_query_def call's resultDesc. To avoid
cluttering things impossibly, I decided that resultDesc ought to be
passed down in the deparse_context not as a separate parameter, and
while doing that it seemed better to do the same with colNamesVisible.
(There is one place that has to save-n-restore colNamesVisible to
make that work, but overall it's less code and clutter.)
It's slightly annoying that there are now three places that know
how get_target_list chooses output column names. I thought about
refactoring to get that down to one place, but couldn't really
find anything that didn't net out as more code and ugliness.
Also, while looking at this I realized that get_select_query_def's
habit of saving-and-restoring windowClause and windowTList is
completely pointless: it has only one caller and that one has
just set up a fresh deparse_context, which won't be used anymore
afterwards. If there were a code path in which that did something,
the lack of save-and-restore logic in get_update_query_def and
friends would likely be a bug. So I just dropped it. I believe
that it's not really necessary to save-n-restore inGroupBy or
varInOrderBy either (rather than just reset them to false).
But those cases aren't quite as clear-cut, and resetting to false
isn't much cheaper than restoring a saved value, so I left those
bits as-is.
regards, tom lane
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 00eda1b34c..b31be31321 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -50,7 +50,6 @@
#include "optimizer/optimizer.h"
#include "parser/parse_agg.h"
#include "parser/parse_func.h"
-#include "parser/parse_node.h"
#include "parser/parse_oper.h"
#include "parser/parse_relation.h"
#include "parser/parser.h"
@@ -114,14 +113,16 @@ typedef struct
{
StringInfo buf; /* output buffer to append to */
List *namespaces; /* List of deparse_namespace nodes */
+ TupleDesc resultDesc; /* if top level of a view, the view's tupdesc */
+ List *targetList; /* Current query level's SELECT targetlist */
List *windowClause; /* Current query level's WINDOW clause */
- List *windowTList; /* targetlist for resolving WINDOW clause */
int prettyFlags; /* enabling of pretty-print functions */
int wrapColumn; /* max line length, or -1 for no limit */
int indentLevel; /* current indent level for pretty-print */
bool varprefix; /* true to print prefixes on Vars */
- ParseExprKind special_exprkind; /* set only for exprkinds needing special
- * handling */
+ bool colNamesVisible; /* do we care about output column names? */
+ bool inGroupBy; /* deparsing GROUP BY clause? */
+ bool varInOrderBy; /* deparsing simple Var in ORDER BY? */
Bitmapset *appendparents; /* if not null, map child Vars of these relids
* back to the parent rel */
} deparse_context;
@@ -398,27 +399,19 @@ static void get_query_def(Query *query, StringInfo buf, List *parentnamespace,
int prettyFlags, int wrapColumn, int startIndent);
static void get_values_def(List *values_lists, deparse_context *context);
static void get_with_clause(Query *query, deparse_context *context);
-static void get_select_query_def(Query *query, deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible);
-static void get_insert_query_def(Query *query, deparse_context *context,
- bool colNamesVisible);
-static void get_update_query_def(Query *query, deparse_context *context,
- bool colNamesVisible);
+static void get_select_query_def(Query *query, deparse_context *context);
+static void get_insert_query_def(Query *query, deparse_context *context);
+static void get_update_query_def(Query *query, deparse_context *context);
static void get_update_query_targetlist_def(Query *query, List *targetList,
deparse_context *context,
RangeTblEntry *rte);
-static void get_delete_query_def(Query *query, deparse_context *context,
- bool colNamesVisible);
-static void get_merge_query_def(Query *query, deparse_context *context,
- bool colNamesVisible);
+static void get_delete_query_def(Query *query, deparse_context *context);
+static void get_merge_query_def(Query *query, deparse_context *context);
static void get_utility_query_def(Query *query, deparse_context *context);
-static void get_basic_select_query(Query *query, deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible);
-static void get_target_list(List *targetList, deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible);
+static void get_basic_select_query(Query *query, deparse_context *context);
+static void get_target_list(List *targetList, deparse_context *context);
static void get_setop_query(Node *setOp, Query *query,
- deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible);
+ deparse_context *context);
static Node *get_rule_sortgroupclause(Index ref, List *tlist,
bool force_colno,
deparse_context *context);
@@ -515,7 +508,7 @@ static char *generate_qualified_relation_name(Oid relid);
static char *generate_function_name(Oid funcid, int nargs,
List *argnames, Oid *argtypes,
bool has_variadic, bool *use_variadic_p,
- ParseExprKind special_exprkind);
+ bool inGroupBy);
static char *generate_operator_name(Oid operid, Oid arg1, Oid arg2);
static void add_cast_to(StringInfo buf, Oid typid);
static char *generate_qualified_type_name(Oid typid);
@@ -1094,13 +1087,16 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
/* Set up context with one-deep namespace stack */
context.buf = &buf;
context.namespaces = list_make1(&dpns);
+ context.resultDesc = NULL;
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = true;
context.prettyFlags = GET_PRETTY_FLAGS(pretty);
context.wrapColumn = WRAP_COLUMN_DEFAULT;
context.indentLevel = PRETTYINDENT_STD;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.colNamesVisible = true;
+ context.inGroupBy = false;
+ context.varInOrderBy = false;
context.appendparents = NULL;
get_rule_expr(qual, &context, false);
@@ -1111,7 +1107,7 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
appendStringInfo(&buf, "EXECUTE FUNCTION %s(",
generate_function_name(trigrec->tgfoid, 0,
NIL, NULL,
- false, NULL, EXPR_KIND_NONE));
+ false, NULL, false));
if (trigrec->tgnargs > 0)
{
@@ -2992,7 +2988,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
appendStringInfo(&buf, " SUPPORT %s",
generate_function_name(proc->prosupport, 1,
NIL, argtypes,
- false, NULL, EXPR_KIND_NONE));
+ false, NULL, false));
}
if (oldlen != buf.len)
@@ -3632,13 +3628,16 @@ deparse_expression_pretty(Node *expr, List *dpcontext,
initStringInfo(&buf);
context.buf = &buf;
context.namespaces = dpcontext;
+ context.resultDesc = NULL;
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = forceprefix;
context.prettyFlags = prettyFlags;
context.wrapColumn = WRAP_COLUMN_DEFAULT;
context.indentLevel = startIndent;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.colNamesVisible = true;
+ context.inGroupBy = false;
+ context.varInOrderBy = false;
context.appendparents = NULL;
get_rule_expr(expr, &context, showimplicit);
@@ -5283,13 +5282,16 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
context.buf = buf;
context.namespaces = list_make1(&dpns);
+ context.resultDesc = NULL;
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = (list_length(query->rtable) != 1);
context.prettyFlags = prettyFlags;
context.wrapColumn = WRAP_COLUMN_DEFAULT;
context.indentLevel = PRETTYINDENT_STD;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.colNamesVisible = true;
+ context.inGroupBy = false;
+ context.varInOrderBy = false;
context.appendparents = NULL;
set_deparse_for_query(&dpns, query, NIL);
@@ -5451,14 +5453,17 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
context.buf = buf;
context.namespaces = lcons(&dpns, list_copy(parentnamespace));
+ context.resultDesc = NULL;
+ context.targetList = NIL;
context.windowClause = NIL;
- context.windowTList = NIL;
context.varprefix = (parentnamespace != NIL ||
list_length(query->rtable) != 1);
context.prettyFlags = prettyFlags;
context.wrapColumn = wrapColumn;
context.indentLevel = startIndent;
- context.special_exprkind = EXPR_KIND_NONE;
+ context.colNamesVisible = colNamesVisible;
+ context.inGroupBy = false;
+ context.varInOrderBy = false;
context.appendparents = NULL;
set_deparse_for_query(&dpns, query, parentnamespace);
@@ -5466,23 +5471,25 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
switch (query->commandType)
{
case CMD_SELECT:
- get_select_query_def(query, &context, resultDesc, colNamesVisible);
+ /* We set context.resultDesc only if it's a SELECT */
+ context.resultDesc = resultDesc;
+ get_select_query_def(query, &context);
break;
case CMD_UPDATE:
- get_update_query_def(query, &context, colNamesVisible);
+ get_update_query_def(query, &context);
break;
case CMD_INSERT:
- get_insert_query_def(query, &context, colNamesVisible);
+ get_insert_query_def(query, &context);
break;
case CMD_DELETE:
- get_delete_query_def(query, &context, colNamesVisible);
+ get_delete_query_def(query, &context);
break;
case CMD_MERGE:
- get_merge_query_def(query, &context, colNamesVisible);
+ get_merge_query_def(query, &context);
break;
case CMD_NOTHING:
@@ -5687,23 +5694,18 @@ get_with_clause(Query *query, deparse_context *context)
* ----------
*/
static void
-get_select_query_def(Query *query, deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible)
+get_select_query_def(Query *query, deparse_context *context)
{
StringInfo buf = context->buf;
- List *save_windowclause;
- List *save_windowtlist;
bool force_colno;
ListCell *l;
/* Insert the WITH clause if given */
get_with_clause(query, context);
- /* Set up context for possible window functions */
- save_windowclause = context->windowClause;
+ /* Subroutines may need to consult the SELECT targetlist and windowClause */
+ context->targetList = query->targetList;
context->windowClause = query->windowClause;
- save_windowtlist = context->windowTList;
- context->windowTList = query->targetList;
/*
* If the Query node has a setOperations tree, then it's the top level of
@@ -5712,14 +5714,13 @@ get_select_query_def(Query *query, deparse_context *context,
*/
if (query->setOperations)
{
- get_setop_query(query->setOperations, query, context, resultDesc,
- colNamesVisible);
+ get_setop_query(query->setOperations, query, context);
/* ORDER BY clauses must be simple in this case */
force_colno = true;
}
else
{
- get_basic_select_query(query, context, resultDesc, colNamesVisible);
+ get_basic_select_query(query, context);
force_colno = false;
}
@@ -5808,9 +5809,6 @@ get_select_query_def(Query *query, deparse_context *context,
appendStringInfoString(buf, " SKIP LOCKED");
}
}
-
- context->windowClause = save_windowclause;
- context->windowTList = save_windowtlist;
}
/*
@@ -5888,8 +5886,7 @@ get_simple_values_rte(Query *query, TupleDesc resultDesc)
}
static void
-get_basic_select_query(Query *query, deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible)
+get_basic_select_query(Query *query, deparse_context *context)
{
StringInfo buf = context->buf;
RangeTblEntry *values_rte;
@@ -5907,7 +5904,7 @@ get_basic_select_query(Query *query, deparse_context *context,
* VALUES part. This reverses what transformValuesClause() did at parse
* time.
*/
- values_rte = get_simple_values_rte(query, resultDesc);
+ values_rte = get_simple_values_rte(query, context->resultDesc);
if (values_rte)
{
get_values_def(values_rte->values_lists, context);
@@ -5945,7 +5942,7 @@ get_basic_select_query(Query *query, deparse_context *context,
}
/* Then we tell what to select (the targetlist) */
- get_target_list(query->targetList, context, resultDesc, colNamesVisible);
+ get_target_list(query->targetList, context);
/* Add the FROM clause if needed */
get_from_clause(query, " FROM ", context);
@@ -5961,15 +5958,15 @@ get_basic_select_query(Query *query, deparse_context *context,
/* Add the GROUP BY clause if given */
if (query->groupClause != NULL || query->groupingSets != NULL)
{
- ParseExprKind save_exprkind;
+ bool save_ingroupby;
appendContextKeyword(context, " GROUP BY ",
-PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
if (query->groupDistinct)
appendStringInfoString(buf, "DISTINCT ");
- save_exprkind = context->special_exprkind;
- context->special_exprkind = EXPR_KIND_GROUP_BY;
+ save_ingroupby = context->inGroupBy;
+ context->inGroupBy = true;
if (query->groupingSets == NIL)
{
@@ -5997,7 +5994,7 @@ get_basic_select_query(Query *query, deparse_context *context,
}
}
- context->special_exprkind = save_exprkind;
+ context->inGroupBy = save_ingroupby;
}
/* Add the HAVING clause if given */
@@ -6017,13 +6014,10 @@ get_basic_select_query(Query *query, deparse_context *context,
* get_target_list - Parse back a SELECT target list
*
* This is also used for RETURNING lists in INSERT/UPDATE/DELETE/MERGE.
- *
- * resultDesc and colNamesVisible are as for get_query_def()
* ----------
*/
static void
-get_target_list(List *targetList, deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible)
+get_target_list(List *targetList, deparse_context *context)
{
StringInfo buf = context->buf;
StringInfoData targetbuf;
@@ -6080,7 +6074,7 @@ get_target_list(List *targetList, deparse_context *context,
* assigned column name explicitly. Otherwise, show it only if
* it's not FigureColname's fallback.
*/
- attname = colNamesVisible ? NULL : "?column?";
+ attname = context->colNamesVisible ? NULL : "?column?";
}
/*
@@ -6089,8 +6083,9 @@ get_target_list(List *targetList, deparse_context *context,
* effects of any column RENAME that's been done on the view).
* Otherwise, just use what we can find in the TLE.
*/
- if (resultDesc && colno <= resultDesc->natts)
- colname = NameStr(TupleDescAttr(resultDesc, colno - 1)->attname);
+ if (context->resultDesc && colno <= context->resultDesc->natts)
+ colname = NameStr(TupleDescAttr(context->resultDesc,
+ colno - 1)->attname);
else
colname = tle->resname;
@@ -6158,8 +6153,7 @@ get_target_list(List *targetList, deparse_context *context,
}
static void
-get_setop_query(Node *setOp, Query *query, deparse_context *context,
- TupleDesc resultDesc, bool colNamesVisible)
+get_setop_query(Node *setOp, Query *query, deparse_context *context)
{
StringInfo buf = context->buf;
bool need_paren;
@@ -6184,8 +6178,8 @@ get_setop_query(Node *setOp, Query *query, deparse_context *context,
subquery->limitCount);
if (need_paren)
appendStringInfoChar(buf, '(');
- get_query_def(subquery, buf, context->namespaces, resultDesc,
- colNamesVisible,
+ get_query_def(subquery, buf, context->namespaces,
+ context->resultDesc, context->colNamesVisible,
context->prettyFlags, context->wrapColumn,
context->indentLevel);
if (need_paren)
@@ -6195,6 +6189,7 @@ get_setop_query(Node *setOp, Query *query, deparse_context *context,
{
SetOperationStmt *op = (SetOperationStmt *) setOp;
int subindent;
+ bool save_colnamesvisible;
/*
* We force parens when nesting two SetOperationStmts, except when the
@@ -6228,7 +6223,7 @@ get_setop_query(Node *setOp, Query *query, deparse_context *context,
else
subindent = 0;
- get_setop_query(op->larg, query, context, resultDesc, colNamesVisible);
+ get_setop_query(op->larg, query, context);
if (need_paren)
appendContextKeyword(context, ") ", -subindent, 0, 0);
@@ -6272,7 +6267,15 @@ get_setop_query(Node *setOp, Query *query, deparse_context *context,
subindent = 0;
appendContextKeyword(context, "", subindent, 0, 0);
- get_setop_query(op->rarg, query, context, resultDesc, false);
+ /*
+ * The output column names of the RHS sub-select don't matter.
+ */
+ save_colnamesvisible = context->colNamesVisible;
+ context->colNamesVisible = false;
+
+ get_setop_query(op->rarg, query, context);
+
+ context->colNamesVisible = save_colnamesvisible;
if (PRETTY_INDENT(context))
context->indentLevel -= subindent;
@@ -6306,20 +6309,32 @@ get_rule_sortgroupclause(Index ref, List *tlist, bool force_colno,
* Use column-number form if requested by caller. Otherwise, if
* expression is a constant, force it to be dumped with an explicit cast
* as decoration --- this is because a simple integer constant is
- * ambiguous (and will be misinterpreted by findTargetlistEntry()) if we
- * dump it without any decoration. If it's anything more complex than a
- * simple Var, then force extra parens around it, to ensure it can't be
- * misinterpreted as a cube() or rollup() construct.
+ * ambiguous (and will be misinterpreted by findTargetlistEntrySQL92()) if
+ * we dump it without any decoration. Similarly, if it's just a Var,
+ * there is risk of misinterpretation if the column name is reassigned in
+ * the SELECT list, so we may need to force table qualification. And, if
+ * it's anything more complex than a simple Var, then force extra parens
+ * around it, to ensure it can't be misinterpreted as a cube() or rollup()
+ * construct.
*/
if (force_colno)
{
Assert(!tle->resjunk);
appendStringInfo(buf, "%d", tle->resno);
}
- else if (expr && IsA(expr, Const))
+ else if (!expr)
+ /* do nothing, probably can't happen */ ;
+ else if (IsA(expr, Const))
get_const_expr((Const *) expr, context, 1);
- else if (!expr || IsA(expr, Var))
- get_rule_expr(expr, context, true);
+ else if (IsA(expr, Var))
+ {
+ /* Tell get_variable to check for name conflict */
+ bool save_varinorderby = context->varInOrderBy;
+
+ context->varInOrderBy = true;
+ (void) get_variable((Var *) expr, 0, false, context);
+ context->varInOrderBy = save_varinorderby;
+ }
else
{
/*
@@ -6608,8 +6623,7 @@ get_rule_windowspec(WindowClause *wc, List *targetList,
* ----------
*/
static void
-get_insert_query_def(Query *query, deparse_context *context,
- bool colNamesVisible)
+get_insert_query_def(Query *query, deparse_context *context)
{
StringInfo buf = context->buf;
RangeTblEntry *select_rte = NULL;
@@ -6815,7 +6829,7 @@ get_insert_query_def(Query *query, deparse_context *context,
{
appendContextKeyword(context, " RETURNING",
-PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
- get_target_list(query->returningList, context, NULL, colNamesVisible);
+ get_target_list(query->returningList, context);
}
}
@@ -6825,8 +6839,7 @@ get_insert_query_def(Query *query, deparse_context *context,
* ----------
*/
static void
-get_update_query_def(Query *query, deparse_context *context,
- bool colNamesVisible)
+get_update_query_def(Query *query, deparse_context *context)
{
StringInfo buf = context->buf;
RangeTblEntry *rte;
@@ -6872,7 +6885,7 @@ get_update_query_def(Query *query, deparse_context *context,
{
appendContextKeyword(context, " RETURNING",
-PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
- get_target_list(query->returningList, context, NULL, colNamesVisible);
+ get_target_list(query->returningList, context);
}
}
@@ -7034,8 +7047,7 @@ get_update_query_targetlist_def(Query *query, List *targetList,
* ----------
*/
static void
-get_delete_query_def(Query *query, deparse_context *context,
- bool colNamesVisible)
+get_delete_query_def(Query *query, deparse_context *context)
{
StringInfo buf = context->buf;
RangeTblEntry *rte;
@@ -7076,7 +7088,7 @@ get_delete_query_def(Query *query, deparse_context *context,
{
appendContextKeyword(context, " RETURNING",
-PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
- get_target_list(query->returningList, context, NULL, colNamesVisible);
+ get_target_list(query->returningList, context);
}
}
@@ -7086,8 +7098,7 @@ get_delete_query_def(Query *query, deparse_context *context,
* ----------
*/
static void
-get_merge_query_def(Query *query, deparse_context *context,
- bool colNamesVisible)
+get_merge_query_def(Query *query, deparse_context *context)
{
StringInfo buf = context->buf;
RangeTblEntry *rte;
@@ -7240,7 +7251,7 @@ get_merge_query_def(Query *query, deparse_context *context,
{
appendContextKeyword(context, " RETURNING",
-PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
- get_target_list(query->returningList, context, NULL, colNamesVisible);
+ get_target_list(query->returningList, context);
}
}
@@ -7307,6 +7318,7 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
deparse_columns *colinfo;
char *refname;
char *attname;
+ bool need_prefix;
/* Find appropriate nesting depth */
netlevelsup = var->varlevelsup + levelsup;
@@ -7502,7 +7514,45 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
attname = get_rte_attribute_name(rte, attnum);
}
- if (refname && (context->varprefix || attname == NULL))
+ need_prefix = (context->varprefix || attname == NULL);
+
+ /*
+ * If we're considering a plain Var in an ORDER BY (but not GROUP BY)
+ * clause, we may need to add a table-name prefix to prevent
+ * findTargetlistEntrySQL92 from misinterpreting the name as an
+ * output-column name. To avoid cluttering the output with unnecessary
+ * prefixes, do so only if there is a name match to a SELECT tlist item
+ * that is different from the Var.
+ */
+ if (context->varInOrderBy && !context->inGroupBy && !need_prefix)
+ {
+ int colno = 0;
+
+ foreach_node(TargetEntry, tle, context->targetList)
+ {
+ char *colname;
+
+ if (tle->resjunk)
+ continue; /* ignore junk entries */
+ colno++;
+
+ /* This must match colname-choosing logic in get_target_list() */
+ if (context->resultDesc && colno <= context->resultDesc->natts)
+ colname = NameStr(TupleDescAttr(context->resultDesc,
+ colno - 1)->attname);
+ else
+ colname = tle->resname;
+
+ if (colname && strcmp(colname, attname) == 0 &&
+ !equal(var, tle->expr))
+ {
+ need_prefix = true;
+ break;
+ }
+ }
+ }
+
+ if (refname && need_prefix)
{
appendStringInfoString(buf, quote_identifier(refname));
appendStringInfoChar(buf, '.');
@@ -10463,7 +10513,7 @@ get_func_expr(FuncExpr *expr, deparse_context *context,
argnames, argtypes,
expr->funcvariadic,
&use_variadic,
- context->special_exprkind));
+ context->inGroupBy));
nargs = 0;
foreach(l, expr->args)
{
@@ -10533,7 +10583,7 @@ get_agg_expr_helper(Aggref *aggref, deparse_context *context,
funcname = generate_function_name(aggref->aggfnoid, nargs, NIL,
argtypes, aggref->aggvariadic,
&use_variadic,
- context->special_exprkind);
+ context->inGroupBy);
/* Print the aggregate name, schema-qualified if needed */
appendStringInfo(buf, "%s(%s", funcname,
@@ -10674,7 +10724,7 @@ get_windowfunc_expr_helper(WindowFunc *wfunc, deparse_context *context,
if (!funcname)
funcname = generate_function_name(wfunc->winfnoid, nargs, argnames,
argtypes, false, NULL,
- context->special_exprkind);
+ context->inGroupBy);
appendStringInfo(buf, "%s(", funcname);
@@ -10713,7 +10763,7 @@ get_windowfunc_expr_helper(WindowFunc *wfunc, deparse_context *context,
if (wc->name)
appendStringInfoString(buf, quote_identifier(wc->name));
else
- get_rule_windowspec(wc, context->windowTList, context);
+ get_rule_windowspec(wc, context->targetList, context);
break;
}
}
@@ -12420,7 +12470,7 @@ get_tablesample_def(TableSampleClause *tablesample, deparse_context *context)
appendStringInfo(buf, " TABLESAMPLE %s (",
generate_function_name(tablesample->tsmhandler, 1,
NIL, argtypes,
- false, NULL, EXPR_KIND_NONE));
+ false, NULL, false));
nargs = 0;
foreach(l, tablesample->args)
@@ -12840,12 +12890,14 @@ generate_qualified_relation_name(Oid relid)
* the output. For non-FuncExpr cases, has_variadic should be false and
* use_variadic_p can be NULL.
*
+ * inGroupBy must be true if we're deparsing a GROUP BY clause.
+ *
* The result includes all necessary quoting and schema-prefixing.
*/
static char *
generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
bool has_variadic, bool *use_variadic_p,
- ParseExprKind special_exprkind)
+ bool inGroupBy)
{
char *result;
HeapTuple proctup;
@@ -12870,9 +12922,9 @@ generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
/*
* Due to parser hacks to avoid needing to reserve CUBE, we need to force
- * qualification in some special cases.
+ * qualification of some function names within GROUP BY.
*/
- if (special_exprkind == EXPR_KIND_GROUP_BY)
+ if (inGroupBy)
{
if (strcmp(proname, "cube") == 0 || strcmp(proname, "rollup") == 0)
force_qualify = true;
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index f3f8c7b5a2..f551624afb 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -824,6 +824,54 @@ View definition:
FROM temp_view_test.tx1 tx1_1
WHERE tx1.y1 = tx1_1.f1));
+-- Test correct deparsing of ORDER BY when there is an output name conflict
+create view aliased_order_by as
+select x1 as x2, x2 as x1, x3 from tt1
+ order by x2; -- this is interpreted per SQL92, so really ordering by x1
+\d+ aliased_order_by
+ View "testviewschm2.aliased_order_by"
+ Column | Type | Collation | Nullable | Default | Storage | Description
+--------+---------+-----------+----------+---------+----------+-------------
+ x2 | integer | | | | plain |
+ x1 | integer | | | | plain |
+ x3 | text | | | | extended |
+View definition:
+ SELECT x1 AS x2,
+ x2 AS x1,
+ x3
+ FROM tt1
+ ORDER BY tt1.x1;
+
+alter view aliased_order_by rename column x1 to x0;
+\d+ aliased_order_by
+ View "testviewschm2.aliased_order_by"
+ Column | Type | Collation | Nullable | Default | Storage | Description
+--------+---------+-----------+----------+---------+----------+-------------
+ x2 | integer | | | | plain |
+ x0 | integer | | | | plain |
+ x3 | text | | | | extended |
+View definition:
+ SELECT x1 AS x2,
+ x2 AS x0,
+ x3
+ FROM tt1
+ ORDER BY x1;
+
+alter view aliased_order_by rename column x3 to x1;
+\d+ aliased_order_by
+ View "testviewschm2.aliased_order_by"
+ Column | Type | Collation | Nullable | Default | Storage | Description
+--------+---------+-----------+----------+---------+----------+-------------
+ x2 | integer | | | | plain |
+ x0 | integer | | | | plain |
+ x1 | text | | | | extended |
+View definition:
+ SELECT x1 AS x2,
+ x2 AS x0,
+ x3 AS x1
+ FROM tt1
+ ORDER BY tt1.x1;
+
-- Test aliasing of joins
create view view_of_joins as
select * from
@@ -2248,7 +2296,7 @@ drop cascades to view aliased_view_2
drop cascades to view aliased_view_3
drop cascades to view aliased_view_4
DROP SCHEMA testviewschm2 CASCADE;
-NOTICE: drop cascades to 79 other objects
+NOTICE: drop cascades to 80 other objects
DETAIL: drop cascades to table t1
drop cascades to view temporal1
drop cascades to view temporal2
@@ -2275,6 +2323,7 @@ drop cascades to view mysecview9
drop cascades to view unspecified_types
drop cascades to table tt1
drop cascades to table tx1
+drop cascades to view aliased_order_by
drop cascades to view view_of_joins
drop cascades to table tbl1a
drop cascades to view view_of_joins_2a
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 3a78be1b0c..ae6841308b 100644
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -373,6 +373,22 @@ ALTER TABLE tmp1 RENAME TO tx1;
\d+ aliased_view_3
\d+ aliased_view_4
+-- Test correct deparsing of ORDER BY when there is an output name conflict
+
+create view aliased_order_by as
+select x1 as x2, x2 as x1, x3 from tt1
+ order by x2; -- this is interpreted per SQL92, so really ordering by x1
+
+\d+ aliased_order_by
+
+alter view aliased_order_by rename column x1 to x0;
+
+\d+ aliased_order_by
+
+alter view aliased_order_by rename column x3 to x1;
+
+\d+ aliased_order_by
+
-- Test aliasing of joins
create view view_of_joins as
On Thu, Aug 29, 2024 at 3:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I was just about to commit v2 when I realized that it's wrong, because > what we need to be checking against is the output column name(s) that > get_target_list() will print, and that's not necessarily tle->resname. > You can in fact make v2 misbehave with ALTER VIEW RENAME COLUMN, as > shown by the added test cases in attached v3. Ah yes, we need to match the column name from the view's tuple descriptor, which may differ from tle->resname after column RENAME. +1 to v3. Thanks Richard
Richard Guo <guofenglinux@gmail.com> writes:
> +1 to v3.
Pushed, thanks for looking at it.
regards, tom lane