Re: 9.5: Better memory accounting, towards memory-bounded HashAgg

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