Re: [PATCHES] Memory leak in nodeAgg
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 [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Memory leak in nodeAgg
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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
[PATCHES] 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
Index: src/backend/executor/nodeAgg.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/executor/nodeAgg.c,v
retrieving revision 1.152
diff -p -c -r1.152 nodeAgg.c
*** src/backend/executor/nodeAgg.c 2 Apr 2007 03:49:38 - 1.152
--- src/backend/executor/nodeAgg.c 6 Aug 2007 19:29:11 -
*** ExecReScanAgg(AggState *node, ExprContex
*** 1646,1653
MemSet(econtext-ecxt_aggvalues, 0, sizeof(Datum) * node-numaggs);
MemSet(econtext-ecxt_aggnulls, 0, sizeof(bool) * node-numaggs);
! /* Release all temp storage */
! MemoryContextReset(node-aggcontext);
if (((Agg *) node-ss.ps.plan)-aggstrategy == AGG_HASHED)
{
--- 1646,1657
MemSet(econtext-ecxt_aggvalues, 0, sizeof(Datum) * node-numaggs);
MemSet(econtext-ecxt_aggnulls, 0, sizeof(bool) * node-numaggs);
! /*
! * Release all temp storage. Note that in the AGG_HASHED case the agg
! * hash table is allocated in a sub-context, so we need to use
! * MemoryContextResetAndDeleteChildren() to reclaim that storage.
! */
! MemoryContextResetAndDeleteChildren(node-aggcontext);
if (((Agg *) node-ss.ps.plan)-aggstrategy == AGG_HASHED)
{
---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at
http://www.postgresql.org/about/donate
Re: [PATCHES] Memory leak in nodeAgg
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Memory leak in nodeAgg
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
Re: [PATCHES] Memory leak in nodeAgg
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
