Allocate ParamListInfo once per plpgsql function, not once per expression.

setup_param_list() was allocating a fresh ParamListInfo for each query or
expression evaluation requested by a plpgsql function.  There was probably
once good reason to do it like that, but for a long time we've had a
convention that there's a one-to-one mapping between the function's
PLpgSQL_datum array and the ParamListInfo slots, which means that a single
ParamListInfo can serve all the function's evaluation requests: the data
that would need to be passed is the same anyway.

In this patch, we retain the pattern of zeroing out the ParamListInfo
contents during each setup_param_list() call, because some of the slots may
be stale and we don't know exactly which ones.  So this patch only saves a
palloc/pfree per evaluation cycle and nothing more; still, that seems to be
good for a couple percent overall speedup on simple-arithmetic type
statements.  In future, though, we might be able to improve matters still
more by managing the param array contents more carefully.

Also, unify the former use of estate->cur_expr with that of
paramLI->parserSetupArg; they both were used to point to the active
expression, so we can combine the variables into just one.

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index f24f55a2ff43bcffd83e7639809e80f162650d89..0ad32f72e9ab957fb4e7ec983d63eceefc5e869b 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2197,10 +2197,6 @@ exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
                elog(ERROR, "could not open cursor: %s",
                         SPI_result_code_string(SPI_result));
 
-       /* don't need paramlist any more */
-       if (paramLI)
-               pfree(paramLI);
-
        /*
         * If cursor variable was NULL, store the generated portal name in it
         */
@@ -3169,6 +3165,16 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
        estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums);
        /* caller is expected to fill the datums array */
 
+       /* initialize ParamListInfo with one entry per datum, all invalid */
+       estate->paramLI = (ParamListInfo)
+               palloc0(offsetof(ParamListInfoData, params) +
+                               estate->ndatums * sizeof(ParamExternData));
+       estate->paramLI->paramFetch = plpgsql_param_fetch;
+       estate->paramLI->paramFetchArg = (void *) estate;
+       estate->paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
+       estate->paramLI->parserSetupArg = NULL;         /* filled during use */
+       estate->paramLI->numParams = estate->ndatums;
+
        /* set up for use of appropriate simple-expression EState */
        if (simple_eval_estate)
                estate->simple_eval_estate = simple_eval_estate;
@@ -3179,7 +3185,6 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
        estate->eval_processed = 0;
        estate->eval_lastoid = InvalidOid;
        estate->eval_econtext = NULL;
-       estate->cur_expr = NULL;
 
        estate->err_stmt = NULL;
        estate->err_text = NULL;
@@ -3495,9 +3500,6 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
                                         (rc == SPI_OK_SELECT) ? errhint("If you want to discard the results of a SELECT, use PERFORM instead.") : 0));
        }
 
-       if (paramLI)
-               pfree(paramLI);
-
        return PLPGSQL_RC_OK;
 }
 
@@ -3864,8 +3866,6 @@ exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
 
        if (curname)
                pfree(curname);
-       if (paramLI)
-               pfree(paramLI);
 
        return PLPGSQL_RC_OK;
 }
@@ -4050,7 +4050,7 @@ exec_assign_c_string(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
 
 
 /* ----------
- * exec_assign_value                   Put a value into a target field
+ * exec_assign_value                   Put a value into a target datum
  *
  * Note: in some code paths, this will leak memory in the eval_econtext;
  * we assume that will be cleaned up later by exec_eval_cleanup.  We cannot
@@ -4909,8 +4909,6 @@ exec_run_select(PLpgSQL_execstate *estate,
                if (*portalP == NULL)
                        elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
                                 expr->query, SPI_result_code_string(SPI_result));
-               if (paramLI)
-                       pfree(paramLI);
                return SPI_OK_CURSOR;
        }
 
@@ -4930,9 +4928,6 @@ exec_run_select(PLpgSQL_execstate *estate,
        estate->eval_processed = SPI_processed;
        estate->eval_lastoid = SPI_lastoid;
 
-       if (paramLI)
-               pfree(paramLI);
-
        return rc;
 }
 
@@ -5140,7 +5135,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
        LocalTransactionId curlxid = MyProc->lxid;
        CachedPlan *cplan;
        ParamListInfo paramLI;
-       PLpgSQL_expr *save_cur_expr;
+       void       *save_setup_arg;
        MemoryContext oldcontext;
 
        /*
@@ -5216,14 +5211,10 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
        }
 
        /*
-        * Create the param list in econtext's temporary memory context. We won't
-        * need to free it explicitly, since it will go away at the next reset of
-        * that context.
-        *
-        * Just for paranoia's sake, save and restore the prior value of
-        * estate->cur_expr, which setup_param_list() sets.
+        * Set up param list.  For safety, save and restore
+        * estate->paramLI->parserSetupArg around our use of the param list.
         */
-       save_cur_expr = estate->cur_expr;
+       save_setup_arg = estate->paramLI->parserSetupArg;
 
        paramLI = setup_param_list(estate, expr);
        econtext->ecxt_param_list_info = paramLI;
@@ -5244,7 +5235,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
        /* Assorted cleanup */
        expr->expr_simple_in_use = false;
 
-       estate->cur_expr = save_cur_expr;
+       estate->paramLI->parserSetupArg = save_setup_arg;
 
        if (!estate->readonly_func)
                PopActiveSnapshot();
@@ -5268,6 +5259,15 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 /*
  * Create a ParamListInfo to pass to SPI
  *
+ * We share a single ParamListInfo array across all SPI calls made from this
+ * estate.  This is generally OK since any given slot in the array would
+ * need to contain the same current datum value no matter which query or
+ * expression we're evaluating.  However, paramLI->parserSetupArg points to
+ * the specific PLpgSQL_expr being evaluated.  This is not an issue for
+ * statement-level callers, but lower-level callers should save and restore
+ * estate->paramLI->parserSetupArg just in case there's an active evaluation
+ * at an outer call level.
+ *
  * We fill in the values for any expression parameters that are plain
  * PLpgSQL_var datums; these are cheap and safe to evaluate, and by setting
  * them with PARAM_FLAG_CONST flags, we allow the planner to use those values
@@ -5277,9 +5277,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
  * the expression that might never be evaluated at runtime.  To handle those
  * parameters, we set up a paramFetch hook for the executor to call when it
  * wants a not-presupplied value.
- *
- * The result is a locally palloc'd array that should be pfree'd after use;
- * but note it can be NULL.
  */
 static ParamListInfo
 setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
@@ -5293,22 +5290,28 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
        Assert(expr->plan != NULL);
 
        /*
-        * Could we re-use these arrays instead of palloc'ing a new one each time?
-        * However, we'd have to re-fill the array each time anyway, since new
-        * values might have been assigned to the variables.
+        * We only need a ParamListInfo if the expression has parameters.  In
+        * principle we should test with bms_is_empty(), but we use a not-null
+        * test because it's faster.  In current usage bits are never removed from
+        * expr->paramnos, only added, so this test is correct anyway.
         */
-       if (!bms_is_empty(expr->paramnos))
+       if (expr->paramnos)
        {
                int                     dno;
 
-               paramLI = (ParamListInfo)
-                       palloc0(offsetof(ParamListInfoData, params) +
-                                       estate->ndatums * sizeof(ParamExternData));
-               paramLI->paramFetch = plpgsql_param_fetch;
-               paramLI->paramFetchArg = (void *) estate;
-               paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
-               paramLI->parserSetupArg = (void *) expr;
-               paramLI->numParams = estate->ndatums;
+               /* Use the common ParamListInfo for all evals in this estate */
+               paramLI = estate->paramLI;
+
+               /*
+                * Reset all entries to "invalid".  It's pretty annoying to have to do
+                * this, but we don't currently track enough information to know which
+                * old entries might be obsolete.  (There are a couple of nontrivial
+                * issues that would have to be dealt with in order to do better here.
+                * First, ROW and RECFIELD datums depend on other datums, and second,
+                * exec_eval_datum() will return short-lived palloc'd values for ROW
+                * and REC datums.)
+                */
+               MemSet(paramLI->params, 0, estate->ndatums * sizeof(ParamExternData));
 
                /* Instantiate values for "safe" parameters of the expression */
                dno = -1;
@@ -5330,10 +5333,10 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
 
                /*
                 * Set up link to active expr where the hook functions can find it.
-                * Callers must save and restore cur_expr if there is any chance that
-                * they are interrupting an active use of parameters.
+                * Callers must save and restore parserSetupArg if there is any chance
+                * that they are interrupting an active use of parameters.
                 */
-               estate->cur_expr = expr;
+               paramLI->parserSetupArg = (void *) expr;
 
                /*
                 * Also make sure this is set before parser hooks need it.  There is
@@ -5373,7 +5376,7 @@ plpgsql_param_fetch(ParamListInfo params, int paramid)
 
        /* fetch back the hook data */
        estate = (PLpgSQL_execstate *) params->paramFetchArg;
-       expr = estate->cur_expr;
+       expr = (PLpgSQL_expr *) params->parserSetupArg;
        Assert(params->numParams == estate->ndatums);
 
        /*

diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 4ec462838f06b26c76bc9605dacd99fce01910a4..66d4da61d100a5884e68ed0cdb4a3e140802ecaf 100644 (file)
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -784,6 +784,9 @@ typedef struct PLpgSQL_execstate
        int                     ndatums;
        PLpgSQL_datum **datums;
 
+       /* we pass datums[i] to the executor, when needed, in paramLI->params[i] */
+       ParamListInfo paramLI;
+
        /* EState to use for "simple" expression evaluation */
        EState     *simple_eval_estate;
 
@@ -792,7 +795,6 @@ typedef struct PLpgSQL_execstate
        uint32          eval_processed;
        Oid                     eval_lastoid;
        ExprContext *eval_econtext; /* for executing simple expressions */
-       PLpgSQL_expr *cur_expr;         /* current query/expr being evaluated */
 
        /* status information for error context reporting */
        PLpgSQL_stmt *err_stmt;         /* current stmt */