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

On Fri, 2014-08-29 at 10:12 -0400, Tom Lane wrote:
> What about removing the callback per se and just keeping the argument,
> as it were. That is, a MemoryContext has a field of type "size_t *"
> that is normally NULL, but if set to non-NULL, then we increment the
> pointed-to value for pallocs and decrement for pfrees.

I like that idea; patch attached. Two differences:
* At block level, not chunk level.
* I use a uint64, because a size_t is only guaranteed to hold one
allocation, and we need to sum up many allocations.

When unused, it does still appear to have a little overhead in Robert's
test on his power machine. It seems to be between a 0.5-1.0% regression.
There's not much extra code on that path, just a branch, pointer
dereference, and an addition; so I don't think it will get much cheaper
than it is.

This regression seems harder to reproduce on my workstation, or perhaps
it's just noisier.

> One thought in either case is that we don't have to touch the API for
> MemoryContextCreate: rather, the feature can be enabled in a separate
> call after making the context.

That seems fairly awkward to me because the pointer needs to be
inherited from the parent context when not specified. I left the extra
API call in.

The inheritance is awkward anyway, though. If you create a tracked
context as a child of an already-tracked context, allocations in the
newer one won't count against the original. I don't see a way around
that without introducing even more performance problems.

If this patch is unacceptable, my only remaining idea is to add the
memory only for the current context with no inheritance (thereby
eliminating the branch, also). That will require HashAgg to traverse all
the child contexts to check whether the memory limit has been exceeded.
As Tomas pointed out, that could be a lot of work in the case of
array_agg with many groups.

Regards,
Jeff Davis