From: Jeff Davis <pgsql@j-davis.com>
To: pgsql-bugs@postgresql.org
Subject: HashAgg degenerate case
Date: Tue, 05 Nov 2024 16:59:56 -0800
Message-ID: <a050b6067334cf513bf34f9c7bc925d651160f3a.camel@j-davis.com> (raw)
HashAgg tracks the hash table bucket array (only TupleHashEntryData)
memory differently than the overall hash table (including the
firstTuple) memory.
It can run into a degenerate case when the bucket array grows larger
than the allotted memory for the overall hash table. That, by itself,
shouldn't be a major problem because it would just spill any new groups
and then start over in the next batch.
However, ResetTupleHashTable() calls tuplehash_reset(), and that
preserves the bucket array (just clearing it). That means that the next
time around it's already over the limit, and degenerates to holding a
single tuple.
Unfortunately I don't have a small reproducible case yet. I believe it
would need to be a case where the bucket array is more than half of the
memory limit, and then the hash table needs to grow again (doubling).
The actual case involves parallelism, so maybe that throws off the
estimates in an interesting way.
Fixing it seems fairly easy though: we just need to completely destroy
the hash table each time and recreate it. Something close to the
attached patch (rough).
We might also want to make the degenerate case less painful -- should
we raise the minimum number of groups per batch? One possible problem
would be around groups where the state is large, like ARRAY_AGG(), but
we could have different limits.
Regards,
Jeff Davis
Attachments:v1-0001-HashAgg-completely-rebuild-hash-tables-each-itera.patch (text/x-patch, 1.3K) download
From 775750fd59ef6f6214d41b187c3f1e7b23961520 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 5 Nov 2024 16:52:17 -0800
Subject: [PATCH v1] HashAgg: completely rebuild hash tables each iteration.
---
src/backend/executor/nodeAgg.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.cindex 53ead77ece..9b29d67b7d 100644--- a/src/backend/executor/nodeAgg.c+++ b/src/backend/executor/nodeAgg.c@@ -1469,18 +1469,13 @@ build_hash_tables(AggState *aggstate)
{
int setno;
+ MemoryContextReset(aggstate->hash_metacxt);
for (setno = 0; setno < aggstate->num_hashes; ++setno)
{
AggStatePerHash perhash = &aggstate->perhash[setno];
long nbuckets;
Size memory;
- if (perhash->hashtable != NULL)- {- ResetTupleHashTable(perhash->hashtable);- continue;- }-
Assert(perhash->aggnode->numGroups > 0);
memory = aggstate->hash_mem_limit / aggstate->num_hashes;
@@ -2622,8 +2617,7 @@ agg_refill_hash_table(AggState *aggstate)
/* free memory and reset hash tables */
ReScanExprContext(aggstate->hashcontext);
- for (int setno = 0; setno < aggstate->num_hashes; setno++)- ResetTupleHashTable(aggstate->perhash[setno].hashtable);+ build_hash_tables(aggstate);
aggstate->hash_ngroups_current = 0;
--
2.34.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: pgsql@j-davis.com
Subject: Re: HashAgg degenerate case
In-Reply-To: <a050b6067334cf513bf34f9c7bc925d651160f3a.camel@j-davis.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox