Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ns64v-0001Lk-JF for pgsql-bugs@arkaria.postgresql.org; Fri, 20 May 2022 17:06:17 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ns64i-0007Rd-Oi for pgsql-bugs@arkaria.postgresql.org; Fri, 20 May 2022 17:06:04 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ns64i-0007RT-7U for pgsql-bugs@lists.postgresql.org; Fri, 20 May 2022 17:06:04 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ns64e-0004fW-IA for pgsql-bugs@lists.postgresql.org; Fri, 20 May 2022 17:06:03 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 24KH5wtP428378; Fri, 20 May 2022 13:05:59 -0400 From: Tom Lane To: Daniel Gustafsson cc: n.lutic@loxodata.com, pgsql-bugs@lists.postgresql.org Subject: Re: BUG #17486: [pg_restore] Restoring a view fails if this view contains an attribute without alias name. In-reply-to: <347193.1653059973@sss.pgh.pa.us> References: <17486-1ad6fd786728b8af@postgresql.org> <792E8C73-EBE0-41FC-A83F-2F7A7AF4ABBB@yesql.se> <334334.1653057260@sss.pgh.pa.us> <310D60C6-5CF0-41C8-A814-3A3DE7ACF551@yesql.se> <347193.1653059973@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Fri, 20 May 2022 11:19:33 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <428349.1653066327.0@sss.pgh.pa.us> Date: Fri, 20 May 2022 13:05:58 -0400 Message-ID: <428377.1653066358@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <428349.1653066327.1@sss.pgh.pa.us> I wrote: > Hmm ... it's a very easy code change, but it results in a lot of > changes in the regression tests (and I've only tried the core tests > so far). Given the lack of prior complaints, I wonder if it's going > to be worth this much behavioral churn. > It'd be better if we could do this only when the name is actually > referenced somewhere, but I don't think that's an easy thing to > determine. I thought of a compromise position that's not hard to implement: change the behavior only if the SELECT output column name is *possibly* visible elsewhere, which it is not in (for example) an EXISTS subquery. This is easy to keep track of while descending the parse tree. The attached quick-hack draft results in only one place changing in the regression tests, and that's a place where a view's visible column name is already "?column?", so whoever wrote that test case didn't give a fig for prettiness anyway. This seems like it might be an acceptable amount of behavioral churn. Thoughts? regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="display-column-name-when-needed-wip.patch"; charset="us-ascii" Content-ID: <428349.1653066327.2@sss.pgh.pa.us> Content-Description: display-column-name-when-needed-wip.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rul= eutils.c index 49c4201dde..d8b54e1361 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -395,12 +395,12 @@ static void make_ruledef(StringInfo buf, HeapTuple r= uletup, TupleDesc rulettc, static void make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rul= ettc, int prettyFlags, int wrapColumn); static void get_query_def(Query *query, StringInfo buf, List *parentnames= pace, - TupleDesc resultDesc, + TupleDesc resultDesc, bool colNamesVisible, 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); + TupleDesc resultDesc, bool colNamesVisible); 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 *targetLis= t, @@ -409,12 +409,12 @@ static void get_update_query_targetlist_def(Query *q= uery, List *targetList, static void get_delete_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); + TupleDesc resultDesc, bool colNamesVisible); static void get_target_list(List *targetList, deparse_context *context, - TupleDesc resultDesc); + TupleDesc resultDesc, bool colNamesVisible); static void get_setop_query(Node *setOp, Query *query, deparse_context *context, - TupleDesc resultDesc); + TupleDesc resultDesc, bool colNamesVisible); static Node *get_rule_sortgroupclause(Index ref, List *tlist, bool force_colno, deparse_context *context); @@ -1544,7 +1544,8 @@ pg_get_querydef(Query *query, bool pretty) = initStringInfo(&buf); = - get_query_def(query, &buf, NIL, NULL, prettyFlags, WRAP_COLUMN_DEFAULT, = 0); + get_query_def(query, &buf, NIL, NULL, true, + prettyFlags, WRAP_COLUMN_DEFAULT, 0); = return buf.data; } @@ -3548,7 +3549,7 @@ print_function_sqlbody(StringInfo buf, HeapTuple pro= ctup) = /* It seems advisable to get at least AccessShareLock on rels */ AcquireRewriteLocks(query, false, false); - get_query_def(query, buf, list_make1(&dpns), NULL, + get_query_def(query, buf, list_make1(&dpns), NULL, false, PRETTYFLAG_INDENT, WRAP_COLUMN_DEFAULT, 1); appendStringInfoChar(buf, ';'); appendStringInfoChar(buf, '\n'); @@ -3562,7 +3563,7 @@ print_function_sqlbody(StringInfo buf, HeapTuple pro= ctup) = /* It seems advisable to get at least AccessShareLock on rels */ AcquireRewriteLocks(query, false, false); - get_query_def(query, buf, list_make1(&dpns), NULL, + get_query_def(query, buf, list_make1(&dpns), NULL, false, 0, WRAP_COLUMN_DEFAULT, 0); } } @@ -5299,7 +5300,7 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, Tupl= eDesc rulettc, foreach(action, actions) { query =3D (Query *) lfirst(action); - get_query_def(query, buf, NIL, viewResultDesc, + get_query_def(query, buf, NIL, viewResultDesc, true, prettyFlags, WRAP_COLUMN_DEFAULT, 0); if (prettyFlags) appendStringInfoString(buf, ";\n"); @@ -5313,7 +5314,7 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, Tupl= eDesc rulettc, Query *query; = query =3D (Query *) linitial(actions); - get_query_def(query, buf, NIL, viewResultDesc, + get_query_def(query, buf, NIL, viewResultDesc, true, prettyFlags, WRAP_COLUMN_DEFAULT, 0); appendStringInfoChar(buf, ';'); } @@ -5387,7 +5388,7 @@ make_viewdef(StringInfo buf, HeapTuple ruletup, Tupl= eDesc rulettc, = ev_relation =3D table_open(ev_class, AccessShareLock); = - get_query_def(query, buf, NIL, RelationGetDescr(ev_relation), + get_query_def(query, buf, NIL, RelationGetDescr(ev_relation), true, prettyFlags, wrapColumn, 0); appendStringInfoChar(buf, ';'); = @@ -5404,7 +5405,7 @@ make_viewdef(StringInfo buf, HeapTuple ruletup, Tupl= eDesc rulettc, */ static void get_query_def(Query *query, StringInfo buf, List *parentnamespace, - TupleDesc resultDesc, + TupleDesc resultDesc, bool colNamesVisible, int prettyFlags, int wrapColumn, int startIndent) { deparse_context context; @@ -5442,7 +5443,7 @@ get_query_def(Query *query, StringInfo buf, List *pa= rentnamespace, switch (query->commandType) { case CMD_SELECT: - get_select_query_def(query, &context, resultDesc); + get_select_query_def(query, &context, resultDesc, colNamesVisible); break; = case CMD_UPDATE: @@ -5578,6 +5579,7 @@ get_with_clause(Query *query, deparse_context *conte= xt) if (PRETTY_INDENT(context)) appendContextKeyword(context, "", 0, 0, 0); get_query_def((Query *) cte->ctequery, buf, context->namespaces, NULL, + true, context->prettyFlags, context->wrapColumn, context->indentLevel); if (PRETTY_INDENT(context)) @@ -5659,7 +5661,7 @@ get_with_clause(Query *query, deparse_context *conte= xt) */ static void get_select_query_def(Query *query, deparse_context *context, - TupleDesc resultDesc) + TupleDesc resultDesc, bool colNamesVisible) { StringInfo buf =3D context->buf; List *save_windowclause; @@ -5683,13 +5685,14 @@ get_select_query_def(Query *query, deparse_context= *context, */ if (query->setOperations) { - get_setop_query(query->setOperations, query, context, resultDesc); + get_setop_query(query->setOperations, query, context, resultDesc, + colNamesVisible); /* ORDER BY clauses must be simple in this case */ force_colno =3D true; } else { - get_basic_select_query(query, context, resultDesc); + get_basic_select_query(query, context, resultDesc, colNamesVisible); force_colno =3D false; } = @@ -5859,7 +5862,7 @@ get_simple_values_rte(Query *query, TupleDesc result= Desc) = static void get_basic_select_query(Query *query, deparse_context *context, - TupleDesc resultDesc) + TupleDesc resultDesc, bool colNamesVisible) { StringInfo buf =3D context->buf; RangeTblEntry *values_rte; @@ -5915,7 +5918,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); + get_target_list(query->targetList, context, resultDesc, colNamesVisible)= ; = /* Add the FROM clause if needed */ get_from_clause(query, " FROM ", context); @@ -5987,11 +5990,13 @@ get_basic_select_query(Query *query, deparse_conte= xt *context, * get_target_list - Parse back a SELECT target list * * This is also used for RETURNING lists in INSERT/UPDATE/DELETE. + * + * XXX document arguments better * ---------- */ static void get_target_list(List *targetList, deparse_context *context, - TupleDesc resultDesc) + TupleDesc resultDesc, bool colNamesVisible) { StringInfo buf =3D context->buf; StringInfoData targetbuf; @@ -6042,8 +6047,13 @@ get_target_list(List *targetList, deparse_context *= context, else { get_rule_expr((Node *) tle->expr, context, true); - /* We'll show the AS name unless it's this: */ - attname =3D "?column?"; + + /* + * When colNamesVisible is true, we should always show the + * assigned column name explicitly. Otherwise, show it only if + * it's not FigureColname's fallback. + */ + attname =3D colNamesVisible ? NULL : "?column?"; } = /* @@ -6122,7 +6132,7 @@ get_target_list(List *targetList, deparse_context *c= ontext, = static void get_setop_query(Node *setOp, Query *query, deparse_context *context, - TupleDesc resultDesc) + TupleDesc resultDesc, bool colNamesVisible) { StringInfo buf =3D context->buf; bool need_paren; @@ -6148,6 +6158,7 @@ get_setop_query(Node *setOp, Query *query, deparse_c= ontext *context, if (need_paren) appendStringInfoChar(buf, '('); get_query_def(subquery, buf, context->namespaces, resultDesc, + colNamesVisible, context->prettyFlags, context->wrapColumn, context->indentLevel); if (need_paren) @@ -6190,7 +6201,7 @@ get_setop_query(Node *setOp, Query *query, deparse_c= ontext *context, else subindent =3D 0; = - get_setop_query(op->larg, query, context, resultDesc); + get_setop_query(op->larg, query, context, resultDesc, colNamesVisible); = if (need_paren) appendContextKeyword(context, ") ", -subindent, 0, 0); @@ -6234,7 +6245,7 @@ get_setop_query(Node *setOp, Query *query, deparse_c= ontext *context, subindent =3D 0; appendContextKeyword(context, "", subindent, 0, 0); = - get_setop_query(op->rarg, query, context, resultDesc); + get_setop_query(op->rarg, query, context, resultDesc, false); = if (PRETTY_INDENT(context)) context->indentLevel -=3D subindent; @@ -6680,6 +6691,7 @@ get_insert_query_def(Query *query, deparse_context *= context) { /* Add the SELECT */ get_query_def(select_rte->subquery, buf, context->namespaces, NULL, + false, context->prettyFlags, context->wrapColumn, context->indentLevel); } @@ -6773,7 +6785,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); + get_target_list(query->returningList, context, NULL, true); } } = @@ -6828,7 +6840,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); + get_target_list(query->returningList, context, NULL, true); } } = @@ -7031,7 +7043,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); + get_target_list(query->returningList, context, NULL, true); } } = @@ -11039,7 +11051,7 @@ get_sublink_expr(SubLink *sublink, deparse_context= *context) if (need_paren) appendStringInfoChar(buf, '('); = - get_query_def(query, buf, context->namespaces, NULL, + get_query_def(query, buf, context->namespaces, NULL, false, context->prettyFlags, context->wrapColumn, context->indentLevel); = @@ -11548,6 +11560,7 @@ get_from_clause_item(Node *jtnode, Query *query, d= eparse_context *context) /* Subquery RTE */ appendStringInfoChar(buf, '('); get_query_def(rte->subquery, buf, context->namespaces, NULL, + true, context->prettyFlags, context->wrapColumn, context->indentLevel); appendStringInfoChar(buf, ')'); diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expe= cted/matview.out index 313c72a268..c109d97635 100644 --- a/src/test/regress/expected/matview.out +++ b/src/test/regress/expected/matview.out @@ -347,7 +347,7 @@ CREATE VIEW mvtest_vt2 AS SELECT moo, 2*moo FROM mvtes= t_vt1 UNION ALL SELECT moo ?column? | integer | | | | plain | = View definition: SELECT mvtest_vt1.moo, - 2 * mvtest_vt1.moo + 2 * mvtest_vt1.moo AS "?column?" FROM mvtest_vt1 UNION ALL SELECT mvtest_vt1.moo, @@ -363,7 +363,7 @@ CREATE MATERIALIZED VIEW mv_test2 AS SELECT moo, 2*moo= FROM mvtest_vt2 UNION ALL ?column? | integer | | | | plain | = | = View definition: SELECT mvtest_vt2.moo, - 2 * mvtest_vt2.moo + 2 * mvtest_vt2.moo AS "?column?" FROM mvtest_vt2 UNION ALL SELECT mvtest_vt2.moo, ------- =_aaaaaaaaaa0--