Fix missed lock acquisition while inlining new-style SQL functions.

When starting to use a query parsetree loaded from the catalogs,
we must begin by applying AcquireRewriteLocks(), to obtain the same
relation locks that the parser would have gotten if the query were
entered interactively, and to do some other cleanup such as dealing
with later-dropped columns.  New-style SQL functions are just as
subject to this rule as other stored parsetrees; however, of the
places dealing with such functions, only init_sql_fcache had gotten
the memo.  In particular, if we successfully inlined a new-style
set-returning SQL function that contained any relation references,
we'd either get an assertion failure or attempt to use those
relation(s) sans locks.

I also added AcquireRewriteLocks calls to fmgr_sql_validator and
print_function_sqlbody.  Desultory experiments didn't demonstrate any
failures in those, but I suspect that I just didn't try hard enough.
Certainly we don't expect nearby code paths to operate without locks.

On the same logic of it-ought-to-have-the-same-effects-as-the-old-code,
call pg_rewrite_query() in fmgr_sql_validator, too.  It's possible
that neither code path there needs to bother with rewriting, but
doing the analysis to prove that is beyond my goals for today.

Per bug #17161 from Alexander Lakhin.

Discussion: https://postgr.es/m/17161-048a1cdff8422800@postgresql.org

diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 1454d2fb67b020d0210ff33b0f4004f6e1ea3c2f..25d35230d027d30053764d14fa0d8cc910b76177 100644 (file)
--- 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 7187f17da57423a494591ed75a950672c059e65e..3412d31117aa74d619b962a6675a35cdc8c2b8c9 100644 (file)
--- 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 4df8cc5abf6c240b3343e8eae8ac458938d759b4..c92958149e859b9ccfbfb167e461c03d4c9ec1fd 100644 (file)
--- 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 5955859bb57b4a21ea3f7882fda27944bf189298..a77df01042efa509761f7e2084aa27039ed50a86 100644 (file)
--- 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 6e8b838ff2d48082c5be9b0ecfd83a694ed4a532..23a46b0b11426e1787b5e38159a21609ca3efc8e 100644 (file)
--- 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