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

@@ -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))

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;

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;