Joe Conway <[email protected]> writes:
> Not the most scientific of tests, but I think a reasonable one:
> ...
> 2.7% performance penalty
I made a slightly different test based on the original example.
This uses generate_series() which is surely faster than a SQL
function, so it shows the problem to a larger degree:
create table t1 as select x from generate_series(1,10000000) x;
vacuum t1;
-- time repeated executions of this:
select count(*) from t1
where exists (select 1 from generate_series(x,x+ (random()*10)::int));
-- and this:
select count(*) from t1
where exists (select 1 from
generate_series(x,x+ (random()*10)::int::text::int));
The first of these test queries doesn't leak memory because the argument
expressions are all pass-by-value; the second one adds some useless text
conversions just to provoke the leak (it eats about 1GB in HEAD).
HEAD, with asserts off:
first query:
Time: 21694.557 ms
Time: 21629.107 ms
Time: 21593.510 ms
second query:
Time: 26698.445 ms
Time: 26699.408 ms
Time: 26751.742 ms
With yesterday's patch:
first query:
Time: 23919.438 ms
Time: 24053.108 ms
Time: 23884.989 ms
... so about 10.7% slower, not good
second query no longer bloats, but:
Time: 29986.850 ms
Time: 29951.179 ms
Time: 29771.533 ms
... almost 12% slower
With the attached patch instead, I get:
first query:
Time: 21017.503 ms
Time: 21113.656 ms
Time: 21107.617 ms
... 2.5% faster??
second query:
Time: 27033.626 ms
Time: 26997.907 ms
Time: 26984.397 ms
... about 1% slower
In the first query, the MemoryContextReset is nearly free since there's
nothing to free (and we've special-cased that path in aset.c). It's
certainly a measurement artifact that it measures out faster, which says
to me that these numbers can only be trusted within a couple percent;
but at least we're not taking much hit in cases where the patch isn't
actually conferring any benefit. For the second query, losing 1% to avoid
memory bloat seems well worthwhile.
Barring objections I'll apply and back-patch this.
regards, tom lane
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index f162e92..bc79e3a 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*************** ExecMakeFunctionResultNoSets(FuncExprSta
*** 2002,2007 ****
--- 2002,2008 ----
Tuplestorestate *
ExecMakeTableFunctionResult(ExprState *funcexpr,
ExprContext *econtext,
+ MemoryContext argContext,
TupleDesc expectedDesc,
bool randomAccess)
{
*************** ExecMakeTableFunctionResult(ExprState *f
*** 2083,2094 ****
/*
* Evaluate the function's argument list.
*
! * Note: ideally, we'd do this in the per-tuple context, but then the
! * argument values would disappear when we reset the context in the
! * inner loop. So do it in caller context. Perhaps we should make a
! * separate context just to hold the evaluated arguments?
*/
argDone = ExecEvalFuncArgs(&fcinfo, fcache->args, econtext);
/* We don't allow sets in the arguments of the table function */
if (argDone != ExprSingleResult)
ereport(ERROR,
--- 2084,2101 ----
/*
* Evaluate the function's argument list.
*
! * We can't do this in the per-tuple context: the argument values
! * would disappear when we reset that context in the inner loop. And
! * the caller's CurrentMemoryContext is typically a query-lifespan
! * context, so we don't want to leak memory there. We require the
! * caller to pass a separate memory context that can be used for this,
! * and can be reset each time through to avoid bloat.
*/
+ MemoryContextReset(argContext);
+ oldcontext = MemoryContextSwitchTo(argContext);
argDone = ExecEvalFuncArgs(&fcinfo, fcache->args, econtext);
+ MemoryContextSwitchTo(oldcontext);
+
/* We don't allow sets in the arguments of the table function */
if (argDone != ExprSingleResult)
ereport(ERROR,
diff --git a/src/backend/executor/nodeFunctionscan.c b/src/backend/executor/nodeFunctionscan.c
index da5d8c1..945a414 100644
*** a/src/backend/executor/nodeFunctionscan.c
--- b/src/backend/executor/nodeFunctionscan.c
***************
*** 28,33 ****
--- 28,34 ----
#include "nodes/nodeFuncs.h"
#include "parser/parsetree.h"
#include "utils/builtins.h"
+ #include "utils/memutils.h"
/*
*************** FunctionNext(FunctionScanState *node)
*** 94,99 ****
--- 95,101 ----
node->funcstates[0].tstore = tstore =
ExecMakeTableFunctionResult(node->funcstates[0].funcexpr,
node->ss.ps.ps_ExprContext,
+ node->argcontext,
node->funcstates[0].tupdesc,
node->eflags & EXEC_FLAG_BACKWARD);
*************** FunctionNext(FunctionScanState *node)
*** 152,157 ****
--- 154,160 ----
fs->tstore =
ExecMakeTableFunctionResult(fs->funcexpr,
node->ss.ps.ps_ExprContext,
+ node->argcontext,
fs->tupdesc,
node->eflags & EXEC_FLAG_BACKWARD);
*************** ExecInitFunctionScan(FunctionScan *node,
*** 515,520 ****
--- 518,536 ----
ExecAssignResultTypeFromTL(&scanstate->ss.ps);
ExecAssignScanProjectionInfo(&scanstate->ss);
+ /*
+ * Create a memory context that ExecMakeTableFunctionResult 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 just
+ * need one context, because we evaluate each function separately.
+ */
+ scanstate->argcontext = AllocSetContextCreate(CurrentMemoryContext,
+ "Table function arguments",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+
return scanstate;
}
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 5e4a15c..239aff3 100644
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
*************** extern Datum GetAttributeByName(HeapTupl
*** 231,236 ****
--- 231,237 ----
bool *isNull);
extern Tuplestorestate *ExecMakeTableFunctionResult(ExprState *funcexpr,
ExprContext *econtext,
+ MemoryContext argContext,
TupleDesc expectedDesc,
bool randomAccess);
extern Datum ExecEvalExprSwitchContext(ExprState *expression, ExprContext *econtext,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 1f7c6d1..b271f21 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct SubqueryScanState
*** 1407,1412 ****
--- 1407,1413 ----
* nfuncs number of functions being executed
* funcstates per-function execution states (private in
* nodeFunctionscan.c)
+ * argcontext memory context to evaluate function arguments in
* ----------------
*/
struct FunctionScanPerFuncState;
*************** typedef struct FunctionScanState
*** 1421,1426 ****
--- 1422,1428 ----
int nfuncs;
struct FunctionScanPerFuncState *funcstates; /* array of length
* nfuncs */
+ MemoryContext argcontext;
} FunctionScanState;
/* ----------------
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
