Re: Memory leak in FDW

On 27.04.2011 04:19, Heikki Linnakangas wrote:
> On 26.04.2011 21:30, Tom Lane wrote:
>> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)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