Avoid premature free of pass-by-reference CALL arguments.

Prematurely freeing the EState used to evaluate CALL arguments led, in some
cases, to passing dangling pointers to the procedure.  This was masked in
trivial cases because the argument pointers would point to Const nodes in
the original expression tree, and in some other cases because the result
value would end up in the standalone ExprContext rather than in memory
belonging to the EState --- but that wasn't exactly high quality
programming either, because the standalone ExprContext was never
explicitly freed, breaking assorted API contracts.

In addition, using a separate EState for each argument was just silly.

So let's use just one EState, and one ExprContext, and make the latter
belong to the former rather than be standalone, and clean up the EState
(and hence the ExprContext) post-call.

While at it, improve the function's commentary a bit.

Discussion: https://postgr.es/m/29173.1518282748@sss.pgh.pa.us

3 files changed, 33 insertions, 11 deletions

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 5d94c1ca27c..a027b19744a 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -2204,6 +2204,12 @@ ExecuteDoStmt(DoStmt *stmt, bool atomic)
  * transaction commands based on that information, e.g., call
  * SPI_connect_ext(SPI_OPT_NONATOMIC).  The language should also pass on the
  * atomic flag to any nested invocations to CALL.
+ *
+ * The expression data structures and execution context that we create
+ * within this function are children of the portalContext of the Portal
+ * that the CALL utility statement runs in.  Therefore, any pass-by-ref
+ * values that we're passing to the procedure will survive transaction
+ * commits that might occur inside the procedure.
  */
 void
 ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
@@ -2218,8 +2224,11 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 	FmgrInfo	flinfo;
 	FunctionCallInfoData fcinfo;
 	CallContext *callcontext;
+	EState	   *estate;
+	ExprContext *econtext;
 	HeapTuple	tp;
 
+	/* We need to do parse analysis on the procedure call and its arguments */
 	targs = NIL;
 	foreach(lc, stmt->funccall->args)
 	{
@@ -2241,7 +2250,6 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 	aclresult = pg_proc_aclcheck(fexpr->funcid, GetUserId(), ACL_EXECUTE);
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, OBJECT_PROCEDURE, get_func_name(fexpr->funcid));
-	InvokeFunctionExecuteHook(fexpr->funcid);
 
 	nargs = list_length(fexpr->args);
 
@@ -2254,6 +2262,7 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 							   FUNC_MAX_ARGS,
 							   FUNC_MAX_ARGS)));
 
+	/* Prep the context object we'll pass to the procedure */
 	callcontext = makeNode(CallContext);
 	callcontext->atomic = atomic;
 
@@ -2270,23 +2279,28 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 		callcontext->atomic = true;
 	ReleaseSysCache(tp);
 
+	/* Initialize function call structure */
+	InvokeFunctionExecuteHook(fexpr->funcid);
 	fmgr_info(fexpr->funcid, &flinfo);
 	InitFunctionCallInfoData(fcinfo, &flinfo, nargs, fexpr->inputcollid, (Node *) callcontext, NULL);
 
+	/*
+	 * Evaluate procedure arguments inside a suitable execution context.  Note
+	 * we can't free this context till the procedure returns.
+	 */
+	estate = CreateExecutorState();
+	econtext = CreateExprContext(estate);
+
 	i = 0;
 	foreach(lc, fexpr->args)
 	{
-		EState	   *estate;
 		ExprState  *exprstate;
-		ExprContext *econtext;
 		Datum		val;
 		bool		isnull;
 
-		estate = CreateExecutorState();
 		exprstate = ExecPrepareExpr(lfirst(lc), estate);
-		econtext = CreateStandaloneExprContext();
+
 		val = ExecEvalExprSwitchContext(exprstate, econtext, &isnull);
-		FreeExecutorState(estate);
 
 		fcinfo.arg[i] = val;
 		fcinfo.argnull[i] = isnull;
@@ -2295,4 +2309,6 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 	}
 
 	FunctionCallInvoke(&fcinfo);
+
+	FreeExecutorState(estate);
 }
diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index ccad5c87d5e..873907dc434 100644
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -21,6 +21,8 @@ LINE 1: SELECT ptest1('x');
                ^
 HINT:  To call a procedure, use CALL.
 CALL ptest1('a');  -- ok
+CALL ptest1('xy' || 'zzy');  -- ok, constant-folded arg
+CALL ptest1(substring(random()::text, 1, 1));  -- ok, volatile arg
 \df ptest1
                         List of functions
  Schema |  Name  | Result data type | Argument data types | Type 
@@ -28,11 +30,13 @@ CALL ptest1('a');  -- ok
  public | ptest1 |                  | x text              | proc
 (1 row)
 
-SELECT * FROM cp_test ORDER BY a;
- a | b 
----+---
+SELECT * FROM cp_test ORDER BY b COLLATE "C";
+ a |   b   
+---+-------
+ 1 | 0
  1 | a
-(1 row)
+ 1 | xyzzy
+(3 rows)
 
 CREATE PROCEDURE ptest2()
 LANGUAGE SQL
diff --git a/src/test/regress/sql/create_procedure.sql b/src/test/regress/sql/create_procedure.sql
index 8c47b7e9ef9..d65e568a64e 100644
--- a/src/test/regress/sql/create_procedure.sql
+++ b/src/test/regress/sql/create_procedure.sql
@@ -13,10 +13,12 @@ $$;
 
 SELECT ptest1('x');  -- error
 CALL ptest1('a');  -- ok
+CALL ptest1('xy' || 'zzy');  -- ok, constant-folded arg
+CALL ptest1(substring(random()::text, 1, 1));  -- ok, volatile arg
 
 \df ptest1
 
-SELECT * FROM cp_test ORDER BY a;
+SELECT * FROM cp_test ORDER BY b COLLATE "C";
 
 
 CREATE PROCEDURE ptest2()