Re: Memory leak in nodeAgg

Attached is a patch that fixes a gradual memory leak in ExecReScanAgg(),
when the AGG_HASHED strategy is used:

* the aggregation hash table is allocated in a newly-created
sub-context of the agg's aggcontext

* MemoryContextReset() resets the memory allocated in child
contexts, but not the child contexts themselves

* ExecReScanAgg() builds a brand-new hash table, which allocates
a brand-new sub-context, thus leaking the header for the
previous hashtable sub-context

The patch fixes this by using MemoryContextDeleteAndResetChildren(). (I
briefly looked at other call-sites of hash_create() to see if this
problem exists elsewhere, but I didn't see anything obvious.)

We run into the leak quite easily at Truviso; with a sufficiently
long-lived query in vanilla Postgres, you should be able to reproduce
the same problem.

Credit: Sailesh Krishnamurthy at Truviso for diagnosing the cause of the
leak.

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> Attached is a patch that fixes a gradual memory leak in ExecReScanAgg(),
> when the AGG_HASHED strategy is used:

Hmm. Good catch, but I can't help wondering if this is just the tip
of the iceberg. Should *every* MemoryContextReset be
MemoryContextResetAndDeleteChildren?

What this probably boils down to is whether there are cases where we
keep pointers to a sub-context in some place other than the parent
context. I remember thinking there would be cases like that when I
proposed the current memory context API, but now I'm less sure that
it's a good idea.

If we redefined MemoryContextReset to be the same as
MemoryContextResetAndDeleteChildren, it'd be possible to keep the
headers for child contexts in their parent context, thus easing
traffic in TopMemoryContext, and perhaps saving a few pfree cycles
when resetting the parent (since we'd not bother explicitly releasing
the child headers). But that would be a pain to undo if it turned out
wrong.

Anyone want to investigate what happens if we make MemoryContextReset
the same as MemoryContextResetAndDeleteChildren?

regards, tom lane

On Mon, 2007-08-06 at 18:52 -0400, Tom Lane wrote:
> Hmm. Good catch, but I can't help wondering if this is just the tip
> of the iceberg. Should *every* MemoryContextReset be
> MemoryContextResetAndDeleteChildren?

Yeah, the same thought occurred to me. Certainly having the current
behavior as the default is error-prone: it's quite easy to leak child
contexts on Reset. Perhaps we could redefine Reset to mean
ResetAndDeleteChildren, and add another name for the current Reset
functionality. ResetAndPreserveChildren, maybe?

> If we redefined MemoryContextReset to be the same as
> MemoryContextResetAndDeleteChildren, it'd be possible to keep the
> headers for child contexts in their parent context, thus easing
> traffic in TopMemoryContext, and perhaps saving a few pfree cycles
> when resetting the parent

The fact that MemoryContextCreate allocates the context header in
TopMemoryContext has always made me uneasy, so getting rid of that would
be nice. I wonder if there's not at least *one* place that depends on
the current Reset behavior, though...

> Anyone want to investigate what happens if we make MemoryContextReset
> the same as MemoryContextResetAndDeleteChildren?

Sure, I'll take a look, but I'll apply the attached patch in the mean
time (above cleanup is probably 8.4 material anyway).

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> ... Perhaps we could redefine Reset to mean
> ResetAndDeleteChildren, and add another name for the current Reset
> functionality. ResetAndPreserveChildren, maybe?

Yeah, I was considering exactly that as an interim step.

>> Anyone want to investigate what happens if we make MemoryContextReset
>> the same as MemoryContextResetAndDeleteChildren?

> Sure, I'll take a look, but I'll apply the attached patch in the mean
> time (above cleanup is probably 8.4 material anyway).

Probably, given that we've not noticed any major leaks from other places
that might have the same problem.

regards, tom lane

On Mon, 2007-08-06 at 14:21 -0700, Neil Conway wrote:
> Attached is a patch that fixes a gradual memory leak in ExecReScanAgg(),
> when the AGG_HASHED strategy is used

Applied to HEAD, and backpatched back to 7.4.

-Neil

Added to TODO:

* Consider simplifying how memory context resets handle child contexts

http://archives.postgresql.org/pgsql-patches/2007-08/msg00067.php

---------------------------------------------------------------------------

Neil Conway wrote:
> On Mon, 2007-08-06 at 18:52 -0400, Tom Lane wrote:
> > Hmm. Good catch, but I can't help wondering if this is just the tip
> > of the iceberg. Should *every* MemoryContextReset be
> > MemoryContextResetAndDeleteChildren?
>
> Yeah, the same thought occurred to me. Certainly having the current
> behavior as the default is error-prone: it's quite easy to leak child
> contexts on Reset. Perhaps we could redefine Reset to mean
> ResetAndDeleteChildren, and add another name for the current Reset
> functionality. ResetAndPreserveChildren, maybe?
>
> > If we redefined MemoryContextReset to be the same as
> > MemoryContextResetAndDeleteChildren, it'd be possible to keep the
> > headers for child contexts in their parent context, thus easing
> > traffic in TopMemoryContext, and perhaps saving a few pfree cycles
> > when resetting the parent
>
> The fact that MemoryContextCreate allocates the context header in
> TopMemoryContext has always made me uneasy, so getting rid of that would
> be nice. I wonder if there's not at least *one* place that depends on
> the current Reset behavior, though...
>
> > Anyone want to investigate what happens if we make MemoryContextReset
> > the same as MemoryContextResetAndDeleteChildren?
>
> Sure, I'll take a look, but I'll apply the attached patch in the mean
> time (above cleanup is probably 8.4 material anyway).
>
> -Neil
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +