Re: BUG #17486: [pg_restore] Restoring a view fails if this view contains an attribute without alias name. - Mailing list pgsql-bugs

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

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.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 ruletup, TupleDesc rulettc,
 static void make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
                          int prettyFlags, int wrapColumn);
 static void get_query_def(Query *query, StringInfo buf, List *parentnamespace,
-                          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 *targetList,
@@ -409,12 +409,12 @@ static void get_update_query_targetlist_def(Query *query, 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 proctup)

             /* 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 proctup)

         /* 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, TupleDesc rulettc,
         foreach(action, actions)
         {
             query = (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, TupleDesc rulettc,
         Query       *query;

         query = (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, TupleDesc rulettc,

     ev_relation = 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, TupleDesc 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 *parentnamespace,
     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 *context)
         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 *context)
  */
 static void
 get_select_query_def(Query *query, deparse_context *context,
-                     TupleDesc resultDesc)
+                     TupleDesc resultDesc, bool colNamesVisible)
 {
     StringInfo    buf = 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 = true;
     }
     else
     {
-        get_basic_select_query(query, context, resultDesc);
+        get_basic_select_query(query, context, resultDesc, colNamesVisible);
         force_colno = false;
     }

@@ -5859,7 +5862,7 @@ get_simple_values_rte(Query *query, TupleDesc resultDesc)

 static void
 get_basic_select_query(Query *query, deparse_context *context,
-                       TupleDesc resultDesc)
+                       TupleDesc resultDesc, bool colNamesVisible)
 {
     StringInfo    buf = 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_context *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 = 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 = "?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 = colNamesVisible ? NULL : "?column?";
         }

         /*
@@ -6122,7 +6132,7 @@ get_target_list(List *targetList, deparse_context *context,

 static void
 get_setop_query(Node *setOp, Query *query, deparse_context *context,
-                TupleDesc resultDesc)
+                TupleDesc resultDesc, bool colNamesVisible)
 {
     StringInfo    buf = context->buf;
     bool        need_paren;
@@ -6148,6 +6158,7 @@ get_setop_query(Node *setOp, Query *query, deparse_context *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_context *context,
         else
             subindent = 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_context *context,
             subindent = 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 -= 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, deparse_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/expected/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 mvtest_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,