From 877cdf11eaa9cabcb9b1e3c1bef0760fe08efdc3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 16 Mar 2018 12:48:13 -0400 Subject: [PATCH] Mop-up for letting VOID-returning SQL functions end with a SELECT. Part of the intent in commit fd1a421fe was to allow SQL functions that are declared to return VOID to contain anything, including an unrelated final SELECT, the same as SQL-language procedures can. However, the planner's inlining logic didn't get that memo. Fix it, and add some regression tests covering this area, since evidently we had none. In passing, clean up some typos in comments in create_function_3.sql, and get rid of its none-too-safe assumption that DROP CASCADE notice output is immutably ordered. Per report from Prabhat Sahu. Discussion: https://postgr.es/m/CANEvxPqxAj6nNHVcaXxpTeEFPmh24Whu+23emgjiuKrhJSct0A@mail.gmail.com --- src/backend/optimizer/util/clauses.c | 25 ++++-- .../regress/expected/create_function_3.out | 90 ++++++++++++++----- src/test/regress/sql/create_function_3.sql | 48 ++++++++-- 3 files changed, 130 insertions(+), 33 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index a9a09afd2b..93e5658a35 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -4487,8 +4487,8 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, * properties. (The prokind and nargs checks are just paranoia.) */ if (funcform->prolang != SQLlanguageId || - funcform->prosecdef || funcform->prokind != PROKIND_FUNCTION || + funcform->prosecdef || funcform->proretset || funcform->prorettype == RECORDOID || !heap_attisnull(func_tuple, Anum_pg_proc_proconfig) || @@ -4623,9 +4623,18 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, /* Now we can grab the tlist expression */ newexpr = (Node *) ((TargetEntry *) linitial(querytree->targetList))->expr; - /* Assert that check_sql_fn_retval did the right thing */ - Assert(exprType(newexpr) == result_type); - /* It couldn't have made any dangerous tlist changes, either */ + /* + * If the SQL function returns VOID, we can only inline it if it is a + * SELECT of an expression returning VOID (ie, it's just a redirection to + * another VOID-returning function). In all non-VOID-returning cases, + * check_sql_fn_retval should ensure that newexpr returns the function's + * declared result type, so this test shouldn't fail otherwise; but we may + * as well cope gracefully if it does. + */ + if (exprType(newexpr) != result_type) + goto fail; + + /* check_sql_fn_retval couldn't have made any dangerous tlist changes */ Assert(!modifyTargetList); /* @@ -5010,12 +5019,16 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) * properties. In particular it mustn't be declared STRICT, since we * couldn't enforce that. It also mustn't be VOLATILE, because that is * supposed to cause it to be executed with its own snapshot, rather than - * sharing the snapshot of the calling query. (Rechecking proretset is - * just paranoia.) + * sharing the snapshot of the calling query. We also disallow returning + * SETOF VOID, because inlining would result in exposing the actual result + * of the function's last SELECT, which should not happen in that case. + * (Rechecking prokind and proretset is just paranoia.) */ if (funcform->prolang != SQLlanguageId || + funcform->prokind != PROKIND_FUNCTION || funcform->proisstrict || funcform->provolatile == PROVOLATILE_VOLATILE || + funcform->prorettype == VOIDOID || funcform->prosecdef || !funcform->proretset || !heap_attisnull(func_tuple, Anum_pg_proc_proconfig)) diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out index 175f3b59a3..3301885fc8 100644 --- a/src/test/regress/expected/create_function_3.out +++ b/src/test/regress/expected/create_function_3.out @@ -1,13 +1,17 @@ -- -- CREATE FUNCTION -- --- sanity check of pg_proc catalog to the given parameters +-- Assorted tests using SQL-language functions -- +-- All objects made in this test are in temp_func_test schema CREATE USER regress_unpriv_user; CREATE SCHEMA temp_func_test; GRANT ALL ON SCHEMA temp_func_test TO public; SET search_path TO temp_func_test, public; -- +-- Make sanity checks on the pg_proc entries created by CREATE FUNCTION +-- +-- -- ARGUMENT and RETURN TYPES -- CREATE FUNCTION functest_A_1(text, date) RETURNS bool LANGUAGE 'sql' @@ -127,7 +131,7 @@ SELECT proname, proleakproof FROM pg_proc functest_e_2 | t (2 rows) -ALTER FUNCTION functest_E_2(int) NOT LEAKPROOF; -- remove leakproog attribute +ALTER FUNCTION functest_E_2(int) NOT LEAKPROOF; -- remove leakproof attribute SELECT proname, proleakproof FROM pg_proc WHERE oid in ('functest_E_1'::regproc, 'functest_E_2'::regproc) ORDER BY proname; @@ -137,7 +141,7 @@ SELECT proname, proleakproof FROM pg_proc functest_e_2 | f (2 rows) --- it takes superuser privilege to turn on leakproof, but not for turn off +-- it takes superuser privilege to turn on leakproof, but not to turn off ALTER FUNCTION functest_E_1(int) OWNER TO regress_unpriv_user; ALTER FUNCTION functest_E_2(int) OWNER TO regress_unpriv_user; SET SESSION AUTHORIZATION regress_unpriv_user; @@ -146,7 +150,7 @@ ALTER FUNCTION functest_E_1(int) NOT LEAKPROOF; ALTER FUNCTION functest_E_2(int) LEAKPROOF; ERROR: only superuser can define a leakproof function CREATE FUNCTION functest_E_3(int) RETURNS bool LANGUAGE 'sql' - LEAKPROOF AS 'SELECT $1 < 200'; -- failed + LEAKPROOF AS 'SELECT $1 < 200'; -- fail ERROR: only superuser can define a leakproof function RESET SESSION AUTHORIZATION; -- @@ -280,24 +284,66 @@ CREATE OR REPLACE PROCEDURE functest1(a int) LANGUAGE SQL AS 'SELECT $1'; ERROR: cannot change routine kind DETAIL: "functest1" is a function. DROP FUNCTION functest1(a int); --- Cleanups +-- Check behavior of VOID-returning SQL functions +CREATE FUNCTION voidtest1(a int) RETURNS VOID LANGUAGE SQL AS +$$ SELECT a + 1 $$; +SELECT voidtest1(42); + voidtest1 +----------- + +(1 row) + +CREATE FUNCTION voidtest2(a int, b int) RETURNS VOID LANGUAGE SQL AS +$$ SELECT voidtest1(a + b) $$; +SELECT voidtest2(11,22); + voidtest2 +----------- + +(1 row) + +-- currently, we can inline voidtest2 but not voidtest1 +EXPLAIN (verbose, costs off) SELECT voidtest2(11,22); + QUERY PLAN +------------------------- + Result + Output: voidtest1(33) +(2 rows) + +CREATE TEMP TABLE sometable(f1 int); +CREATE FUNCTION voidtest3(a int) RETURNS VOID LANGUAGE SQL AS +$$ INSERT INTO sometable VALUES(a + 1) $$; +SELECT voidtest3(17); + voidtest3 +----------- + +(1 row) + +CREATE FUNCTION voidtest4(a int) RETURNS VOID LANGUAGE SQL AS +$$ INSERT INTO sometable VALUES(a - 1) RETURNING f1 $$; +SELECT voidtest4(39); + voidtest4 +----------- + +(1 row) + +TABLE sometable; + f1 +---- + 18 + 38 +(2 rows) + +CREATE FUNCTION voidtest5(a int) RETURNS SETOF VOID LANGUAGE SQL AS +$$ SELECT generate_series(1, a) $$ STABLE; +SELECT * FROM voidtest5(3); + voidtest5 +----------- +(0 rows) + +-- Cleanup +\set VERBOSITY terse \\ -- suppress cascade details DROP SCHEMA temp_func_test CASCADE; -NOTICE: drop cascades to 16 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() -drop cascades to function functest_b_2(integer) -drop cascades to function functest_b_3(integer) -drop cascades to function functest_b_4(integer) -drop cascades to function functest_c_1(integer) -drop cascades to function functest_c_2(integer) -drop cascades to function functest_c_3(integer) -drop cascades to function functest_e_1(integer) -drop cascades to function functest_e_2(integer) -drop cascades to function functest_f_1(integer) -drop cascades to function functest_f_2(integer) -drop cascades to function functest_f_3(integer) -drop cascades to function functest_f_4(integer) -drop cascades to function functest_b_2(bigint) +NOTICE: drop cascades to 21 other objects +\set VERBOSITY default DROP USER regress_unpriv_user; RESET search_path; diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql index 8f209d55af..24bb900990 100644 --- a/src/test/regress/sql/create_function_3.sql +++ b/src/test/regress/sql/create_function_3.sql @@ -1,8 +1,11 @@ -- -- CREATE FUNCTION -- --- sanity check of pg_proc catalog to the given parameters +-- Assorted tests using SQL-language functions -- + +-- All objects made in this test are in temp_func_test schema + CREATE USER regress_unpriv_user; CREATE SCHEMA temp_func_test; @@ -10,6 +13,10 @@ GRANT ALL ON SCHEMA temp_func_test TO public; SET search_path TO temp_func_test, public; +-- +-- Make sanity checks on the pg_proc entries created by CREATE FUNCTION +-- + -- -- ARGUMENT and RETURN TYPES -- @@ -88,12 +95,12 @@ SELECT proname, proleakproof FROM pg_proc WHERE oid in ('functest_E_1'::regproc, 'functest_E_2'::regproc) ORDER BY proname; -ALTER FUNCTION functest_E_2(int) NOT LEAKPROOF; -- remove leakproog attribute +ALTER FUNCTION functest_E_2(int) NOT LEAKPROOF; -- remove leakproof attribute SELECT proname, proleakproof FROM pg_proc WHERE oid in ('functest_E_1'::regproc, 'functest_E_2'::regproc) ORDER BY proname; --- it takes superuser privilege to turn on leakproof, but not for turn off +-- it takes superuser privilege to turn on leakproof, but not to turn off ALTER FUNCTION functest_E_1(int) OWNER TO regress_unpriv_user; ALTER FUNCTION functest_E_2(int) OWNER TO regress_unpriv_user; @@ -103,7 +110,7 @@ ALTER FUNCTION functest_E_1(int) NOT LEAKPROOF; ALTER FUNCTION functest_E_2(int) LEAKPROOF; CREATE FUNCTION functest_E_3(int) RETURNS bool LANGUAGE 'sql' - LEAKPROOF AS 'SELECT $1 < 200'; -- failed + LEAKPROOF AS 'SELECT $1 < 200'; -- fail RESET SESSION AUTHORIZATION; @@ -183,7 +190,38 @@ CREATE OR REPLACE PROCEDURE functest1(a int) LANGUAGE SQL AS 'SELECT $1'; DROP FUNCTION functest1(a int); --- Cleanups +-- Check behavior of VOID-returning SQL functions + +CREATE FUNCTION voidtest1(a int) RETURNS VOID LANGUAGE SQL AS +$$ SELECT a + 1 $$; +SELECT voidtest1(42); + +CREATE FUNCTION voidtest2(a int, b int) RETURNS VOID LANGUAGE SQL AS +$$ SELECT voidtest1(a + b) $$; +SELECT voidtest2(11,22); + +-- currently, we can inline voidtest2 but not voidtest1 +EXPLAIN (verbose, costs off) SELECT voidtest2(11,22); + +CREATE TEMP TABLE sometable(f1 int); + +CREATE FUNCTION voidtest3(a int) RETURNS VOID LANGUAGE SQL AS +$$ INSERT INTO sometable VALUES(a + 1) $$; +SELECT voidtest3(17); + +CREATE FUNCTION voidtest4(a int) RETURNS VOID LANGUAGE SQL AS +$$ INSERT INTO sometable VALUES(a - 1) RETURNING f1 $$; +SELECT voidtest4(39); + +TABLE sometable; + +CREATE FUNCTION voidtest5(a int) RETURNS SETOF VOID LANGUAGE SQL AS +$$ SELECT generate_series(1, a) $$ STABLE; +SELECT * FROM voidtest5(3); + +-- Cleanup +\set VERBOSITY terse \\ -- suppress cascade details DROP SCHEMA temp_func_test CASCADE; +\set VERBOSITY default DROP USER regress_unpriv_user; RESET search_path;