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

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