|From:||Tomas Vondra <tv(at)fuzzy(dot)cz>|
|Subject:||Re: 9.5: Better memory accounting, towards memory-bounded HashAgg|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
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
* Parent is already unlinked from context, so can't use
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.
|Next Message||Amit Kapila||2014-08-16 14:27:57||Re: replication commands and log_statements|
|Previous Message||Tomas Vondra||2014-08-16 13:31:08||Re: bad estimation together with large work_mem generates terrible slow hash joins|