Thread: 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
Attachment
Neil Conway <neilc@samurai.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@samurai.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@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +