On Tue, 2014-08-19 at 12:54 +0200, Tomas Vondra wrote:
> The use-case for this is tracking a chosen subtree of contexts - e.g.
> aggcontext and below, so I'd expect the tracked subtrees to be relatively
> shallow. Am I right?
Right.
> My fear is that by removing the inheritance bit, we'll hurt cases with a
> lot of child contexts. For example, array_agg currently creates a separate
> context for each group - what happens if you have 100k groups and do
> MemoryContextGetAllocated? I guess iterating over 100k groups is not free.
Good point. We don't want to make checking the allocated space into an
expensive operation, because then we will be forced to call it less
frequently, which implies that we'd be sloppier about actually fitting
in work_mem.
> Wouldn't the solution with inheritance and propagating the accounting info
> to the parent actually better? Or maybe allowing both, having two flags
> when creating a context instead of one?
That seems overly complicated. We only have one driving use case, so two
orthogonal options sounds excessive. Perhaps one option if we can't
solve the performance problem and we need to isolate the changes to
hashagg.
> Also, do we really need to track allocated bytes - couldn't we track
> kilobytes or something and use smaller data types to get below the 64B?
Good idea.
I attached a patch that uses two uint32 fields so that it doesn't
increase the size of MemoryContextData, and it tracks memory usage for
all contexts. I was unable to detect any performance regression vs.
master, but on my machine the results are noisy.
It would be easier to resolve the performance concern if I could
reliably get the results Robert is getting. I think I was able to
reproduce the regression with the old patch, but the results were still
noisy.
Regards,
Jeff Davis
