Skip to content

Commit 589be6f

Browse files
tglsfdctglsfdc
committed
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
1 parent bb466c6 commit 589be6f

File tree

5 files changed

+64
-20
lines changed

5 files changed

+64
-20
lines changed

‎src/backend/catalog/pg_proc.c

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "parser/analyze.h"
3636
#include "parser/parse_coerce.h"
3737
#include "parser/parse_type.h"
38+
#include "rewrite/rewriteHandler.h"
3839
#include "tcop/pquery.h"
3940
#include "tcop/tcopprot.h"
4041
#include "utils/acl.h"
@@ -891,12 +892,30 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
891892
if (!isnull)
892893
{
893894
Node *n;
895+
List *stored_query_list;
894896

895897
n = stringToNode(TextDatumGetCString(tmp));
896898
if (IsA(n, List))
897-
querytree_list = castNode(List, n);
899+
stored_query_list = linitial(castNode(List, n));
898900
else
899-
querytree_list = list_make1(list_make1(n));
901+
stored_query_list = list_make1(n);
902+
903+
querytree_list = NIL;
904+
foreach(lc, stored_query_list)
905+
{
906+
Query *parsetree = lfirst_node(Query, lc);
907+
List *querytree_sublist;
908+
909+
/*
910+
* Typically, we'd have acquired locks already while parsing
911+
* the body of the CREATE FUNCTION command. However, a
912+
* validator function cannot assume that it's only called in
913+
* that context.
914+
*/
915+
AcquireRewriteLocks(parsetree, true, false);
916+
querytree_sublist = pg_rewrite_query(parsetree);
917+
querytree_list = lappend(querytree_list, querytree_sublist);
918+
}
900919
}
901920
else
902921
{

‎src/backend/optimizer/util/clauses.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
#include "parser/parse_agg.h"
4444
#include "parser/parse_coerce.h"
4545
#include "parser/parse_func.h"
46+
#include "rewrite/rewriteHandler.h"
4647
#include "rewrite/rewriteManip.h"
4748
#include "tcop/tcopprot.h"
4849
#include "utils/acl.h"
@@ -4470,6 +4471,12 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
44704471
if (list_length(querytree_list) != 1)
44714472
goto fail;
44724473
querytree = linitial(querytree_list);
4474+
4475+
/*
4476+
* Because we'll insist below that the querytree have an empty rtable
4477+
* and no sublinks, it cannot have any relation references that need
4478+
* to be locked or rewritten. So we can omit those steps.
4479+
*/
44734480
}
44744481
else
44754482
{
@@ -5022,6 +5029,8 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
50225029
goto fail;
50235030
querytree = linitial(querytree_list);
50245031

5032+
/* Acquire necessary locks, then apply rewriter. */
5033+
AcquireRewriteLocks(querytree, true, false);
50255034
querytree_list = pg_rewrite_query(querytree);
50265035
if (list_length(querytree_list) != 1)
50275036
goto fail;

‎src/backend/utils/adt/ruleutils.c

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2973,7 +2973,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
29732973
}
29742974

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

3442-
get_query_def(query, buf, list_make1(&dpns), NULL, PRETTYFLAG_INDENT, WRAP_COLUMN_DEFAULT, 1);
3442+
/* It seems advisable to get at least AccessShareLock on rels */
3443+
AcquireRewriteLocks(query, false, false);
3444+
get_query_def(query, buf, list_make1(&dpns), NULL,
3445+
PRETTYFLAG_INDENT, WRAP_COLUMN_DEFAULT, 1);
34433446
appendStringInfoChar(buf, ';');
34443447
appendStringInfoChar(buf, '\n');
34453448
}
@@ -3448,7 +3451,12 @@ print_function_sqlbody(StringInfo buf, HeapTuple proctup)
34483451
}
34493452
else
34503453
{
3451-
get_query_def(castNode(Query, n), buf, list_make1(&dpns), NULL, 0, WRAP_COLUMN_DEFAULT, 0);
3454+
Query *query = castNode(Query, n);
3455+
3456+
/* It seems advisable to get at least AccessShareLock on rels */
3457+
AcquireRewriteLocks(query, false, false);
3458+
get_query_def(query, buf, list_make1(&dpns), NULL,
3459+
0, WRAP_COLUMN_DEFAULT, 0);
34523460
}
34533461
}
34543462

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

3470-
SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
3478+
(void) SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
34713479
if (isnull)
34723480
{
34733481
ReleaseSysCache(proctup);

‎src/test/regress/expected/create_function_3.out

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -543,11 +543,13 @@ ERROR: cannot change routine kind
543543
DETAIL: "functest1" is a function.
544544
DROP FUNCTION functest1(a int);
545545
-- inlining of set-returning functions
546+
CREATE TABLE functest3 (a int);
547+
INSERT INTO functest3 VALUES (1), (2), (3);
546548
CREATE FUNCTION functest_sri1() RETURNS SETOF int
547549
LANGUAGE SQL
548550
STABLE
549551
AS '
550-
VALUES (1), (2), (3);
552+
SELECT * FROM functest3;
551553
';
552554
SELECT * FROM functest_sri1();
553555
functest_sri1
@@ -558,17 +560,17 @@ SELECT * FROM functest_sri1();
558560
(3 rows)
559561

560562
EXPLAIN (verbose, costs off) SELECT * FROM functest_sri1();
561-
QUERY PLAN
562-
------------------------------
563-
Values Scan on "*VALUES*"
564-
Output: "*VALUES*".column1
563+
QUERY PLAN
564+
--------------------------------------
565+
Seq Scan on temp_func_test.functest3
566+
Output: functest3.a
565567
(2 rows)
566568

567569
CREATE FUNCTION functest_sri2() RETURNS SETOF int
568570
LANGUAGE SQL
569571
STABLE
570572
BEGIN ATOMIC
571-
VALUES (1), (2), (3);
573+
SELECT * FROM functest3;
572574
END;
573575
SELECT * FROM functest_sri2();
574576
functest_sri2
@@ -579,12 +581,14 @@ SELECT * FROM functest_sri2();
579581
(3 rows)
580582

581583
EXPLAIN (verbose, costs off) SELECT * FROM functest_sri2();
582-
QUERY PLAN
583-
------------------------------
584-
Values Scan on "*VALUES*"
585-
Output: "*VALUES*".column1
584+
QUERY PLAN
585+
--------------------------------------
586+
Seq Scan on temp_func_test.functest3
587+
Output: functest3.a
586588
(2 rows)
587589

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

644648
-- Cleanup
645649
DROP SCHEMA temp_func_test CASCADE;
646-
NOTICE: drop cascades to 30 other objects
650+
NOTICE: drop cascades to 29 other objects
647651
DETAIL: drop cascades to function functest_a_1(text,date)
648652
drop cascades to function functest_a_2(text[])
649653
drop cascades to function functest_a_3()
@@ -668,7 +672,6 @@ drop cascades to function functest_s_13()
668672
drop cascades to function functest_s_15(integer)
669673
drop cascades to function functest_b_2(bigint)
670674
drop cascades to function functest_sri1()
671-
drop cascades to function functest_sri2()
672675
drop cascades to function voidtest1(integer)
673676
drop cascades to function voidtest2(integer,integer)
674677
drop cascades to function voidtest3(integer)

‎src/test/regress/sql/create_function_3.sql

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -319,11 +319,14 @@ DROP FUNCTION functest1(a int);
319319

320320
-- inlining of set-returning functions
321321

322+
CREATE TABLE functest3 (a int);
323+
INSERT INTO functest3 VALUES (1), (2), (3);
324+
322325
CREATE FUNCTION functest_sri1() RETURNS SETOF int
323326
LANGUAGE SQL
324327
STABLE
325328
AS '
326-
VALUES (1), (2), (3);
329+
SELECT * FROM functest3;
327330
';
328331

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

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

345+
DROP TABLE functest3 CASCADE;
346+
342347

343348
-- Check behavior of VOID-returning SQL functions
344349

0 commit comments

Comments
 (0)