Message-ID: <af12804ead9f0530c3946c849099c5ef35763527.camel@j-davis.com>
Subject: Re: Significant performance issues with array_agg() + HashAggregate
 plans on Postgres 17
From: Jeff Davis <pgsql@j-davis.com>
To: David Rowley <dgrowleyml@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Scott Carey <scott.carey@algonomy.com>,
	pgsql-performance@lists.postgresql.org
Date: Tue, 07 Apr 2026 14:58:22 -0700
In-Reply-To: 
 <CAApHDvo9=fmTwHkw63CU8FJooH8AWFP0RXzHe3X0S2Hr3OL8KA@mail.gmail.com>
References: 
	<CA+vubOGaanYaGGb98etJPeJCbN=RAawx652MdmJVRK8+BeEP0w@mail.gmail.com>
	 <CAApHDvq5fMzhK7OwGOhOgqRF-wErGNsT75yie5LtobSir1HhnA@mail.gmail.com>
	 <CA+vubOE6feEWh7XxoWqdPh634SUVjHKW7s9BgMD755wJTqQFPw@mail.gmail.com>
	 <CA+vubOHgrdkm+3wgC0FPcStDeMS9ZQ_Du9vUXWjxN9MN9CO2uw@mail.gmail.com>
	 <2764544.1774985169@sss.pgh.pa.us>
	 <CA+vubOEbfm5+BxtuFGb_tTj3sSX1u0Y+=zHUxoF1Q9feGpRfNg@mail.gmail.com>
	 <2897039.1775051081@sss.pgh.pa.us>
	 <CA+vubOGqg7f2E0_OW8EMruAObejnCS1gCgguT+cnwHG9bRxMDQ@mail.gmail.com>
	 <3579239.1775151498@sss.pgh.pa.us>
	 <CA+vubOH1mcEbsHnkm5L7aJWR+eeiVpOnGfcdWax9td6pyOkMww@mail.gmail.com>
	 <3671092.1775167393@sss.pgh.pa.us> <3825075.1775244255@sss.pgh.pa.us>
	 <3b569609adc36db30aacbf05e487b472fa8edc84.camel@j-davis.com>
	 <3829071.1775246182@sss.pgh.pa.us>
	 <CAApHDvo9=fmTwHkw63CU8FJooH8AWFP0RXzHe3X0S2Hr3OL8KA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.3-0ubuntu1.1 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/af12804ead9f0530c3946c849099c5ef35763527.camel%40j-davis.com>
Precedence: bulk

On Sat, 2026-04-04 at 13:21 +1300, David Rowley wrote:
> A slight variation on this that I was thinking of would be to
> introduce a MemoryPool struct that could be tagged onto a
> MemoryContext which contains a pool_limit. A child MemoryContext
> would, by default, inherit its parent's MemoryPool. On malloc/free,
> if
> the owning context has a non-null MemoryPool, the MemoryPool's
> memory_allocated is updated. At a safe point in nodeAgg.c, we'd check
> if the pool limit has been reached. I assume there's some simple
> inline function that just checks if memory_allocated is greater than
> pool_limit. Doing it this way would mean there's no need to
> recursively propagate the mentioned child_mem_allocated field up the
> hierarchy, as there is only a single field to update if the
> MemoryPool
> field is set.

I like that idea, because it could also be a good place to hold a max
block size for that tree of contexts. That's important to ensure that
the block size is significantly less than work_mem.

But it also means there's only one pool in any given subtree (unless
you mean that we should make that work somehow), which is an awkward
requirement, especially with MemoryContextSetParent().

Regards,
	Jeff Davis