On 10.8.2014 22:50, Jeff Davis wrote: > On Fri, 2014-08-08 at 01:16 -0700, Jeff Davis wrote: >> Either way, it's better to be conservative. Attached is a version >> of the patch with opt-in memory usage tracking. Child contexts >> inherit the setting from their parent. > > There was a problem with the previous patch: the parent was unlinked > before the delete_context method was called, which meant that the > parent's memory was not being decremented. > > Attached is a fix. It would be simpler to just reorder the operations > in MemoryContextDelete, but there is a comment warning against that. > So, I pass the old parent as an argument to the delete_context > method.
Hi, I did a quick check of the patch, mostly because I wasn't sure whether it allows accounting for a selected subtree only (e.g. aggcontext and it's children). And yes, it does exactly that, which is great. Also, the code seems fine to me, except maybe for this piece in AllocSetDelete: /* * Parent is already unlinked from context, so can't use * update_allocation(). */ while (parent != NULL) { parent->total_allocated -= context->total_allocated; parent = parent->parent; } I believe this should check parent->track_mem, just like update_allocation, because this way it walks all the parent context up to the TopMemoryContext. It does not really break anything (because parents without enabled tracking don't report allocated space), but for plans creating/destroying a lot of contexts (say, GroupAggregate with a lot of groups) this might unnecessarily add overhead. regards Tomas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers