On 7/29/22 20:23, Nathan Bossart wrote:
> On Fri, Jul 29, 2022 at 01:55:10PM -0400, Tom Lane wrote:
>> Nathan Bossart <nathandbossart@gmail.com> writes:
>>> On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote:
>>>> slab.c
>>>> does not in SlabContextCreate(). Is this intentional, it seems to be an
>>>> oversight to me.
>>
>>> IIUC this is because the header is tracked separately from the first
>>> regular block, unlike aset.c.
>>
>> That doesn't make it not an oversight, though. It looks like aset.c
>> thinks that mem_allocated includes all the context's overhead, whereas
>> this implementation doesn't seem to have that result. The comments
>> associated with mem_allocated are sufficiently vague that it's impossible
>> to tell which implementation is correct. Maybe we don't really care,
>> but ...
>
> Hm. mmgr/README indicates the following note about mem_allocated:
>
> * inquire about the total amount of memory allocated to the context
> (the raw memory from which the context allocates chunks; not the
> chunks themselves)
>
> AFAICT MemoryContextMemAllocated() is only used for determining when to
> spill to disk for hash aggegations at the moment. I don't know whether I'd
> classify this as an oversight or if it even makes any meaningful
> difference, but consistency among the different implementations is probably
> desirable either way. So, I guess I'm +1 for including the memory context
> header in mem_allocated in this case.
>
I don't think this can make meaningful difference - as you mention, we
only really use this to decide when to spill to disk etc. So maybe
you'll spill a bit sooner, but the work_mem is pretty crude threshold
anyway, people don't tune it to an exact byte value (which would be
pretty futile anyway).
OTOH it does seem like an oversight, or at least an inconsistency with
the two other contexts, so if anyone feels like tweaking it ...
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company