Re: BUG #18589: pg_get_viewdef returns wrong query - Mailing list pgsql-bugs

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