Fix mis-deparsing of ORDER BY lists when there is a name conflict.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 29 Aug 2024 17:24:17 +0000 (13:24 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 29 Aug 2024 17:24:17 +0000 (13:24 -0400)
If an ORDER BY item in SELECT is a bare identifier, the parser
first seeks it as an output column name of the SELECT (for SQL92
compatibility).  However, ruleutils.c is expecting the SQL99
interpretation where such a name is an input column name.  So it's
possible to produce an incorrect display of a view in the (admittedly
pretty ill-advised) case where some other column is renamed in the
SELECT output list to match an ORDER BY column.

This can be fixed by table-qualifying such names in the dumped
view text.  To avoid cluttering less-ill-advised queries, we'd
like to do so only when there's an actual name conflict.
That requires passing the current get_query_def call's resultDesc
parameter down to get_variable, so that it can determine what
the output column names are.  In hopes of reducing rather than
increasing notational clutter in ruleutils.c, I moved that value
into the deparse_context struct and removed it from the parameter
lists of get_query_def's other subroutines.

I made a few other cosmetic changes while at it:
* Likewise move the colNamesVisible parameter into deparse_context.
* Rename deparse_context's windowTList field to targetList,
since it's no longer used only in connection with WINDOW clauses.
* Replace the special_exprkind field with a bool inGroupBy,
since that was all it was being used for, and the apparent
flexibility of storing a ParseExprKind proved to be illusory.
(We need a separate varInOrderBy field to make this patch work.)
* Remove useless save/restore logic in get_select_query_def.

In principle, this bug is quite old.  However, it seems unreachable
before 1b4d280ea, because before that the presence of "new" and "old"
entries in a view's rangetable caused us to always table-qualify every
Var reference in dumped views.  Hence, back-patch to v16 where that
came in.

Per bug #18589 from Quynh Tran.

Discussion: https://postgr.es/m/18589-70091cb81db1a3f1@postgresql.org

src/backend/utils/adt/ruleutils.c
src/test/regress/expected/create_view.out
src/test/regress/sql/create_view.sql

index 00eda1b34c0a9106d74785045c41d7f511aecf09..b31be31321dc3c6bbc36a5be8e8495e835c4cea3 100644 (file)
@@ -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;
index f3f8c7b5a2fa0f52ccefd4d0d276461e6769b356..f551624afb3a3cd2b66955910bc1983123dac5ad 100644 (file)
@@ -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
index 3a78be1b0c7c486b60fe82432a87cd539adafbd2..ae6841308b9b6149999f5aa7605a4dc054f90972 100644 (file)
@@ -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