From a7eb633563c6ba8857cf59c7b43b593614537617 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 29 Aug 2024 13:24:17 -0400 Subject: [PATCH] Fix mis-deparsing of ORDER BY lists when there is a name conflict. 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 | 250 +++++++++++++--------- src/test/regress/expected/create_view.out | 51 ++++- src/test/regress/sql/create_view.sql | 16 ++ 3 files changed, 217 insertions(+), 100 deletions(-) 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