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

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 5d94c1ca27cfa63b8932df379eb64efce1fb040a..a027b19744aa1af004f752cfcfe7a57ae0de2aeb 100644 (file)
--- 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 ccad5c87d5eca446c40fd4732f6f1b618677975a..873907dc4348351bea4a3517f1a41c253985ef80 100644 (file)
--- 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 8c47b7e9ef9a159afef0b64d9765ee209da66dd3..d65e568a64ed87939a011bd5648dc39467fc9370 100644 (file)
--- 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()