Partial fix for dropped columns in functions returning composite.

When a view has a function-returning-composite in FROM, and there are
some dropped columns in the underlying composite type, ruleutils.c
printed junk in the column alias list for the reconstructed FROM entry.
Before 9.3, this was prevented by doing get_rte_attribute_is_dropped
tests while printing the column alias list; but that solution is not
currently available to us for reasons I'll explain below.  Instead,
check for empty-string entries in the alias list, which can only exist
if that column position had been dropped at the time the view was made.
(The parser fills in empty strings to preserve the invariant that the
aliases correspond to physical column positions.)

While this is sufficient to handle the case of columns dropped before
the view was made, we have still got issues with columns dropped after
the view was made.  In particular, the view could contain Vars that
explicitly reference such columns!  The dependency machinery really
ought to refuse the column drop attempt in such cases, as it would do
when trying to drop a table column that's explicitly referenced in
views.  However, we currently neglect to store dependencies on columns
of composite types, and fixing that is likely to be too big to be
back-patchable (not to mention that existing views in existing databases
would not have the needed pg_depend entries anyway).  So I'll leave that
for a separate patch.

Pre-9.3, ruleutils would print such Vars normally (with their original
column names) even though it suppressed their entries in the RTE's
column alias list.  This is certainly bogus, since the printed view
definition would fail to reload, but at least it didn't crash.  However,
as of 9.3 the printed column alias list is tightly tied to the names
printed for Vars; so we can't treat columns as dropped for one purpose
and not dropped for the other.  This is why we can't just put back the
get_rte_attribute_is_dropped test: it results in an assertion failure
if the view in fact contains any Vars referencing the dropped column.
Once we've got dependencies preventing such cases, we'll probably want
to do it that way instead of relying on the empty-string test used here.

This fix turned up a very ancient bug in outfuncs/readfuncs, namely
that T_String nodes containing empty strings were not dumped/reloaded
correctly: the node was printed as "<>" which is read as a string
value of <>.  Since (per SQL) we disallow empty-string identifiers,
such nodes don't occur normally, which is why we'd not noticed.
(Such nodes aren't used for literal constants, just identifiers.)

Per report from Marc Schablewski.  Back-patch to 9.3 which is where
the rule printing behavior changed.  The dangling-variable case is
broken all the way back, but that's not what his complaint is about.

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 11c74860070ea1c90f2f350dfeea621763fdc772..fc0b5fccf11958468ec2e724e37f878ef3f77357 100644 (file)
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2499,8 +2499,14 @@ _outValue(StringInfo str, const Value *value)
                        appendStringInfoString(str, value->val.str);
                        break;
                case T_String:
+
+                       /*
+                        * We use _outToken to provide escaping of the string's content,
+                        * but we don't want it to do anything with an empty string.
+                        */
                        appendStringInfoChar(str, '"');
-                       _outToken(str, value->val.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 a30d8febf853590707d07522f13e5bef863e328c..143c8b268bb7e83ba3794626d718d4aac6d14b22 100644 (file)
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -3086,7 +3086,16 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte,
                i = 0;
                foreach(lc, rte->eref->colnames)
                {
-                       real_colnames[i] = strVal(lfirst(lc));
+                       /*
+                        * 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 06b203793a3f848e2d8fee54aa73d8e362a790e4..e2d427667593d91154fc7f766707f842d0bf82dd 100644 (file)
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -1332,6 +1332,58 @@ select pg_get_viewdef('vv6', true);
       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 e09bc1a2799becf5f60be20cb0f50b4cbf72b52e..d3b3f128bb3eb9098d7e4a675885d078bd6f54b7 100644 (file)
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -435,6 +435,40 @@ alter table tt11 add column z int;
 
 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;