Re: BUG #17161: Assert failed on opening a relation that exists in two schemas via the LANGUAGE SQL function - Mailing list pgsql-bugs

Alexander Lakhin <exclusion@gmail.com> writes:
> My initial conclusion was incorrect. Duplicate relation is not needed in
> the second schema. It's sufficient to just change search_path to another
> schema:

Actually, you don't need to mess with search_path at all: inlining of
set-returning SQL functions just plain does not work, if they are
new-style.  The reason is that we forgot to apply AcquireRewriteLocks
in that code path.  That leads to the assertion failure you show here.
Even without that, we risk problems with JOINs on dropped columns, which
is another thing that AcquireRewriteLocks is charged with cleaning up.

The regression tests fail to show this problem because, while they do
test inlining of a new-style SRF, the solitary test case contains no
relation references.

I looked around at other places that consult prosqlbody, and saw
that the only one that accounts for the lock issue is init_sql_fcache.
fmgr_sql_validator and print_function_sqlbody are also being cavalier
about this, and I doubt that either one of them is really safe.
(You could maybe argue that print_function_sqlbody doesn't need locks,
but in view of the dropped-JOIN-column issue, I don't think I believe
it.  Likewise in fmgr_sql_validator, which wasn't applying the rewriter
at all, which I doubt is OK considering the older code path does it.)

Proposed patch attached.

            regards, tom lane

diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 1454d2fb67..25d35230d0 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -35,6 +35,7 @@
 #include "parser/analyze.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_type.h"
+#include "rewrite/rewriteHandler.h"
 #include "tcop/pquery.h"
 #include "tcop/tcopprot.h"
 #include "utils/acl.h"
@@ -891,12 +892,30 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
         if (!isnull)
         {
             Node       *n;
+            List       *stored_query_list;

             n = stringToNode(TextDatumGetCString(tmp));
             if (IsA(n, List))
-                querytree_list = castNode(List, n);
+                stored_query_list = linitial(castNode(List, n));
             else
-                querytree_list = list_make1(list_make1(n));
+                stored_query_list = list_make1(n);
+
+            querytree_list = NIL;
+            foreach(lc, stored_query_list)
+            {
+                Query       *parsetree = lfirst_node(Query, lc);
+                List       *querytree_sublist;
+
+                /*
+                 * Typically, we'd have acquired locks already while parsing
+                 * the body of the CREATE FUNCTION command.  However, a
+                 * validator function cannot assume that it's only called in
+                 * that context.
+                 */
+                AcquireRewriteLocks(parsetree, true, false);
+                querytree_sublist = pg_rewrite_query(parsetree);
+                querytree_list = lappend(querytree_list, querytree_sublist);
+            }
         }
         else
         {
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 7187f17da5..3412d31117 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -43,6 +43,7 @@
 #include "parser/parse_agg.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_func.h"
+#include "rewrite/rewriteHandler.h"
 #include "rewrite/rewriteManip.h"
 #include "tcop/tcopprot.h"
 #include "utils/acl.h"
@@ -4470,6 +4471,12 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
         if (list_length(querytree_list) != 1)
             goto fail;
         querytree = linitial(querytree_list);
+
+        /*
+         * Because we'll insist below that the querytree have an empty rtable
+         * and no sublinks, it cannot have any relation references that need
+         * to be locked or rewritten.  So we can omit those steps.
+         */
     }
     else
     {
@@ -5022,6 +5029,8 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
             goto fail;
         querytree = linitial(querytree_list);

+        /* Acquire necessary locks, then apply rewriter. */
+        AcquireRewriteLocks(querytree, true, false);
         querytree_list = pg_rewrite_query(querytree);
         if (list_length(querytree_list) != 1)
             goto fail;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 4df8cc5abf..c92958149e 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2973,7 +2973,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
     }

     /* And finally the function definition ... */
-    tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
+    (void) SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
     if (proc->prolang == SQLlanguageId && !isnull)
     {
         print_function_sqlbody(&buf, proctup);
@@ -3439,7 +3439,10 @@ print_function_sqlbody(StringInfo buf, HeapTuple proctup)
         {
             Query       *query = lfirst_node(Query, lc);

-            get_query_def(query, buf, list_make1(&dpns), NULL, PRETTYFLAG_INDENT, WRAP_COLUMN_DEFAULT, 1);
+            /* It seems advisable to get at least AccessShareLock on rels */
+            AcquireRewriteLocks(query, false, false);
+            get_query_def(query, buf, list_make1(&dpns), NULL,
+                          PRETTYFLAG_INDENT, WRAP_COLUMN_DEFAULT, 1);
             appendStringInfoChar(buf, ';');
             appendStringInfoChar(buf, '\n');
         }
@@ -3448,7 +3451,12 @@ print_function_sqlbody(StringInfo buf, HeapTuple proctup)
     }
     else
     {
-        get_query_def(castNode(Query, n), buf, list_make1(&dpns), NULL, 0, WRAP_COLUMN_DEFAULT, 0);
+        Query       *query = castNode(Query, n);
+
+        /* It seems advisable to get at least AccessShareLock on rels */
+        AcquireRewriteLocks(query, false, false);
+        get_query_def(query, buf, list_make1(&dpns), NULL,
+                      0, WRAP_COLUMN_DEFAULT, 0);
     }
 }

@@ -3467,7 +3475,7 @@ pg_get_function_sqlbody(PG_FUNCTION_ARGS)
     if (!HeapTupleIsValid(proctup))
         PG_RETURN_NULL();

-    SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
+    (void) SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
     if (isnull)
     {
         ReleaseSysCache(proctup);
diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out
index 5955859bb5..a77df01042 100644
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -543,11 +543,13 @@ ERROR:  cannot change routine kind
 DETAIL:  "functest1" is a function.
 DROP FUNCTION functest1(a int);
 -- inlining of set-returning functions
+CREATE TABLE functest3 (a int);
+INSERT INTO functest3 VALUES (1), (2), (3);
 CREATE FUNCTION functest_sri1() RETURNS SETOF int
 LANGUAGE SQL
 STABLE
 AS '
-    VALUES (1), (2), (3);
+    SELECT * FROM functest3;
 ';
 SELECT * FROM functest_sri1();
  functest_sri1
@@ -558,17 +560,17 @@ SELECT * FROM functest_sri1();
 (3 rows)

 EXPLAIN (verbose, costs off) SELECT * FROM functest_sri1();
-          QUERY PLAN
-------------------------------
- Values Scan on "*VALUES*"
-   Output: "*VALUES*".column1
+              QUERY PLAN
+--------------------------------------
+ Seq Scan on temp_func_test.functest3
+   Output: functest3.a
 (2 rows)

 CREATE FUNCTION functest_sri2() RETURNS SETOF int
 LANGUAGE SQL
 STABLE
 BEGIN ATOMIC
-    VALUES (1), (2), (3);
+    SELECT * FROM functest3;
 END;
 SELECT * FROM functest_sri2();
  functest_sri2
@@ -579,12 +581,14 @@ SELECT * FROM functest_sri2();
 (3 rows)

 EXPLAIN (verbose, costs off) SELECT * FROM functest_sri2();
-          QUERY PLAN
-------------------------------
- Values Scan on "*VALUES*"
-   Output: "*VALUES*".column1
+              QUERY PLAN
+--------------------------------------
+ Seq Scan on temp_func_test.functest3
+   Output: functest3.a
 (2 rows)

+DROP TABLE functest3 CASCADE;
+NOTICE:  drop cascades to function functest_sri2()
 -- Check behavior of VOID-returning SQL functions
 CREATE FUNCTION voidtest1(a int) RETURNS VOID LANGUAGE SQL AS
 $$ SELECT a + 1 $$;
@@ -643,7 +647,7 @@ SELECT * FROM voidtest5(3);

 -- Cleanup
 DROP SCHEMA temp_func_test CASCADE;
-NOTICE:  drop cascades to 30 other objects
+NOTICE:  drop cascades to 29 other objects
 DETAIL:  drop cascades to function functest_a_1(text,date)
 drop cascades to function functest_a_2(text[])
 drop cascades to function functest_a_3()
@@ -668,7 +672,6 @@ drop cascades to function functest_s_13()
 drop cascades to function functest_s_15(integer)
 drop cascades to function functest_b_2(bigint)
 drop cascades to function functest_sri1()
-drop cascades to function functest_sri2()
 drop cascades to function voidtest1(integer)
 drop cascades to function voidtest2(integer,integer)
 drop cascades to function voidtest3(integer)
diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql
index 6e8b838ff2..23a46b0b11 100644
--- a/src/test/regress/sql/create_function_3.sql
+++ b/src/test/regress/sql/create_function_3.sql
@@ -319,11 +319,14 @@ DROP FUNCTION functest1(a int);

 -- inlining of set-returning functions

+CREATE TABLE functest3 (a int);
+INSERT INTO functest3 VALUES (1), (2), (3);
+
 CREATE FUNCTION functest_sri1() RETURNS SETOF int
 LANGUAGE SQL
 STABLE
 AS '
-    VALUES (1), (2), (3);
+    SELECT * FROM functest3;
 ';

 SELECT * FROM functest_sri1();
@@ -333,12 +336,14 @@ CREATE FUNCTION functest_sri2() RETURNS SETOF int
 LANGUAGE SQL
 STABLE
 BEGIN ATOMIC
-    VALUES (1), (2), (3);
+    SELECT * FROM functest3;
 END;

 SELECT * FROM functest_sri2();
 EXPLAIN (verbose, costs off) SELECT * FROM functest_sri2();

+DROP TABLE functest3 CASCADE;
+

 -- Check behavior of VOID-returning SQL functions