Pavel Stehule <pavel.steh...@gmail.com> writes:
> I am sending another patch that tries to allow CachedPlans for CALL
> statements. I think this patch is very accurate, but it is not nice,
> because it is smudging very precious reference counting for CachedPlans.
I spent some time testing this. Although the #1 patch gets rid of
the major memory leak of cached plans, the original test case still
shows a pretty substantial leak across repeated executions of a CALL.
The reason is that the stanza for rebuilding stmt->target also gets
executed each time through, and that leaks not only the relatively
small PLpgSQL_row datum but also a bunch of catalog lookup cruft
created on the way to building the datum. Basically this code forgot
that plpgsql's outer execution layer can't assume that it's running
in a short-lived context.
I attach a revised #1 that takes care of that problem, and also
cleans up what seems to me to be pretty sloppy thinking in both
the original code and Pavel's #1 patch: we should be restoring
the previous value of expr->plan, not cavalierly assuming that
it was necessarily NULL. I didn't care for looking at the plan's
"saved" field to decide what was happening, either. We really
should have a local flag variable clearly defining which behavior
it is that we're implementing.
With this patch, I see zero memory bloat on Pavel's original example,
even with a much larger repeat count.
I don't like much of anything about plpgsql-stmt_call-fix-2.patch.
It feels confused and poorly documented, possibly because "fragile"
is not a very clear term for whatever property it is you're trying to
attribute to plans. But in any case, I think it's fixing the problem
in the wrong place. I think the right way to fix it probably is to
manage a CALL's saved plan the same as every other plpgsql plan,
but arrange for the transient refcount on that plan to be held by a
ResourceOwner that is not a child of any transaction resowner, but
rather belongs to the procedure's execution and will be released on
the way out of the procedure.
In any case, I doubt we'd risk back-patching either the #2 patch
or any other approach to avoiding the repeat planning. We need a
back-patchable fix that at least tamps down the memory bloat,
and this seems like it'd do.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index d4a3d58daa..7a2f2dfe91 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2145,40 +2145,60 @@ exec_stmt_perform(PLpgSQL_execstate *estate, PLpgSQL_stmt_perform *stmt)
/*
* exec_stmt_call
+ *
+ * NOTE: this is used for both CALL and DO statements.
*/
static int
exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
{
PLpgSQL_expr *expr = stmt->expr;
+ SPIPlanPtr orig_plan = expr->plan;
+ bool local_plan;
+ PLpgSQL_variable *volatile cur_target = stmt->target;
volatile LocalTransactionId before_lxid;
LocalTransactionId after_lxid;
volatile bool pushed_active_snap = false;
volatile int rc;
+ /*
+ * If not in atomic context, we make a local plan that we'll just use for
+ * this invocation, and will free at the end. Otherwise, transaction ends
+ * would cause errors about plancache leaks.
+ *
+ * XXX This would be fixable with some plancache/resowner surgery
+ * elsewhere, but for now we'll just work around this here.
+ */
+ local_plan = !estate->atomic;
+
/* PG_TRY to ensure we clear the plan link, if needed, on failure */
PG_TRY();
{
SPIPlanPtr plan = expr->plan;
ParamListInfo paramLI;
- if (plan == NULL)
+ /*
+ * Make a plan if we don't have one, or if we need a local one. Note
+ * that we'll overwrite expr->plan either way; the PG_TRY block will
+ * ensure we undo that on the way out, if the plan is local.
+ */
+ if (plan == NULL || local_plan)
{
+ /* Don't let SPI save the plan if it's going to be local */
+ exec_prepare_plan(estate, expr, 0, !local_plan);
+ plan = expr->plan;
/*
- * Don't save the plan if not in atomic context. Otherwise,
- * transaction ends would cause errors about plancache leaks.
- *
- * XXX This would be fixable with some plancache/resowner surgery
- * elsewhere, but for now we'll just work around this here.
+ * A CALL should never be a simple expression. (If it could be,
+ * we'd have to worry about saving/restoring the previous values
+ * of the related expr fields, not just expr->plan.)
*/
- exec_prepare_plan(estate, expr, 0, estate->atomic);
+ Assert(!expr->expr_simple_expr);
/*
* The procedure call could end transactions, which would upset
* the snapshot management in SPI_execute*, so don't let it do it.
* Instead, we set the snapshots ourselves below.
*/
- plan = expr->plan;
plan->no_snapshots = true;
/*
@@ -2186,14 +2206,20 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
* case the procedure's argument list has changed.
*/
stmt->target = NULL;
+ cur_target = NULL;
}
/*
* We construct a DTYPE_ROW datum representing the plpgsql variables
* associated with the procedure's output arguments. Then we can use
- * exec_move_row() to do the assignments.
+ * exec_move_row() to do the assignments. If we're using a local
+ * plan, also make a local target; otherwise, since the above code
+ * will force a new plan each time through, we'd repeatedly leak the
+ * memory for the target. (Note: we also leak the target when a plan
+ * change is forced, but that isn't so likely to cause excessive
+ * memory leaks.)
*/
- if (stmt->is_call && stmt->target == NULL)
+ if (stmt->is_call && cur_target == NULL)
{
Node *node;
FuncExpr *funcexpr;
@@ -2208,6 +2234,9 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
int i;
ListCell *lc;
+ /* Use eval_mcontext for any cruft accumulated here */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
+
/*
* Get the parsed CallStmt, and look up the called procedure
*/
@@ -2239,9 +2268,11 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
ReleaseSysCache(func_tuple);
/*
- * Begin constructing row Datum
+ * Begin constructing row Datum; keep it in fn_cxt if it's to be
+ * long-lived.
*/
- oldcontext = MemoryContextSwitchTo(estate->func->fn_cxt);
+ if (!local_plan)
+ MemoryContextSwitchTo(estate->func->fn_cxt);
row = (PLpgSQL_row *) palloc0(sizeof(PLpgSQL_row));
row->dtype = PLPGSQL_DTYPE_ROW;
@@ -2249,7 +2280,8 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
row->lineno = -1;
row->varnos = (int *) palloc(sizeof(int) * list_length(funcargs));
- MemoryContextSwitchTo(oldcontext);
+ if (!local_plan)
+ MemoryContextSwitchTo(get_eval_mcontext(estate));
/*
* Examine procedure's argument list. Each output arg position
@@ -2293,7 +2325,13 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
row->nfields = nfields;
- stmt->target = (PLpgSQL_variable *) row;
+ cur_target = (PLpgSQL_variable *) row;
+
+ /* We can save and re-use the target datum, if it's not local */
+ if (!local_plan)
+ stmt->target = cur_target;
+
+ MemoryContextSwitchTo(oldcontext);
}
paramLI = setup_param_list(estate, expr);
@@ -2316,17 +2354,27 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
PG_CATCH();
{
/*
- * If we aren't saving the plan, unset the pointer. Note that it
- * could have been unset already, in case of a recursive call.
+ * If we are using a local plan, restore the old plan link.
*/
- if (expr->plan && !expr->plan->saved)
- expr->plan = NULL;
+ if (local_plan)
+ expr->plan = orig_plan;
PG_RE_THROW();
}
PG_END_TRY();
- if (expr->plan && !expr->plan->saved)
- expr->plan = NULL;
+ /*
+ * If we are using a local plan, restore the old plan link; then free the
+ * local plan to avoid memory leaks. (Note that the error exit path above
+ * just clears the link without risking calling SPI_freeplan; we expect
+ * that xact cleanup will take care of the mess in that case.)
+ */
+ if (local_plan)
+ {
+ SPIPlanPtr plan = expr->plan;
+
+ expr->plan = orig_plan;
+ SPI_freeplan(plan);
+ }
if (rc < 0)
elog(ERROR, "SPI_execute_plan_with_paramlist failed executing query \"%s\": %s",
@@ -2363,10 +2411,10 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
{
SPITupleTable *tuptab = SPI_tuptable;
- if (!stmt->target)
+ if (!cur_target)
elog(ERROR, "DO statement returned a row");
- exec_move_row(estate, stmt->target, tuptab->vals[0], tuptab->tupdesc);
+ exec_move_row(estate, cur_target, tuptab->vals[0], tuptab->tupdesc);
}
else if (SPI_processed > 1)
elog(ERROR, "procedure call returned more than one row");
