Re: Memory leak in FDW

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.

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