Обсуждение: Memory leak in FDW

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

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

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

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

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

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