Horton

Thread

  1. Memory leak in FDW

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-04-26T18:06:51Z

    Foreign data wrapper's IterateForeignScan() function is supposed to be 
called in a short-lived memory context, but the memory context is 
actually not reset during query execution. That's a pretty bad memory 
leak. I've been testing this with file_fdw and a large file, and "SELECT 
COUNT(*) FROM foreign_table"

Interestingly, if you add any WHERE clause to it, the memory context is 
reset in ExecScan and the leak goes away. This is only a problem with 
the fastpath in ExecScan for the case of no quals and no projections.

The trivial fix is to reset the per-tuple memory context between 
iterations. I tried to look around for other executor nodes that might 
have the same problem. I didn't see any obvious leaks, although index 
scan node seems to call AM's getnext without resetting the memory 
context in between. That's a pretty well-tested codepath, however, and 
there hasn't been any complains of leaks with index scans, so there must 
be something that mitigates it.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  2. Re: Memory leak in FDW

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-04-26T18:15:46Z

    Excerpts from Heikki Linnakangas's message of mar abr 26 15:06:51 -0300 2011:

> I tried to look around for other executor nodes that might 
> have the same problem. I didn't see any obvious leaks, although index 
> scan node seems to call AM's getnext without resetting the memory 
> context in between. That's a pretty well-tested codepath, however, and 
> there hasn't been any complains of leaks with index scans, so there must 
> be something that mitigates it.

Don't we have some rule that functions used in index AMs are supposed to
be leak-free?

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  3. Re: Memory leak in FDW

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-26T18:30:42Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> The trivial fix is to reset the per-tuple memory context between 
> iterations.

Have you tested this with SRFs?

ForeignNext seems like quite the wrong place for resetting
exprcontext in any case ...

			regards, tom lane

  4. Re: Memory leak in FDW

    Greg Stark <gsstark@mit.edu> — 2011-04-26T19:45:25Z

    On Tue, Apr 26, 2011 at 7:15 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Excerpts from Heikki Linnakangas's message of mar abr 26 15:06:51 -0300 2011:
>
>> I tried to look around for other executor nodes that might
>> have the same problem. I didn't see any obvious leaks, although index
>> scan node seems to call AM's getnext without resetting the memory
>> context in between. That's a pretty well-tested codepath, however, and
>> there hasn't been any complains of leaks with index scans, so there must
>> be something that mitigates it.
>
> Don't we have some rule that functions used in index AMs are supposed to
> be leak-free?

btree operators and opclass functions are supposed to be leak-free. I
think other AMs don't try to have the same strictness.


-- 
greg

  5. Re: Memory leak in FDW

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-04-27T08:19:32Z

    On 26.04.2011 21:30, Tom Lane wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  writes:
>> The trivial fix is to reset the per-tuple memory context between
>> iterations.
>
> Have you tested this with SRFs?
>
> ForeignNext seems like quite the wrong place for resetting
> exprcontext in any case ...

ExecScan would be more appropriate I guess (attached).

This means the context will be reset between each tuple even for nodes 
like seqscan that don't use the per-tuple context for anything. 
AllocSetReset exits quickly if there's nothing to do, but it takes a 
couple of function calls to get there. I wouldn't normally worry about 
that, but this is a very hot path for simple queries.

I tested this with:

CREATE TABLE foo AS SELECT generate_series(1,10000000);

I ran "SELECT COUNT(*) FROM foo" many times with \timing on, and took 
the smallest time with and without the patch. I got:

1230 ms with the patch
1186 ms without the patch

This was quite repeatable, it's consistently faster without the patch. 
That's a bigger difference than I expected. Any random code change can 
swing results on micro-benchmarks like this by 1-2%, but that's over 3%. 
Do we care?

I might be getting a bit carried away with this, but we could buy that 
back by moving the isReset flag from AllocSetContext to 
MemoryContextData. That way MemoryContextReset can exit more quickly if 
there's nothing to do, patch attached.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  6. Re: Memory leak in FDW

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-05-21T18:54:37Z

    On 27.04.2011 04:19, Heikki Linnakangas wrote:
> On 26.04.2011 21:30, Tom Lane wrote:
>> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes:
>>> The trivial fix is to reset the per-tuple memory context between
>>> iterations.
>>
>> Have you tested this with SRFs?
>>
>> ForeignNext seems like quite the wrong place for resetting
>> exprcontext in any case ...
>
> ExecScan would be more appropriate I guess (attached).
>
> This means the context will be reset between each tuple even for nodes
> like seqscan that don't use the per-tuple context for anything.
> AllocSetReset exits quickly if there's nothing to do, but it takes a
> couple of function calls to get there. I wouldn't normally worry about
> that, but this is a very hot path for simple queries.
>
> I tested this with:
>
> CREATE TABLE foo AS SELECT generate_series(1,10000000);
>
> I ran "SELECT COUNT(*) FROM foo" many times with \timing on, and took
> the smallest time with and without the patch. I got:
>
> 1230 ms with the patch
> 1186 ms without the patch
>
> This was quite repeatable, it's consistently faster without the patch.
> That's a bigger difference than I expected. Any random code change can
> swing results on micro-benchmarks like this by 1-2%, but that's over 3%.
> Do we care?
>
> I might be getting a bit carried away with this, but we could buy that
> back by moving the isReset flag from AllocSetContext to
> MemoryContextData. That way MemoryContextReset can exit more quickly if
> there's nothing to do, patch attached.

I hear no objections, so committed.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com