From 5d4790e3aaf7da1cfb9a74c0917ce0fe14dfb261 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 6 Oct 2017 20:55:13 -0700 Subject: [PATCH] Reduce memory usage of targetlist SRFs. Previously nodeProjectSet only released memory once per input tuple, rather than once per returned tuple. If the computation of an individual returned tuple requires a lot of memory, that could be a lot. Instead change things so that the expression context can be reset once per output tuple, which requires a new memory context to store SRF arguments in. This is a longstanding issue, but was hard to fix before 9.6, due to the way tSRFs where evaluated. But it's fairly easy to fix now. Reported-By: Lucas Fairchild Author: Andres Freund, per discussion with Tom Lane Discussion: https://postgr.es/m/4514.1507318623@sss.pgh.pa.us --- src/backend/executor/execSRF.c | 31 ++++++++++++++++++++++++++++--- src/backend/executor/nodeProjectSet.c | 32 +++++++++++++++++++++++++++----- src/include/executor/executor.h | 1 + src/include/nodes/execnodes.h | 1 + 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/backend/executor/execSRF.c b/src/backend/executor/execSRF.c index 1be683db83d..cce771d4bea 100644 --- a/src/backend/executor/execSRF.c +++ b/src/backend/executor/execSRF.c @@ -467,13 +467,16 @@ ExecInitFunctionResultSet(Expr *expr, * function itself. The argument expressions may not contain set-returning * functions (the planner is supposed to have separated evaluation for those). * - * This should be called in a short-lived (per-tuple) context. + * This should be called in a short-lived (per-tuple) context, argContext + * needs to live until all rows have been returned (i.e. *isDone set to + * ExprEndResult or ExprSingleResult). * * This is used by nodeProjectSet.c. */ Datum ExecMakeFunctionResultSet(SetExprState *fcache, ExprContext *econtext, + MemoryContext argContext, bool *isNull, ExprDoneCond *isDone) { @@ -497,8 +500,21 @@ restart: */ if (fcache->funcResultStore) { - if (tuplestore_gettupleslot(fcache->funcResultStore, true, false, - fcache->funcResultSlot)) + TupleTableSlot *slot = fcache->funcResultSlot; + MemoryContext oldContext; + bool foundTup; + + /* + * Have to make sure tuple in slot lives long enough, otherwise + * clearing the slot could end up trying to free something already + * freed. + */ + oldContext = MemoryContextSwitchTo(slot->tts_mcxt); + foundTup = tuplestore_gettupleslot(fcache->funcResultStore, true, false, + fcache->funcResultSlot); + MemoryContextSwitchTo(oldContext); + + if (foundTup) { *isDone = ExprMultipleResult; if (fcache->funcReturnsTuple) @@ -526,11 +542,20 @@ restart: * function manager. We skip the evaluation if it was already done in the * previous call (ie, we are continuing the evaluation of a set-valued * function). Otherwise, collect the current argument values into fcinfo. + * + * The arguments have to live in a context that lives at least until all + * rows from this SRF have been returned, otherwise ValuePerCall SRFs + * would reference freed memory after the first returned row. */ fcinfo = &fcache->fcinfo_data; arguments = fcache->args; if (!fcache->setArgsValid) + { + MemoryContext oldContext = MemoryContextSwitchTo(argContext); + ExecEvalFuncArgs(fcinfo, arguments, econtext); + MemoryContextSwitchTo(oldContext); + } else { /* Reset flag (we may set it again below) */ diff --git a/src/backend/executor/nodeProjectSet.c b/src/backend/executor/nodeProjectSet.c index 68981296f90..41e89275f5f 100644 --- a/src/backend/executor/nodeProjectSet.c +++ b/src/backend/executor/nodeProjectSet.c @@ -52,6 +52,13 @@ ExecProjectSet(PlanState *pstate) econtext = node->ps.ps_ExprContext; + /* + * Reset per-tuple context to free expression-evaluation storage allocated + * for a potentially previously returned tuple. Note that the SRF argument + * context has a different lifetime and is reset below. + */ + ResetExprContext(econtext); + /* * Check to see if we're still projecting out tuples from a previous scan * tuple (because there is a function-returning-set in the projection @@ -66,11 +73,13 @@ ExecProjectSet(PlanState *pstate) } /* - * Reset per-tuple memory context to free any expression evaluation - * storage allocated in the previous tuple cycle. Note this can't happen - * until we're done projecting out tuples from a scan tuple. + * Reset argument context to free any expression evaluation storage + * allocated in the previous tuple cycle. Note this can't happen until + * we're done projecting out tuples from a scan tuple, as ValuePerCall + * functions are allowed to reference the arguments for each returned + * tuple. */ - ResetExprContext(econtext); + MemoryContextReset(node->argcontext); /* * Get another input tuple and project SRFs from it. @@ -164,7 +173,8 @@ ExecProjectSRF(ProjectSetState *node, bool continuing) * Evaluate SRF - possibly continuing previously started output. */ *result = ExecMakeFunctionResultSet((SetExprState *) elem, - econtext, isnull, isdone); + econtext, node->argcontext, + isnull, isdone); if (*isdone != ExprEndResult) hasresult = true; @@ -291,6 +301,18 @@ ExecInitProjectSet(ProjectSet *node, EState *estate, int eflags) off++; } + + /* + * Create a memory context that ExecMakeFunctionResult can use to evaluate + * function arguments in. We can't use the per-tuple context for this + * because it gets reset too often; but we don't want to leak evaluation + * results into the query-lifespan context either. We use one context for + * arguments, as they have roughly equivalent lifetimes. + */ + state->argcontext = AllocSetContextCreate(CurrentMemoryContext, + "tSRF function arguments", + ALLOCSET_DEFAULT_SIZES); + return state; } diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 770881849cf..37fd6b2700a 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -400,6 +400,7 @@ extern SetExprState *ExecInitFunctionResultSet(Expr *expr, ExprContext *econtext, PlanState *parent); extern Datum ExecMakeFunctionResultSet(SetExprState *fcache, ExprContext *econtext, + MemoryContext argContext, bool *isNull, ExprDoneCond *isDone); diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index c6d3021c859..c46113444fa 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -946,6 +946,7 @@ typedef struct ProjectSetState ExprDoneCond *elemdone; /* array of per-SRF is-done states */ int nelems; /* length of elemdone[] array */ bool pending_srf_tuples; /* still evaluating srfs in tlist? */ + MemoryContext argcontext; /* context for SRF arguments */ } ProjectSetState; /* ---------------- -- 2.14.1.536.g6867272d5b.dirty