From 34d52e6d3daa7dfeecddc32be91f7d5ee4dcb92f Mon Sep 17 00:00:00 2001 From: David Rowley Date: Wed, 28 Oct 2026 09:06:30 +1300 Subject: [PATCH] Reintroduce hash_mem cap on buckets in BuildTupleHashTable 8f29467c5 removed the hash_mem cap from the nbuckets calculation in BuildTupleHashTable() and put the trust into the planner to provide that function with a sane value, which it would only cap at 2^32. There are various places in the planner which cannot provide good estimates here and it's been demonstrated that it's not difficult to craft a query which requests 2^32 buckets. Here we reintroduce the sanity checks so we don't end up making the hash table's initial size to something that would exceed hash_mem. This may cause the hash table to have to grow several times before it fills, but that seems much better than having the executor OOM due to the request being too large. Users are also used to work_mem and hash_mem_multiplier having an effect on query performance, so this shouldn't be too surprising. It also restores the limits that were imposed pre-8f29467c5, which were tried and tested. Bug: #19759 Reported-by: Alperen Keles Author: David Rowley Reviewed-by: shihao zhong Discussion: https://postgr.es/m/19759-acd3f33402e54f36@postgresql.org Backpatch-through: 19 --- src/backend/executor/execGrouping.c | 35 ++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c index 948aa2f177a..2296506e4ee 100644 --- a/src/backend/executor/execGrouping.c +++ b/src/backend/executor/execGrouping.c @@ -203,18 +203,37 @@ BuildTupleHashTable(PlanState *parent, /* * tuplehash_create requires a uint32 element count, so we had better - * clamp the given nelements to fit in that. As long as we have to do - * that, we might as well protect against completely insane input like - * zero or NaN. But it is not our job here to enforce issues like staying - * within hash_mem: the caller should have done that, and we don't have - * enough info to second-guess. + * clamp the given nelements to fit in that. We'd also better do + * something sane when faced with a zero or NaN estimate. There are + * various reasons why the planner might not be able to provide a good + * estimate here; for example, recursive CTEs don't base their estimate + * on anything reliable and can yield some rather large estimates. Here + * we cap nbuckets to hash_mem to help prevent insane memory allocations. + * The hash table can still grow when the bucket count is insufficient. + * That will make the query a little slower, but that's better than + * outright failing due to OOM if we were to request an insane bucket + * count. Queries being slightly slower with smaller work_mem values is + * also aligned to user expectations. We purposefully don't include the + * tuple width in this calculation, as that would make the cap depend on + * planner estimates for the tuple width, which could also be unreliable. */ if (isnan(nelements) || nelements <= 0) nbuckets = 1; - else if (nelements >= PG_UINT32_MAX) - nbuckets = PG_UINT32_MAX; else - nbuckets = (uint32) nelements; + { + size_t entrysize; + size_t hash_mem_limit; + + if (nelements >= PG_UINT32_MAX) + nbuckets = PG_UINT32_MAX; + else + nbuckets = (uint32) nelements; + + entrysize = sizeof(TupleHashEntryData) + additionalsize; + hash_mem_limit = get_hash_memory_limit() / entrysize; + if (nbuckets > hash_mem_limit) + nbuckets = hash_mem_limit; + } /* tuplescxt must be separate, else ResetTupleHashTable breaks things */ Assert(metacxt != tuplescxt);