Re: Shared hash table allocations

On 3/27/26 23:37, Heikki Linnakangas wrote:
> I'm starting a new thread for this topic that we discussed on the
> "Better shared data structure management and resizable shared data
> structures" thread:
> 
> On 25/03/2026 20:37, Robert Haas wrote:
>> On Sat, Mar 21, 2026 at 8:14 PM Heikki Linnakangas <hlinnaka@iki.fi>
>> wrote:
>>> I wonder if we should set aside a small amount of memory, like 10-20 kB,
>>> in the fixed shmem segment for small structs like that. Currently, such
>>> allocations will usually succeed because we leave some wiggle room, but
>>> the  can also be consumed by other things like locks. But we could
>>> reserve a small amount of memory specifically for small allocations in
>>> extensions like this.
>>
>> Yeah, I don't really understand why we let the lock table use up that
>> space. I mean, I think it would be useful to have a way to let the
>> lock table expand without a server restart, and I also suspect that we
>> could come up with a less-silly data structure than the PROCLOCK hash,
>> but also if the only thing keeping you from running out of lock space
>> is the wiggle room, maybe you just need to bump up
>> max_locks_per_transaction. Like, you could easily burn through the
>> wiggle room, get an error anyway, and then later find that you also
>> now can't load certain extensions without a server restart.
> 
> Attached patch set tightens up all shared memory hash tables so that
> they no longer use the "wiggle room". They are now truly fixed size. You
> specify the number of elements in ShmemInitHash(), and that's it.
> 
> This also addresses the accounting issue we currently have with hash
> tables, that the size reported in pg_shmem_allocations only shows the
> size of fixed header and the directory, not the actual hash buckets.
> They were previously all lumped together in the "<anonymous>" section.
> These patches fix that.
> 
> There was an earlier attempt at that at last year [1], but it got
> reverted. I hope my approach is less invasive: instead of changing
> dynahash.c to use a single allocation directly, I'm providing it an
> "alloc" callback that carves out the different parts it needs from the
> single contiguous shmem area, which in turn is allocated with
> ShmemInitStruct().
> 

Thanks!

That earlier attempt was committed+reverted by me, and I certainly find
this new approach much easier to understand (but I thought I understand
the old patch too ...).

A couple comments, based on a quick review:

* 0001 - seems fine

* 0002 - +1 to getting rid of HASH_SEGMENT, but I don't see the point of
renaming DEF_SEGSIZE to HASH_SEGSIZE. Isn't that a bit unnecessary?

* 0003 - I'd probably rename CurrentDynaHashCxt to something that
doesn't seem like a "global" variable, e.g. "dynahashCxt"

* 0004 - seems fine, +1 to get rid of unused pieces

* 0005 - seems fine

* 0006 - Doesn't this completely change the alignment? ShmemHashAlloc
used to call ShmemAllocRaw, which is very careful to use CACHELINEALIGN.
But now ShmemHashAlloc just does MAXALIGN, which ShmemAllocRaw claims is
not enough on modern systems.

* 0007 - this left one comment referencing HASH_DIRSIZE in dynahash.c

* 0008 - makes sense


regards

-- 
Tomas Vondra