Message-ID: <b2451225-0a10-4ded-abef-4e9efbde7a8e@iki.fi>
Date: Sat, 28 Mar 2026 01:17:13 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Better shared data structure management and resizable shared data
 structures
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>,
 pgsql-hackers <pgsql-hackers@postgresql.org>, chaturvedipalak1911@gmail.com
References: 
 <CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com>
 <CA+TgmoaoSha9KnSdrA7DDMYD9+Uor4OPEaQ88bbEC3QuVE5Mcg@mail.gmail.com>
 <62b8dc23-8f6a-4cac-91ff-f74bb5bc159a@iki.fi>
 <CA+TgmoboxAaS6MCRf__LRhf8Ji3B0O9_=d7KzRS12QhOuocnTQ@mail.gmail.com>
 <c7b4dafc-7a6c-43e4-8b9e-20807396590b@iki.fi>
 <CA+Tgmob7E3CGDoEBB3mwfk+v1CgcOG8yL0VHje2gy9Ts=e1Zxw@mail.gmail.com>
 <f8fc6f77-292d-4225-9f54-382c39da5ebc@iki.fi>
 <CA+TgmobqwCHp1w6xkTJmZiq4C8Pr0O3tNUvSFZcAgYLkvF45bg@mail.gmail.com>
 <8a6799be-bd42-49fb-8914-856c97bb1977@iki.fi>
 <CA+Tgmob8ot7qam7a650nx_4jyEoFLs8s_kPMc1+EB7-dr56=WA@mail.gmail.com>
 <cc3ff976-c303-45cd-9a8d-a90483170e37@iki.fi>
 <CA+TgmoauqYXm8iA3FGRAVKxYShUxWBiS_MSLmQpTrmO7wNHamw@mail.gmail.com>
 <113724ab-0028-493f-9605-6e8570f0939f@iki.fi>
 <CAExHW5uTNWOSxJDWQAUnS0tZawob2_J3dRAtc67NHNZ98X4_xA@mail.gmail.com>
 <CAExHW5t439y61YD9bc7d5wZWHp6J=M43Qu3eEZOBPguZML7o2A@mail.gmail.com>
 <CAExHW5v5FVZbsO9sLzztMZ11C3hgGStE=HkkV2bQkCyncess4w@mail.gmail.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: 
 <CAExHW5v5FVZbsO9sLzztMZ11C3hgGStE=HkkV2bQkCyncess4w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/b2451225-0a10-4ded-abef-4e9efbde7a8e%40iki.fi>
Precedence: bulk

Thanks, will incorporate your comments in next version. Replying to just 
a few of them here:

On 27/03/2026 09:01, Ashutosh Bapat wrote:
> /* Restore basic shared memory pointers */
> if (UsedShmemSegAddr != NULL)
> + {
> InitShmemAllocator(UsedShmemSegAddr);
> + ShmemCallRequestCallbacks();
> 
> It's not clear how we keep the list of registered callbacks across the
> backends and also after restart in-sync. How do we make sure that the
> callbacks registered at this time are the same callbacks registered
> before creating the shared memory? How do we make sure that the
> callbacks registered after the startup are also registered after
> restart?

On Unix systems, the registered callbacks are inherited by fork(), and 
also survive over crash restart. With EXEC_BACKEND, the assumption is 
that calling a library's _PG_init() function will register the same 
callbacks every time. We make the same assumption today with the 
shmem_startup hook.

> +void
> +RegisterShmemCallbacks(const ShmemCallbacks *callbacks)
> ... snip ...
> + foreach(lc, requested_shmem_areas)
> 
> Doesn't this list contain all the areas, not just registered in this
> instance of the call. Does that mean that we need to have all the
> attach functions idempotent? Why can't we deal with the newly
> registered areas only?

registered_shmem_areas is supposed to be empty when the function is 
entered. There's an assertion for that too before the foreach().

However, it's missing this, after processing the list:

         list_free_deep(requested_shmem_areas);
         requested_shmem_areas = NIL;

Because of that, this will fail if you load multiple extensions that 
call RegisterShmemCallbacks() in the same session. Will fix that.

> + /*
> + * Extra space to reserve in the shared memory segment, but it's not part
> + * of the struct itself. This is used for shared memory hash tables that
> + * can grow beyond the initial size when more buckets are allocated.
> + */
> + size_t extra_size;
> 
> When we introduce resizable structures (where even the hash table
> directly itself could be resizable), we will introduce a new field
> max_size which is easy to get confused with extra_size. Maybe we can
> rename extra_size to something like "auxilliary_size" to mean size of
> the auxiliary parts of the structure which are not part of the main
> struct itself.
> 
> + /*
> + * max_size is the estimated maximum number of hashtable entries. This is
> + * not a hard limit, but the access efficiency will degrade if it is
> + * exceeded substantially (since it's used to compute directory size and
> + * the hash table buckets will get overfull).
> + */
> + size_t max_size;
> +
> + /*
> + * init_size is the number of hashtable entries to preallocate. For a
> + * table whose maximum size is certain, this should be equal to max_size;
> + * that ensures that no run-time out-of-shared-memory failures can occur.
> + */
> + size_t init_size;
> 
> Everytime I look at these two fields, I question whether those are the
> number of entries (i.e. size of the hash table) or number of bytes
> (size of the memory). I know it's the former, but it indicates that
> something needs to be changed here, like changing the names to have
> _entries instead of _size, or changing the type to int64 or some such.
> Renaming to _entries would conflict with dynahash APIs since they use
> _size, so maybe the latter?

Agreed.

- Heikki