Re: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered - Mailing list pgsql-bugs

I wrote:
> I think I'll go take a look at the missing-dependency aspect now.
> I realized from checking the commit log that we've been putting
> off doing that since 2014, if not before.  Really should fix it.

Here's a proposed patch for that.  I wouldn't consider pushing this
into released branches, but maybe it's not too late for v15?

            regards, tom lane

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index cf9ceddff1..e119674b1f 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -74,6 +74,7 @@
 #include "commands/sequence.h"
 #include "commands/trigger.h"
 #include "commands/typecmds.h"
+#include "funcapi.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteRemove.h"
@@ -205,6 +206,8 @@ static void deleteOneObject(const ObjectAddress *object,
 static void doDeletion(const ObjectAddress *object, int flags);
 static bool find_expr_references_walker(Node *node,
                                         find_expr_references_context *context);
+static void process_function_rte_ref(RangeTblEntry *rte, AttrNumber attnum,
+                                     find_expr_references_context *context);
 static void eliminate_duplicate_dependencies(ObjectAddresses *addrs);
 static int    object_address_comparator(const void *a, const void *b);
 static void add_object_address(ObjectClass oclass, Oid objectId, int32 subId,
@@ -1768,6 +1771,12 @@ find_expr_references_walker(Node *node,
             add_object_address(OCLASS_CLASS, rte->relid, var->varattno,
                                context->addrs);
         }
+        else if (rte->rtekind == RTE_FUNCTION)
+        {
+            /* Might need to add a dependency on a composite type's column */
+            /* (done out of line, because it's a bit bulky) */
+            process_function_rte_ref(rte, var->varattno, context);
+        }

         /*
          * Vars referencing other RTE types require no additional work.  In
@@ -2342,6 +2351,65 @@ find_expr_references_walker(Node *node,
                                   (void *) context);
 }

+/*
+ * find_expr_references_walker subroutine: handle a Var reference
+ * to an RTE_FUNCTION RTE
+ */
+static void
+process_function_rte_ref(RangeTblEntry *rte, AttrNumber attnum,
+                         find_expr_references_context *context)
+{
+    int            atts_done = 0;
+    ListCell   *lc;
+
+    /*
+     * Identify which RangeTblFunction produces this attnum, and see if it
+     * returns a composite type.  If so, we'd better make a dependency on the
+     * referenced column of the composite type (or actually, of its associated
+     * relation).
+     */
+    foreach(lc, rte->functions)
+    {
+        RangeTblFunction *rtfunc = (RangeTblFunction *) lfirst(lc);
+
+        if (attnum > atts_done &&
+            attnum <= atts_done + rtfunc->funccolcount)
+        {
+            TupleDesc    tupdesc;
+
+            tupdesc = get_expr_result_tupdesc(rtfunc->funcexpr, true);
+            if (tupdesc && tupdesc->tdtypeid != RECORDOID)
+            {
+                /*
+                 * Named composite type, so individual columns could get
+                 * dropped.  Make a dependency on this specific column.
+                 */
+                Oid            reltype = get_typ_typrelid(tupdesc->tdtypeid);
+
+                Assert(attnum - atts_done <= tupdesc->natts);
+                if (OidIsValid(reltype))    /* can this fail? */
+                    add_object_address(OCLASS_CLASS, reltype,
+                                       attnum - atts_done,
+                                       context->addrs);
+                return;
+            }
+            /* Nothing to do; function's result type is handled elsewhere */
+            return;
+        }
+        atts_done += rtfunc->funccolcount;
+    }
+
+    /* If we get here, must be looking for the ordinality column */
+    if (rte->funcordinality && attnum == atts_done + 1)
+        return;
+
+    /* this probably can't happen ... */
+    ereport(ERROR,
+            (errcode(ERRCODE_UNDEFINED_COLUMN),
+             errmsg("column %d of relation \"%s\" does not exist",
+                    attnum, rte->eref->aliasname)));
+}
+
 /*
  * Given an array of dependency references, eliminate any duplicates.
  */
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 32fffa472c..d575aa0066 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7330,9 +7330,9 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
         /*
          * If we find a Var referencing a dropped column, it seems better to
          * print something (anything) than to fail.  In general this should
-         * not happen, but there are specific cases involving functions
-         * returning named composite types where we don't sufficiently enforce
-         * that you can't drop a column that's referenced in some view.
+         * not happen, but it used to be possible for some cases involving
+         * functions returning named composite types, and perhaps there are
+         * still bugs out there.
          */
         if (attname == NULL)
             attname = "?dropped?column?";
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index 6ee49249fc..a5db1b4b8e 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -1597,62 +1597,52 @@ select * from tt14v;
  foo | baz | 42
 (1 row)

-begin;
--- this perhaps should be rejected, but it isn't:
-alter table tt14t drop column f3;
--- column f3 is still in the view, sort of ...
+alter table tt14t drop column f3;  -- fail, view has explicit reference to f3
+ERROR:  cannot drop column f3 of table tt14t because other objects depend on it
+DETAIL:  view tt14v depends on column f3 of table tt14t
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+drop view tt14v;
+create view tt14v as select t.f1, t.f4 from tt14f() t;
 select pg_get_viewdef('tt14v', true);
-         pg_get_viewdef
----------------------------------
-  SELECT t.f1,                  +
-     t."?dropped?column?" AS f3,+
-     t.f4                       +
+         pg_get_viewdef
+--------------------------------
+  SELECT t.f1,                 +
+     t.f4                      +
+    FROM tt14f() t(f1, f3, f4);
+(1 row)
+
+select * from tt14v;
+ f1  | f4
+-----+----
+ foo | 42
+(1 row)
+
+alter table tt14t drop column f3;  -- ok
+select pg_get_viewdef('tt14v', true);
+       pg_get_viewdef
+----------------------------
+  SELECT t.f1,             +
+     t.f4                  +
     FROM tt14f() t(f1, f4);
 (1 row)

--- ... and you can even EXPLAIN it ...
 explain (verbose, costs off) select * from tt14v;
                QUERY PLAN
 ----------------------------------------
  Function Scan on testviewschm2.tt14f t
-   Output: t.f1, t.f3, t.f4
+   Output: t.f1, t.f4
    Function Call: tt14f()
 (3 rows)

--- but it will fail at execution
-select f1, f4 from tt14v;
+select * from tt14v;
  f1  | f4
 -----+----
  foo | 42
 (1 row)

-select * from tt14v;
-ERROR:  attribute 3 of type record has been dropped
-rollback;
-begin;
--- this perhaps should be rejected, but it isn't:
-alter table tt14t alter column f4 type integer using f4::integer;
--- f4 is still in the view ...
-select pg_get_viewdef('tt14v', true);
-         pg_get_viewdef
---------------------------------
-  SELECT t.f1,                 +
-     t.f3,                     +
-     t.f4                      +
-    FROM tt14f() t(f1, f3, f4);
-(1 row)
-
--- but will fail at execution
-select f1, f3 from tt14v;
- f1  | f3
------+-----
- foo | baz
-(1 row)
-
-select * from tt14v;
-ERROR:  attribute 4 of type record has wrong type
-DETAIL:  Table has type integer, but query expects text.
-rollback;
+alter table tt14t alter column f4 type integer using f4::integer;  -- fail
+ERROR:  cannot alter type of a column used by a view or rule
+DETAIL:  rule _RETURN on view tt14v depends on column "f4"
 -- check display of whole-row variables in some corner cases
 create type nestedcomposite as (x int8_tbl);
 create view tt15v as select row(i)::nestedcomposite from int8_tbl i;
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index 2334a1321e..2721d5bf62 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -2247,15 +2247,13 @@ select * from usersview;
  id2    |   2 | email2 |       12 | t       |              11 |          2
 (2 rows)

-begin;
-alter table users drop column moredrop;
-select * from usersview;  -- expect clean failure
-ERROR:  attribute 5 of type record has been dropped
-rollback;
-alter table users alter column seq type numeric;
-select * from usersview;  -- expect clean failure
-ERROR:  attribute 2 of type record has wrong type
-DETAIL:  Table has type numeric, but query expects integer.
+alter table users drop column moredrop;  -- fail
+ERROR:  cannot drop column moredrop of table users because other objects depend on it
+DETAIL:  view usersview depends on column moredrop of table users
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+alter table users alter column seq type numeric;  -- fail
+ERROR:  cannot alter type of a column used by a view or rule
+DETAIL:  rule _RETURN on view usersview depends on column "seq"
 drop view usersview;
 drop function get_first_user();
 drop function get_users();
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 949b116625..ec4ca72cac 100644
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -575,33 +575,22 @@ create view tt14v as select t.* from tt14f() t;
 select pg_get_viewdef('tt14v', true);
 select * from tt14v;

-begin;
+alter table tt14t drop column f3;  -- fail, view has explicit reference to f3

--- this perhaps should be rejected, but it isn't:
-alter table tt14t drop column f3;
+drop view tt14v;
+
+create view tt14v as select t.f1, t.f4 from tt14f() t;

--- column f3 is still in the view, sort of ...
 select pg_get_viewdef('tt14v', true);
--- ... and you can even EXPLAIN it ...
-explain (verbose, costs off) select * from tt14v;
--- but it will fail at execution
-select f1, f4 from tt14v;
 select * from tt14v;

-rollback;
-
-begin;
+alter table tt14t drop column f3;  -- ok

--- this perhaps should be rejected, but it isn't:
-alter table tt14t alter column f4 type integer using f4::integer;
-
--- f4 is still in the view ...
 select pg_get_viewdef('tt14v', true);
--- but will fail at execution
-select f1, f3 from tt14v;
+explain (verbose, costs off) select * from tt14v;
 select * from tt14v;

-rollback;
+alter table tt14t alter column f4 type integer using f4::integer;  -- fail

 -- check display of whole-row variables in some corner cases

diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql
index 7e5cde14c4..c834e82c3a 100644
--- a/src/test/regress/sql/rangefuncs.sql
+++ b/src/test/regress/sql/rangefuncs.sql
@@ -682,12 +682,8 @@ SELECT * FROM ROWS FROM(get_users(), generate_series(10,11)) WITH ORDINALITY;
 select * from usersview;
 alter table users add column junk text;
 select * from usersview;
-begin;
-alter table users drop column moredrop;
-select * from usersview;  -- expect clean failure
-rollback;
-alter table users alter column seq type numeric;
-select * from usersview;  -- expect clean failure
+alter table users drop column moredrop;  -- fail
+alter table users alter column seq type numeric;  -- fail

 drop view usersview;
 drop function get_first_user();