Обсуждение: BUG #17161: Assert failed on opening a relation that exists in two schemas via the LANGUAGE SQL function
BUG #17161: Assert failed on opening a relation that exists in two schemas via the LANGUAGE SQL function
От
PG Bug reporting form
Дата:
The following bug has been logged on the website:
Bug reference: 17161
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 14beta3
Operating system: Ubuntu 20.04
Description:
When executing the following query (based on the create_function_3 test):
CREATE TABLE functest2 (a int, b int);
CREATE SCHEMA temp_func_test;
SET search_path TO temp_func_test, public;
CREATE TABLE functest2 (a int, b int);
CREATE FUNCTION functest_sri1() RETURNS SETOF int
LANGUAGE SQL
STABLE
RETURN (SELECT count(a) FROM functest2);
SELECT * FROM functest_sri1();
I get a failed assertion with the following stacktrace:
Core was generated by `postgres: law regression [local] SELECT
'.
Program terminated with signal SIGABRT, Aborted.
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007fc1c488c859 in __GI_abort () at abort.c:79
#2 0x0000558a9eea3a4a in ExceptionalCondition (
conditionName=conditionName@entry=0x558a9f0146e0 "lockmode != NoLock ||
IsBootstrapProcessingMode() || CheckRelationLockedByMe(r, AccessShareLock,
true)", errorType=errorType@entry=0x558a9f0145e0 "FailedAssertion",
fileName=fileName@entry=0x558a9f0145a0 "relation.c",
lineNumber=lineNumber@entry=68) at assert.c:69
#3 0x0000558a9dced3a8 in relation_open (relationId=16389,
lockmode=lockmode@entry=0) at relation.c:68
#4 0x0000558a9df2e764 in table_open (relationId=<optimized out>,
lockmode=lockmode@entry=0) at table.c:43
#5 0x0000558a9e943bd3 in fireRIRrules (parsetree=0x625000051300,
activeRIRs=activeRIRs@entry=0x0)
at rewriteHandler.c:2056
#6 0x0000558a9e944edb in fireRIRonSubLink (node=0x6250000512a8,
activeRIRs=0x0) at rewriteHandler.c:1929
#7 0x0000558a9e5ced9a in expression_tree_walker
(node=node@entry=0x62500004d0a0,
walker=walker@entry=0x558a9e944e43 <fireRIRonSubLink>,
context=context@entry=0x0) at nodeFuncs.c:2159
#8 0x0000558a9e944e94 in fireRIRonSubLink (node=0x62500004d0a0,
activeRIRs=0x0) at rewriteHandler.c:1938
#9 0x0000558a9e5cf0b1 in expression_tree_walker
(node=node@entry=0x6250000519b8,
walker=walker@entry=0x558a9e944e43 <fireRIRonSubLink>,
context=context@entry=0x0) at nodeFuncs.c:2207
#10 0x0000558a9e944e94 in fireRIRonSubLink (node=0x6250000519b8,
activeRIRs=0x0) at rewriteHandler.c:1938
#11 0x0000558a9e5d0156 in query_tree_walker
(query=query@entry=0x625000051158,
walker=walker@entry=0x558a9e944e43 <fireRIRonSubLink>,
context=context@entry=0x0, flags=flags@entry=3)
at nodeFuncs.c:2369
#12 0x0000558a9e94455b in fireRIRrules (parsetree=0x625000051158,
activeRIRs=activeRIRs@entry=0x0)
at rewriteHandler.c:2120
#13 0x0000558a9e950a7b in QueryRewrite
(parsetree=parsetree@entry=0x625000051158) at rewriteHandler.c:4092
#14 0x0000558a9ea63500 in pg_rewrite_query (query=0x625000051158) at
postgres.c:756
#15 0x0000558a9e76f917 in inline_set_returning_function
(root=root@entry=0x625000048940, rte=rte@entry=0x6250000065c0)
at clauses.c:4978
#16 0x0000558a9e74066e in preprocess_function_rtes
(root=root@entry=0x625000048940) at prepjointree.c:656
#17 0x0000558a9e70b5ea in subquery_planner (glob=glob@entry=0x6250000066d8,
parse=parse@entry=0x625000006238,
parent_root=parent_root@entry=0x0,
hasRecursion=hasRecursion@entry=false,
tuple_fraction=tuple_fraction@entry=0)
at planner.c:667
#18 0x0000558a9e70f237 in standard_planner (parse=0x625000006238,
query_string=<optimized out>, cursorOptions=2048,
boundParams=<optimized out>) at planner.c:400
#19 0x0000558a9e7110a3 in planner (parse=parse@entry=0x625000006238,
query_string=query_string@entry=0x625000005220 "SELECT * FROM
functest_sri1();",
cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at planner.c:271
#20 0x0000558a9ea63c01 in pg_plan_query
(querytree=querytree@entry=0x625000006238,
query_string=query_string@entry=0x625000005220 "SELECT * FROM
functest_sri1();",
cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at postgres.c:847
#21 0x0000558a9ea64234 in pg_plan_queries (querytrees=0x625000006fb8,
query_string=query_string@entry=0x625000005220 "SELECT * FROM
functest_sri1();",
cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at postgres.c:939
#22 0x0000558a9ea6482d in exec_simple_query (
query_string=query_string@entry=0x625000005220 "SELECT * FROM
functest_sri1();") at postgres.c:1133
#23 0x0000558a9ea69c43 in PostgresMain (argc=argc@entry=1,
argv=argv@entry=0x7fff77497600, dbname=<optimized out>,
dbname@entry=0x629000011278 "regression",
username=username@entry=0x629000011258 "law") at postgres.c:4486
#24 0x0000558a9e832d79 in BackendRun (port=port@entry=0x615000002d80) at
postmaster.c:4506
#25 0x0000558a9e83c19e in BackendStartup (port=port@entry=0x615000002d80) at
postmaster.c:4228
#26 0x0000558a9e83c938 in ServerLoop () at postmaster.c:1745
#27 0x0000558a9e83f0dc in PostmasterMain (argc=<optimized out>,
argv=<optimized out>) at postmaster.c:1417
#28 0x0000558a9e544823 in main (argc=3, argv=0x603000000610) at main.c:209
25.08.2021 23:00, PG Bug reporting form wrote: > The following bug has been logged on the website: > > Bug reference: 17161 > Logged by: Alexander Lakhin > Email address: exclusion@gmail.com > PostgreSQL version: 14beta3 > Operating system: Ubuntu 20.04 > Description: > > When executing the following query (based on the create_function_3 test): > CREATE TABLE functest2 (a int, b int); > > CREATE SCHEMA temp_func_test; > SET search_path TO temp_func_test, public; > CREATE TABLE functest2 (a int, b int); > > CREATE FUNCTION functest_sri1() RETURNS SETOF int > LANGUAGE SQL > STABLE > RETURN (SELECT count(a) FROM functest2); > > SELECT * FROM functest_sri1(); > 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: CREATE TABLE functest2 (a int, b int); CREATE SCHEMA temp_func_test; SET search_path TO temp_func_test, public; CREATE FUNCTION functest_sri1() RETURNS SETOF int LANGUAGE SQL STABLE RETURN (SELECT count(a) FROM functest2); SELECT * FROM functest_sri1(); Best regards, Alexander
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
31.08.2021 05:39, Tom Lane wrote: > 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. Oops, sorry for the noisy reproduction and incorrect conclusions. I simplified an initial crashing script (thousand lines generated with a fuzzer) but was in hot haste and couldn't think of such a simple explanation. > 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. This patch fixes the issue for me. Thank you! Best regards, Alexander
Alexander Lakhin <exclusion@gmail.com> writes:
> This patch fixes the issue for me. Thank you!
Pushed, then. Thanks for the report!
regards, tom lane