Re: [bug fix] Memory leak in dblink - Mailing list pgsql-hackers

Joe Conway <mail@joeconway.com> 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;

  /* ----------------