Re: Incorrect CHUNKHDRSZ in nodeAgg.c

On Thu, 9 Jan 2025 at 09:50, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Attached POC patch, which reduces memory usage by ~15% for a simple
> distinct query on an integer key. Performance is the same or perhaps a
> hair faster.
>
> It's not many lines of code, but the surrounding code might benefit
> from some refactoring which would make it a bit simpler.

Thanks for working on this. Here's a preliminary review:

Since bump.c does not add headers to the palloc'd chunks, I think the
following code from hash_agg_entry_size() shouldn't be using
CHUNKHDRSZ anymore.

tupleChunkSize = CHUNKHDRSZ + tupleSize;

if (pergroupSize > 0)
pergroupChunkSize = CHUNKHDRSZ + pergroupSize;
else
pergroupChunkSize = 0;

You should be able to get rid of pergroupChunkSize and just use
pergroupSize in the return.

I did some benchmarking using the attached script. There's a general
speedup, but I saw some unexpected increase in the number of batches
with the patched version on certain tests. See the attached results.
For example, the work_mem = 8MB with 10 million rows shows "Batches:
129" on master but "Batches: 641" with the patched version. I didn't
check why.

David