Re: [PATCHES] MemoryContextStats tweak: show tree structure
On Mon, 2007-08-06 at 20:56 -0400, Tom Lane wrote: > Seems reasonable except I'd vote for less indentation --- the lines are > too long already. How do you feel about 1 or 2 spaces per level? Sure, 2 spaces per level is fine with me (1 makes it a bit hard to see the tree structure, IMHO). -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] MemoryContextStats tweak: show tree structure
Neil Conway <[EMAIL PROTECTED]> writes: > Previously, MemoryContextStats() simply emitted a line of output for > each MemoryContext. This is fine, but it makes it difficult to see the > shape of the MemoryContext hierarchy. Attached is a trivial patch to > indent each context by "4 * level" spaces, where "level" is the depth of > the node within the subtree printed by MemoryContextStats(). Seems reasonable except I'd vote for less indentation --- the lines are too long already. How do you feel about 1 or 2 spaces per level? regards, tom lane ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
[PATCHES] MemoryContextStats tweak: show tree structure
Previously, MemoryContextStats() simply emitted a line of output for
each MemoryContext. This is fine, but it makes it difficult to see the
shape of the MemoryContext hierarchy. Attached is a trivial patch to
indent each context by "4 * level" spaces, where "level" is the depth of
the node within the subtree printed by MemoryContextStats().
For example, suppose we have three contexts beneath TopMemoryContext:
TopMemoryContext (...)
FooContext (...)
BarContext (...)
BazContext (...)
With the patch, these might be printed as:
TopMemoryContext (...)
FooContext (...)
BarContext (...)
BazContext (...)
Assuming that's the parent/child relationship between them, of course.
Obviously this is just for debugging, but I've found it useful while
looking at some memory-related issues. Any comments or objections to
including this in HEAD?
-Neil
Index: source/src/backend/utils/mmgr/aset.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/mmgr/aset.c,v
retrieving revision 1.72
diff -p -c -r1.72 aset.c
*** source/src/backend/utils/mmgr/aset.c 30 Apr 2007 00:12:08 - 1.72
--- source/src/backend/utils/mmgr/aset.c 7 Aug 2007 00:29:41 -
*** static void AllocSetReset(MemoryContext
*** 214,220
static void AllocSetDelete(MemoryContext context);
static Size AllocSetGetChunkSpace(MemoryContext context, void *pointer);
static bool AllocSetIsEmpty(MemoryContext context);
! static void AllocSetStats(MemoryContext context);
#ifdef MEMORY_CONTEXT_CHECKING
static void AllocSetCheck(MemoryContext context);
--- 214,220
static void AllocSetDelete(MemoryContext context);
static Size AllocSetGetChunkSpace(MemoryContext context, void *pointer);
static bool AllocSetIsEmpty(MemoryContext context);
! static void AllocSetStats(MemoryContext context, int level);
#ifdef MEMORY_CONTEXT_CHECKING
static void AllocSetCheck(MemoryContext context);
*** AllocSetIsEmpty(MemoryContext context)
*** 1034,1040
* Displays stats about memory consumption of an allocset.
*/
static void
! AllocSetStats(MemoryContext context)
{
AllocSet set = (AllocSet) context;
long nblocks = 0;
--- 1034,1040
* Displays stats about memory consumption of an allocset.
*/
static void
! AllocSetStats(MemoryContext context, int level)
{
AllocSet set = (AllocSet) context;
long nblocks = 0;
*** AllocSetStats(MemoryContext context)
*** 1044,1049
--- 1044,1050
AllocBlock block;
AllocChunk chunk;
int fidx;
+ int i;
for (block = set->blocks; block != NULL; block = block->next)
{
*** AllocSetStats(MemoryContext context)
*** 1060,1065
--- 1061,1070
freespace += chunk->size + ALLOC_CHUNKHDRSZ;
}
}
+
+ for (i = 0; i < level; i++)
+ fprintf(stderr, "");
+
fprintf(stderr,
"%s: %lu total in %ld blocks; %lu free (%ld chunks); %lu used\n",
set->header.name, totalspace, nblocks, freespace, nchunks,
Index: source/src/backend/utils/mmgr/mcxt.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/mmgr/mcxt.c,v
retrieving revision 1.61
diff -p -c -r1.61 mcxt.c
*** source/src/backend/utils/mmgr/mcxt.c 25 Jul 2007 12:22:52 - 1.61
--- source/src/backend/utils/mmgr/mcxt.c 7 Aug 2007 00:30:26 -
*** MemoryContext CurTransactionContext = NU
*** 49,54
--- 49,56
/* This is a transient link to the active portal's memory context: */
MemoryContext PortalContext = NULL;
+ static void MemoryContextStatsInternal(MemoryContext context, int level);
+
/*
* EXPORTED ROUTINES *
*** MemoryContextIsEmpty(MemoryContext conte
*** 321,336
void
MemoryContextStats(MemoryContext context)
{
MemoryContext child;
AssertArg(MemoryContextIsValid(context));
! (*context->methods->stats) (context);
for (child = context->firstchild; child != NULL; child = child->nextchild)
! MemoryContextStats(child);
}
-
/*
* MemoryContextCheck
* Check all chunks in the named context.
--- 323,343
void
MemoryContextStats(MemoryContext context)
{
+ MemoryContextStatsInternal(context, 0);
+ }
+
+ static void
+ MemoryContextStatsInternal(MemoryContext context, int level)
+ {
MemoryContext child;
AssertArg(MemoryContextIsValid(context));
! (*context->methods->stats) (context, level);
for (child = context->firstchild; child != NULL; child = child->nextchild)
! MemoryContextStatsInternal(child, level + 1);
}
/*
* MemoryContextCheck
* Check all chunks in the named context.
Index: source/src/include/nodes/memnodes.h
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/include/n
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
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: > 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
[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] COPYable logs
Tom Lane wrote:
Andrew Dunstan <[EMAIL PROTECTED]> writes:
I'm looking at doing #1, but I'm not sure where I can sensibly check
that redirection is on if cvslog destination is specified. I could check
when elog() is called, but that seems wasteful. Any ideas?
It's only one extra bool test in elog(), isn't it?
if ((Log_destination & LOG_DESTINATION_CSV) && Redirect_stderr)
Hardly seems worth major contortions to avoid, considering the number of
cycles an elog() call expends anyway.
I thought about adding an assign-hook for Log_destination that forbids
setting the CSV bit unless Redirect_stderr is set, but the trouble with
that is that it's making unsupportable assumptions about the order in
which the GUC variables will be set.
agreed. Creating infrastructure for checking internal consistency of GUC
vars would be a major pain.
After sleeping on it I came to pretty much the conclusion you did,
although I'm testing for redirection_done rather than the GUC setting
directly. My current skeleton looks like this:
if (Log_destination & LOG_DESTINATION_STDERR)
{
if (redirection_done)
{
/* send CSV data down the pipe if it's safe to do so */
write_csvlog(edata);
}
else
{
char * msg = _("Not safe to send CSV data\n");
write(fileno(stderr),msg,strlen(msg));
write(fileno(stderr), buf.data, buf.len);
}
}
cheers
andrew
---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
Re: [PATCHES] COPYable logs
Andrew Dunstan <[EMAIL PROTECTED]> writes: > I'm looking at doing #1, but I'm not sure where I can sensibly check > that redirection is on if cvslog destination is specified. I could check > when elog() is called, but that seems wasteful. Any ideas? It's only one extra bool test in elog(), isn't it? if ((Log_destination & LOG_DESTINATION_CSV) && Redirect_stderr) Hardly seems worth major contortions to avoid, considering the number of cycles an elog() call expends anyway. I thought about adding an assign-hook for Log_destination that forbids setting the CSV bit unless Redirect_stderr is set, but the trouble with that is that it's making unsupportable assumptions about the order in which the GUC variables will be set. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
