Re: Incorrect CHUNKHDRSZ in nodeAgg.c

David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> While reading nodeAgg.c, I noticed code that uses CHUNKHDRSZ to help
> figure out how much memory a tuple uses so the code knows when to
> spill to disk. CHUNKHDRSZ is currently set to 16 bytes, which was
> fine when that code was added, but it's a few years out-of-date since
> c6e0fe1f2 in 2022.

> The attached adjusts the 16 to sizeof(MemoryChunk), which is normally
> 8, but 16 in assert builds.

Yeah, this is more formally correct ...

> The memory accounting should be more accurate with the patch, however,
> that accounting doesn't account for blocks that the chunks are on. For
> that reason, I'm thinking of not backpatching this as it will make
> hash aggregates use a bit more memory than unpatched before spilling,
> albeit, for most cases, closer to the hash_mem limit, which is when
> the spilling should be happening.

Agreed that back-patching isn't appropriate.

I thought for a bit about whether we shouldn't try to account for
palloc power-of-2-block-size overhead here. That omission would
typically be a far larger error than the one you are fixing. However,
given that the inputs to hash_agg_entry_size are only estimates,
I'm not sure that we can hope to do better than the current behavior.

Should tuple hash tables be using a different memory context type
that doesn't impose that power-of-2 overhead? It's only useful
when we expect a fair amount of pfree-and-recycle behavior, but
I think we don't have much retail entry removal in tuple hash
tables. Could we use a generation or even bump context?

regards, tom lane