On 31 ÅÃjen 2013, 15:05, Tom Lane wrote:
> Frank van Vugt <ftm.van.vugt@foxi.nl> writes:
>> Op zondag 20 oktober 2013 12:57:43 schreef Tomas Vondra:
>>> Attached is a quick patch removing the local memory context and using
>>> aggcontext instead.
>
>> It looks like this didn't go into git yet. Will it be in v9.2.6/v9.3.1?
>
> Certainly not. We can consider it for 9.4, if Tomas submits it to the
> commitfest process, but it needs review and testing. In particular
> I'm skeptical that it probably outright breaks some cases (look at
> makeMdArrayResult) and results in substantial added bloat, not savings,
> in others.
Yes, I'll submit it to the next commitfest. And you're right that the
makeMdArrayResult is faulty - it shouldn't really do the
MemoryContextDelete as I reused the context of the aggregation function.
Actually, I'm quite surprised it did not fail, even though my tests were
only really sketchy.
As for the 9.2 / 9.3 branches, I think we should discuss whether the
current state is actually correct. I'm inclined to say that a code that
manages to crash a server with 64GB of virtual memory, although with very
light tweaking can happily complete with ~3.5GB of RAM, is not exactly
right.
It seems to me that the code somehow assumes the accumulated arrays are
going to be quite large - in that case preallocating > 1kB per group is
probably fine. I'm fine with "reasonable" preallocation, but 1kB seems way
too aggressive to me. But it's not possible to use smaller preallocation
because this is determined by the block size check in aset.c.
So I see three options for <9.4 versions:
(a) keeping the current implementation, that crashes with cases like this
(and I don't really like this option, because the code seems broken to me)
(b) keep the local memory context but tune it somehow - this however means
changes in aset.c, which however impacts significant part of the codebase
(not sure how much internal / external code relies on this behavior)
(c) remove the local memory context (I'm really wondering what other
benefit it might have in this particular case) and maybe tune the other
parameters (e.g. the initial size / growth rate of the array etc.).
I'd certainly vote for (c) in this case.
Tomas
