Message-ID: <3b569609adc36db30aacbf05e487b472fa8edc84.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: Tom Lane <tgl@sss.pgh.pa.us>, Scott Carey <scott.carey@algonomy.com>
Cc: David Rowley <dgrowleyml@gmail.com>,
	pgsql-performance@lists.postgresql.org
Date: Fri, 03 Apr 2026 12:36:23 -0700
In-Reply-To: <3825075.1775244255@sss.pgh.pa.us>
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>
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/3b569609adc36db30aacbf05e487b472fa8edc84.camel%40j-davis.com>
Precedence: bulk

On Fri, 2026-04-03 at 15:24 -0400, Tom Lane wrote:
> After further study I no longer think there's a leak.=C2=A0 This
> test query involves 50000 GROUP BY groups, and we have an
> array being accumulated for each one.

I was coming to a similar conclusion, though trying to work through the
details=C2=A0of expanded arrays and how the datums are copied around during
aggregation. I don't (yet) see a problem with the correctness of the
memory handling there.

A lot of tiny memory contexts are not ideal, but as long as it's one
per group, that seems within reason.

> So really the whole thing is kind of unsatisfactory.
> I'm not seeing cheap ways to make it better though.
>=20
> A very quick and dirty idea is to tell MemoryContextMemAllocated
> not to recurse, which would restore it to constant-time.=C2=A0 But
> that would make the results much less accurate in cases like
> this one.

One idea would be to update parent contexts' memory totals recursively
each time a subcontext allocates a new block. Block allocations are
infrequent enough that may be acceptable.

If we are worried about affecting unrelated cases, we could set an
"accounting_enabled" flag for the contexts we care about, which would
be automatically inherited by subcontexts, and then stop recursing up
when that flag is false.

Regards,
	Jeff Davis