Issues with dropped columns in views depending on functions

I looked into the problem described here:
http://www.postgresql.org/message-id/53C93986.80904@clickware.de

The core issue is that the ruleutils.c code isn't thinking about dropped
columns in the output of a function-in-FROM that returns a composite type.
Pre-9.3, there had been some code there to cope with the case by means of
doing a get_rte_attribute_is_dropped() test on each column as the alias
list was printed, but I'd lost that in the rewrite that improved handling
of column aliases in RTEs with dropped columns.  I looked at putting that
back, but (a) it's slow, and (b) it didn't really work right even when it
was there: it only accidentally failed to crash in EXPLAIN's usage,
because we were applying exprType() to a null funcexpr field.

The next possibility that I looked at was teaching the code to ignore
empty-string items in a function RTE's alias list, since that's what
the parser inserts to correspond to dropped columns at parse time.
This solves the reported problem, almost, except that it exposed an
ancient bug in outfuncs/readfuncs: the code to print a T_String node is
wrong for the case of an empty string.  What it actually prints is "<>"
which of course reads back as <>, not empty.  After repairing that, I had
a patch that takes care of the reported problem, at least for newly
created views (see attached).  It won't do anything to retroactively fix
the problem in existing views, but I think that might be okay given the
small number of complaints.

However, this only fixes the issue for columns that were dropped before
the view was created.  The system will let you drop columns *after* the
view is created, and then we have a problem, as illustrated by the
rather dubious results in the regression test in the attached patch.
I'm of the opinion that this is a bug and we should disallow dropping
a composite type's column if it's explicitly referenced in any view, much
as would happen if there were a direct reference to the table.  However,
that will take some nontrivial surgery on the dependency code, and I
rather doubt that we want to back-patch such a thing --- especially since
the needed pg_depend entries wouldn't be there anyway for existing views
in existing databases.

What I propose to do is apply the attached back through 9.3, so that at
least users are no worse off than they were pre-9.3.  I will look at
fixing the problem of a post-view-creation column drop later, but I
suspect we'll end up not back-patching those changes.

            regards, tom lane

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 9573a9b..0631bc2 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*************** _outValue(StringInfo str, const Value *v
*** 2506,2512 ****
              break;
          case T_String:
              appendStringInfoChar(str, '"');
!             _outToken(str, value->val.str);
              appendStringInfoChar(str, '"');
              break;
          case T_BitString:
--- 2506,2513 ----
              break;
          case T_String:
              appendStringInfoChar(str, '"');
!             if (value->val.str[0] != '\0')
!                 _outToken(str, value->val.str);
              appendStringInfoChar(str, '"');
              break;
          case T_BitString:
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 0781ac8..7237e5d 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** set_relation_column_names(deparse_namesp
*** 3086,3092 ****
          i = 0;
          foreach(lc, rte->eref->colnames)
          {
!             real_colnames[i] = strVal(lfirst(lc));
              i++;
          }
      }
--- 3086,3101 ----
          i = 0;
          foreach(lc, rte->eref->colnames)
          {
!             /*
!              * If the column name shown in eref is an empty string, then it's
!              * a column that was dropped at the time of parsing the query, so
!              * treat it as dropped.
!              */
!             char       *cname = strVal(lfirst(lc));
!
!             if (cname[0] == '\0')
!                 cname = NULL;
!             real_colnames[i] = cname;
              i++;
          }
      }
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index 06b2037..e2d4276 100644
*** a/src/test/regress/expected/create_view.out
--- b/src/test/regress/expected/create_view.out
*************** select pg_get_viewdef('vv6', true);
*** 1332,1337 ****
--- 1332,1389 ----
        JOIN tt13 USING (z);
  (1 row)

+ --
+ -- Check some cases involving dropped columns in a function's rowtype result
+ --
+ create table tt14t (f1 text, f2 text, f3 text, f4 text);
+ insert into tt14t values('foo', 'bar', 'baz', 'quux');
+ alter table tt14t drop column f2;
+ create function tt14f() returns setof tt14t as
+ $$
+ declare
+     rec1 record;
+ begin
+     for rec1 in select * from tt14t
+     loop
+         return next rec1;
+     end loop;
+ end;
+ $$
+ language plpgsql;
+ create view tt14v as select t.* from tt14f() t;
+ select pg_get_viewdef('tt14v', true);
+          pg_get_viewdef
+ --------------------------------
+   SELECT t.f1,                 +
+      t.f3,                     +
+      t.f4                      +
+     FROM tt14f() t(f1, f3, f4);
+ (1 row)
+
+ select * from tt14v;
+  f1  | f3  |  f4
+ -----+-----+------
+  foo | baz | quux
+ (1 row)
+
+ -- this perhaps should be rejected, but it isn't:
+ alter table tt14t drop column f3;
+ -- f3 is still in the view but will read as nulls
+ select pg_get_viewdef('tt14v', true);
+          pg_get_viewdef
+ --------------------------------
+   SELECT t.f1,                 +
+      t.f3,                     +
+      t.f4                      +
+     FROM tt14f() t(f1, f3, f4);
+ (1 row)
+
+ select * from tt14v;
+  f1  | f3 |  f4
+ -----+----+------
+  foo |    | quux
+ (1 row)
+
  -- clean up all the random objects we made above
  set client_min_messages = warning;
  DROP SCHEMA temp_view_test CASCADE;
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index e09bc1a..d3b3f12 100644
*** a/src/test/regress/sql/create_view.sql
--- b/src/test/regress/sql/create_view.sql
*************** alter table tt11 add column z int;
*** 435,440 ****
--- 435,474 ----

  select pg_get_viewdef('vv6', true);

+ --
+ -- Check some cases involving dropped columns in a function's rowtype result
+ --
+
+ create table tt14t (f1 text, f2 text, f3 text, f4 text);
+ insert into tt14t values('foo', 'bar', 'baz', 'quux');
+
+ alter table tt14t drop column f2;
+
+ create function tt14f() returns setof tt14t as
+ $$
+ declare
+     rec1 record;
+ begin
+     for rec1 in select * from tt14t
+     loop
+         return next rec1;
+     end loop;
+ end;
+ $$
+ language plpgsql;
+
+ create view tt14v as select t.* from tt14f() t;
+
+ select pg_get_viewdef('tt14v', true);
+ select * from tt14v;
+
+ -- this perhaps should be rejected, but it isn't:
+ alter table tt14t drop column f3;
+
+ -- f3 is still in the view but will read as nulls
+ select pg_get_viewdef('tt14v', true);
+ select * from tt14v;
+
  -- clean up all the random objects we made above
  set client_min_messages = warning;
  DROP SCHEMA temp_view_test CASCADE;