Re: Significant performance issues with array_agg() + HashAggregate plans on Postgres 17

On Mon, 2026-04-20 at 16:40 +1200, David Rowley wrote:
> I was maybe wrong about just not bothering to handle
> MemoryContextSetParent(), but I'm not all that sure where the
> complexity is. Shouldn't it just be a matter of:
> 
> If the context has a MemoryPool set, check if the parent has one too,
>    if not just swap parents out as the pool belongs to the context
> that's changing parent.
>    Else, gather memory totals for the swapping context and subtract
> from the MemoryPool, set the context being reparented's pool to NULL
> and change parent.
> else (no pool is set), just swap parent... I think.
> 
> I think there might also need to be a check to see if the new parent
> has a pool and ERROR if it does. Maybe that's the messy part?

Patches attached.

I implemented everything, such that we don't need to ERROR.

It feels slightly over-engineered, but I just didn't like the idea of
erroring on what seem to be valid operations. Given the inheritance
behavior, you may not even be trying to use memory pools, and then
SetParent can still fail, and then what do you do?

Notes:

* It adds 3 extra fields to MemoryContextData inline. The out of line
approaches are not very clean: if we allocate in the context itself
reset will throw it away; if we allocate in the parent context then we
would need to move the allocation on SetParent(); allocating in the
caller means the caller needs to track it even though it has the same
lifetime; and I'm not sure it's a good idea to use malloc() directly.

* The "limit" terminology is a bit awkward because it doesn't really
enforce anything it just adjusts the max block size. Maybe there's a
better term for that?

* allocChunkLimit is not recalculated after SetParent(). I don't think
that's a correctness issue, but I might need to add some more comments.

I like the idea that memory contexts can inherit some information about
work_mem. I've wanted that to be possible for a while, and if we think
this is a good approach then we can expand it to other places in the
executor.

Regards,
	Jeff Davis