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

The following bug has been logged on the website:

Bug reference:      12789
Logged by:          Curtis McEnroe
Email address:      programble@gmail.com
PostgreSQL version: 9.4.1
Operating system:   Mac OS X 10.10.1
Description:

A view created with explicit column names and defined as a VALUES statement
will lose its column names when dumped. When the dump is restored, the
view's columns are named "column1", "column2", etc.

I wrote a short repro script here:
https://gist.github.com/programble/a416df496bfb41259c86

>>>>> ">" == programble  <programble@gmail.com> writes:

 >> The following bug has been logged on the website:
 >> Bug reference:      12789
 >> Logged by:          Curtis McEnroe
 >> Email address:      programble@gmail.com
 >> PostgreSQL version: 9.4.1
 >> Operating system:   Mac OS X 10.10.1
 >> Description:

 >> A view created with explicit column names and defined as a VALUES statement
 >> will lose its column names when dumped. When the dump is restored, the
 >> view's columns are named "column1", "column2", etc.

 >> I wrote a short repro script here:
 >> https://gist.github.com/programble/a416df496bfb41259c86

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.

--
Andrew (irc:RhodiumToad)

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;