Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 21dcda2

Browse files
committed
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.
1 parent e96b7c6 commit 21dcda2

File tree

2 files changed

+50
-45
lines changed

2 files changed

+50
-45
lines changed

src/pl/plpgsql/src/pl_exec.c

+47-44
Original file line numberDiff line numberDiff line change
@@ -2197,10 +2197,6 @@ exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
21972197
elog(ERROR, "could not open cursor: %s",
21982198
SPI_result_code_string(SPI_result));
21992199

2200-
/* don't need paramlist any more */
2201-
if (paramLI)
2202-
pfree(paramLI);
2203-
22042200
/*
22052201
* If cursor variable was NULL, store the generated portal name in it
22062202
*/
@@ -3169,6 +3165,16 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
31693165
estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums);
31703166
/* caller is expected to fill the datums array */
31713167

3168+
/* initialize ParamListInfo with one entry per datum, all invalid */
3169+
estate->paramLI = (ParamListInfo)
3170+
palloc0(offsetof(ParamListInfoData, params) +
3171+
estate->ndatums * sizeof(ParamExternData));
3172+
estate->paramLI->paramFetch = plpgsql_param_fetch;
3173+
estate->paramLI->paramFetchArg = (void *) estate;
3174+
estate->paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
3175+
estate->paramLI->parserSetupArg = NULL; /* filled during use */
3176+
estate->paramLI->numParams = estate->ndatums;
3177+
31723178
/* set up for use of appropriate simple-expression EState */
31733179
if (simple_eval_estate)
31743180
estate->simple_eval_estate = simple_eval_estate;
@@ -3179,7 +3185,6 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
31793185
estate->eval_processed = 0;
31803186
estate->eval_lastoid = InvalidOid;
31813187
estate->eval_econtext = NULL;
3182-
estate->cur_expr = NULL;
31833188

31843189
estate->err_stmt = NULL;
31853190
estate->err_text = NULL;
@@ -3495,9 +3500,6 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
34953500
(rc == SPI_OK_SELECT) ? errhint("If you want to discard the results of a SELECT, use PERFORM instead.") : 0));
34963501
}
34973502

3498-
if (paramLI)
3499-
pfree(paramLI);
3500-
35013503
return PLPGSQL_RC_OK;
35023504
}
35033505

@@ -3864,8 +3866,6 @@ exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
38643866

38653867
if (curname)
38663868
pfree(curname);
3867-
if (paramLI)
3868-
pfree(paramLI);
38693869

38703870
return PLPGSQL_RC_OK;
38713871
}
@@ -4050,7 +4050,7 @@ exec_assign_c_string(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
40504050

40514051

40524052
/* ----------
4053-
* exec_assign_value Put a value into a target field
4053+
* exec_assign_value Put a value into a target datum
40544054
*
40554055
* Note: in some code paths, this will leak memory in the eval_econtext;
40564056
* we assume that will be cleaned up later by exec_eval_cleanup. We cannot
@@ -4909,8 +4909,6 @@ exec_run_select(PLpgSQL_execstate *estate,
49094909
if (*portalP == NULL)
49104910
elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
49114911
expr->query, SPI_result_code_string(SPI_result));
4912-
if (paramLI)
4913-
pfree(paramLI);
49144912
return SPI_OK_CURSOR;
49154913
}
49164914

@@ -4930,9 +4928,6 @@ exec_run_select(PLpgSQL_execstate *estate,
49304928
estate->eval_processed = SPI_processed;
49314929
estate->eval_lastoid = SPI_lastoid;
49324930

4933-
if (paramLI)
4934-
pfree(paramLI);
4935-
49364931
return rc;
49374932
}
49384933

@@ -5140,7 +5135,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
51405135
LocalTransactionId curlxid = MyProc->lxid;
51415136
CachedPlan *cplan;
51425137
ParamListInfo paramLI;
5143-
PLpgSQL_expr *save_cur_expr;
5138+
void *save_setup_arg;
51445139
MemoryContext oldcontext;
51455140

51465141
/*
@@ -5216,14 +5211,10 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
52165211
}
52175212

52185213
/*
5219-
* Create the param list in econtext's temporary memory context. We won't
5220-
* need to free it explicitly, since it will go away at the next reset of
5221-
* that context.
5222-
*
5223-
* Just for paranoia's sake, save and restore the prior value of
5224-
* estate->cur_expr, which setup_param_list() sets.
5214+
* Set up param list. For safety, save and restore
5215+
* estate->paramLI->parserSetupArg around our use of the param list.
52255216
*/
5226-
save_cur_expr = estate->cur_expr;
5217+
save_setup_arg = estate->paramLI->parserSetupArg;
52275218

52285219
paramLI = setup_param_list(estate, expr);
52295220
econtext->ecxt_param_list_info = paramLI;
@@ -5244,7 +5235,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
52445235
/* Assorted cleanup */
52455236
expr->expr_simple_in_use = false;
52465237

5247-
estate->cur_expr = save_cur_expr;
5238+
estate->paramLI->parserSetupArg = save_setup_arg;
52485239

52495240
if (!estate->readonly_func)
52505241
PopActiveSnapshot();
@@ -5268,6 +5259,15 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
52685259
/*
52695260
* Create a ParamListInfo to pass to SPI
52705261
*
5262+
* We share a single ParamListInfo array across all SPI calls made from this
5263+
* estate. This is generally OK since any given slot in the array would
5264+
* need to contain the same current datum value no matter which query or
5265+
* expression we're evaluating. However, paramLI->parserSetupArg points to
5266+
* the specific PLpgSQL_expr being evaluated. This is not an issue for
5267+
* statement-level callers, but lower-level callers should save and restore
5268+
* estate->paramLI->parserSetupArg just in case there's an active evaluation
5269+
* at an outer call level.
5270+
*
52715271
* We fill in the values for any expression parameters that are plain
52725272
* PLpgSQL_var datums; these are cheap and safe to evaluate, and by setting
52735273
* 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,
52775277
* the expression that might never be evaluated at runtime. To handle those
52785278
* parameters, we set up a paramFetch hook for the executor to call when it
52795279
* wants a not-presupplied value.
5280-
*
5281-
* The result is a locally palloc'd array that should be pfree'd after use;
5282-
* but note it can be NULL.
52835280
*/
52845281
static ParamListInfo
52855282
setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
@@ -5293,22 +5290,28 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
52935290
Assert(expr->plan != NULL);
52945291

52955292
/*
5296-
* Could we re-use these arrays instead of palloc'ing a new one each time?
5297-
* However, we'd have to re-fill the array each time anyway, since new
5298-
* values might have been assigned to the variables.
5293+
* We only need a ParamListInfo if the expression has parameters. In
5294+
* principle we should test with bms_is_empty(), but we use a not-null
5295+
* test because it's faster. In current usage bits are never removed from
5296+
* expr->paramnos, only added, so this test is correct anyway.
52995297
*/
5300-
if (!bms_is_empty(expr->paramnos))
5298+
if (expr->paramnos)
53015299
{
53025300
int dno;
53035301

5304-
paramLI = (ParamListInfo)
5305-
palloc0(offsetof(ParamListInfoData, params) +
5306-
estate->ndatums * sizeof(ParamExternData));
5307-
paramLI->paramFetch = plpgsql_param_fetch;
5308-
paramLI->paramFetchArg = (void *) estate;
5309-
paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
5310-
paramLI->parserSetupArg = (void *) expr;
5311-
paramLI->numParams = estate->ndatums;
5302+
/* Use the common ParamListInfo for all evals in this estate */
5303+
paramLI = estate->paramLI;
5304+
5305+
/*
5306+
* Reset all entries to "invalid". It's pretty annoying to have to do
5307+
* this, but we don't currently track enough information to know which
5308+
* old entries might be obsolete. (There are a couple of nontrivial
5309+
* issues that would have to be dealt with in order to do better here.
5310+
* First, ROW and RECFIELD datums depend on other datums, and second,
5311+
* exec_eval_datum() will return short-lived palloc'd values for ROW
5312+
* and REC datums.)
5313+
*/
5314+
MemSet(paramLI->params, 0, estate->ndatums * sizeof(ParamExternData));
53125315

53135316
/* Instantiate values for "safe" parameters of the expression */
53145317
dno = -1;
@@ -5330,10 +5333,10 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
53305333

53315334
/*
53325335
* Set up link to active expr where the hook functions can find it.
5333-
* Callers must save and restore cur_expr if there is any chance that
5334-
* they are interrupting an active use of parameters.
5336+
* Callers must save and restore parserSetupArg if there is any chance
5337+
* that they are interrupting an active use of parameters.
53355338
*/
5336-
estate->cur_expr = expr;
5339+
paramLI->parserSetupArg = (void *) expr;
53375340

53385341
/*
53395342
* Also make sure this is set before parser hooks need it. There is
@@ -5373,7 +5376,7 @@ plpgsql_param_fetch(ParamListInfo params, int paramid)
53735376

53745377
/* fetch back the hook data */
53755378
estate = (PLpgSQL_execstate *) params->paramFetchArg;
5376-
expr = estate->cur_expr;
5379+
expr = (PLpgSQL_expr *) params->parserSetupArg;
53775380
Assert(params->numParams == estate->ndatums);
53785381

53795382
/*

src/pl/plpgsql/src/plpgsql.h

+3-1
Original file line numberDiff line numberDiff line change
@@ -784,6 +784,9 @@ typedef struct PLpgSQL_execstate
784784
int ndatums;
785785
PLpgSQL_datum **datums;
786786

787+
/* we pass datums[i] to the executor, when needed, in paramLI->params[i] */
788+
ParamListInfo paramLI;
789+
787790
/* EState to use for "simple" expression evaluation */
788791
EState *simple_eval_estate;
789792

@@ -792,7 +795,6 @@ typedef struct PLpgSQL_execstate
792795
uint32 eval_processed;
793796
Oid eval_lastoid;
794797
ExprContext *eval_econtext; /* for executing simple expressions */
795-
PLpgSQL_expr *cur_expr; /* current query/expr being evaluated */
796798

797799
/* status information for error context reporting */
798800
PLpgSQL_stmt *err_stmt; /* current stmt */

0 commit comments

Comments
 (0)