Re: BUG #12789: Views defined with VALUES lose their column names when dumped

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> For future reference and as a simpler testcase than the one in the
> script:

> # create view v1(a) as values (1);
> CREATE VIEW

> # select pg_get_viewdef('v1'::regclass);
>  pg_get_viewdef
> ----------------
>   VALUES (1);

> Notice that the column name "a" is lost. Since pg_dump and so on rely on
> pg_get_viewdef, dump and restore changes the column name back to
> "column1".

> The culprit is obviously in ruleutils.c:
> get_simple_values_rte/get_values_def, which mistakenly thinks it only
> has to deal with the result of transformValuesClause(), not considering
> that the result of transformValuesClause might have been further
> mogrified by DefineView. Treating this case as not being "simple" might
> be one approach... not sure of the best way to detect that.

Yeah --- we can check to see if the tlist resnames match what's in the
RTE.  It turns out that get_from_clause_item() is also buggy: apparently
the RTE_VALUES path through that has never been exercised, or at least
nobody has pointed out to us that it prints bad syntax.  I'm guessing
that up to now, get_simple_values_rte *always* succeeds for situations
involving a VALUES RTE and so we never got there.  The attached seems
to fix it though.

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index c1d860c..2fa30be 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** get_simple_values_rte(Query *query)
*** 4498,4507 ****
      /*
       * We want to return TRUE even if the Query also contains OLD or NEW rule
       * RTEs.  So the idea is to scan the rtable and see if there is only one
!      * inFromCl RTE that is a VALUES RTE.  We don't look at the targetlist at
!      * all.  This is okay because parser/analyze.c will never generate a
!      * "bare" VALUES RTE --- they only appear inside auto-generated
!      * sub-queries with very restricted structure.
       */
      foreach(lc, query->rtable)
      {
--- 4498,4504 ----
      /*
       * We want to return TRUE even if the Query also contains OLD or NEW rule
       * RTEs.  So the idea is to scan the rtable and see if there is only one
!      * inFromCl RTE that is a VALUES RTE.
       */
      foreach(lc, query->rtable)
      {
*************** get_simple_values_rte(Query *query)
*** 4518,4523 ****
--- 4515,4547 ----
          else
              return NULL;        /* something else -> not simple VALUES */
      }
+
+     /*
+      * We don't need to check the targetlist in any great detail, because
+      * parser/analyze.c will never generate a "bare" VALUES RTE --- they only
+      * appear inside auto-generated sub-queries with very restricted
+      * structure.  However, DefineView might have modified the tlist by
+      * injecting new column aliases; so compare tlist resnames against the
+      * RTE's names to detect that.
+      */
+     if (result)
+     {
+         ListCell   *lcn;
+
+         if (list_length(query->targetList) != list_length(result->eref->colnames))
+             return NULL;        /* this probably cannot happen */
+         forboth(lc, query->targetList, lcn, result->eref->colnames)
+         {
+             TargetEntry *tle = (TargetEntry *) lfirst(lc);
+             char       *cname = strVal(lfirst(lcn));
+
+             if (tle->resjunk)
+                 return NULL;    /* this probably cannot happen */
+             if (tle->resname == NULL || strcmp(tle->resname, cname) != 0)
+                 return NULL;    /* column name has been changed */
+         }
+     }
+
      return result;
  }

*************** get_from_clause_item(Node *jtnode, Query
*** 8517,8523 ****
--- 8541,8549 ----
                  break;
              case RTE_VALUES:
                  /* Values list RTE */
+                 appendStringInfoChar(buf, '(');
                  get_values_def(rte->values_lists, context);
+                 appendStringInfoChar(buf, ')');
                  break;
              case RTE_CTE:
                  appendStringInfoString(buf, quote_identifier(rte->ctename));
*************** get_from_clause_item(Node *jtnode, Query
*** 8559,8564 ****
--- 8585,8595 ----
               */
              printalias = true;
          }
+         else if (rte->rtekind == RTE_VALUES)
+         {
+             /* Alias is syntactically required for VALUES */
+             printalias = true;
+         }
          else if (rte->rtekind == RTE_CTE)
          {
              /*
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index d50b103..c3e3f09 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** ALTER RULE "_RETURN" ON rule_v1 RENAME T
*** 2678,2680 ****
--- 2678,2733 ----
  ERROR:  renaming an ON SELECT rule is not allowed
  DROP VIEW rule_v1;
  DROP TABLE rule_t1;
+ --
+ -- check display of VALUES in view definitions
+ --
+ create view rule_v1 as values(1,2);
+ \d+ rule_v1
+                  View "public.rule_v1"
+  Column  |  Type   | Modifiers | Storage | Description
+ ---------+---------+-----------+---------+-------------
+  column1 | integer |           | plain   |
+  column2 | integer |           | plain   |
+ View definition:
+  VALUES (1,2);
+
+ drop view rule_v1;
+ create view rule_v1(x) as values(1,2);
+ \d+ rule_v1
+                  View "public.rule_v1"
+  Column  |  Type   | Modifiers | Storage | Description
+ ---------+---------+-----------+---------+-------------
+  x       | integer |           | plain   |
+  column2 | integer |           | plain   |
+ View definition:
+  SELECT "*VALUES*".column1 AS x,
+     "*VALUES*".column2
+    FROM (VALUES (1,2)) "*VALUES*";
+
+ drop view rule_v1;
+ create view rule_v1(x) as select * from (values(1,2)) v;
+ \d+ rule_v1
+                  View "public.rule_v1"
+  Column  |  Type   | Modifiers | Storage | Description
+ ---------+---------+-----------+---------+-------------
+  x       | integer |           | plain   |
+  column2 | integer |           | plain   |
+ View definition:
+  SELECT v.column1 AS x,
+     v.column2
+    FROM ( VALUES (1,2)) v;
+
+ drop view rule_v1;
+ create view rule_v1(x) as select * from (values(1,2)) v(q,w);
+ \d+ rule_v1
+                 View "public.rule_v1"
+  Column |  Type   | Modifiers | Storage | Description
+ --------+---------+-----------+---------+-------------
+  x      | integer |           | plain   |
+  w      | integer |           | plain   |
+ View definition:
+  SELECT v.q AS x,
+     v.w
+    FROM ( VALUES (1,2)) v(q, w);
+
+ drop view rule_v1;
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 1e15f84..4b3e65e 100644
*** a/src/test/regress/sql/rules.sql
--- b/src/test/regress/sql/rules.sql
*************** ALTER RULE "_RETURN" ON rule_v1 RENAME T
*** 1007,1009 ****
--- 1007,1025 ----

  DROP VIEW rule_v1;
  DROP TABLE rule_t1;
+
+ --
+ -- check display of VALUES in view definitions
+ --
+ create view rule_v1 as values(1,2);
+ \d+ rule_v1
+ drop view rule_v1;
+ create view rule_v1(x) as values(1,2);
+ \d+ rule_v1
+ drop view rule_v1;
+ create view rule_v1(x) as select * from (values(1,2)) v;
+ \d+ rule_v1
+ drop view rule_v1;
+ create view rule_v1(x) as select * from (values(1,2)) v(q,w);
+ \d+ rule_v1
+ drop view rule_v1;