MIME-Version: 1.0
References: 
 <CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com>
 <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>
 <b2451225-0a10-4ded-abef-4e9efbde7a8e@iki.fi>
 <CAExHW5tCC0T1ky=Jnq-AvMxa67Adaw7aQ4iQAO=BSdHcbSNBVg@mail.gmail.com>
 <791c3f18-f4de-4d84-ac6b-c7ccc074dd38@iki.fi>
 <CAExHW5tS7GncN90oJWOSzW_3F1EHL9xwe59L7Req3nUVgmObUw@mail.gmail.com>
 <9d919bd9-94dd-4bda-8ccf-ebced4178c53@iki.fi>
 <CAExHW5sYgt=XekOoAE-Tu_Dv8obWZCjCqAPT9vtN2D4m=M=drQ@mail.gmail.com>
 <470e7ebe-0971-49f6-8e46-9b8f6395f88b@iki.fi>
In-Reply-To: <470e7ebe-0971-49f6-8e46-9b8f6395f88b@iki.fi>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Sat, 4 Apr 2026 15:51:09 +0200
Message-ID: 
 <CAEze2Whd_cB4oZwZq-Ar1KgUGiA_gZPPWOvUcw_3LC7sJJB67w@mail.gmail.com>
Subject: Re: Better shared data structure management and resizable shared data
 structures
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
	chaturvedipalak1911@gmail.com
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAEze2Whd_cB4oZwZq-Ar1KgUGiA_gZPPWOvUcw_3LC7sJJB67w%40mail.gmail.com>
Precedence: bulk

On Sat, 4 Apr 2026 at 02:49, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Those are now committed, and here's a new version rebased over those
> changes. The hash options is now called 'nelems', and the 'extra_size'
> in ShmemStructOpts is gone.
>
> Plus a bunch of other fixes and cleanups. I also reordered and
> re-grouped the patches a little, into more logical increments I hope.

0001: LGTM

0002:

> +++ b/src/backend/storage/ipc/shmem.c

> + * Nowadays, there is also a third way to allocate shared memory called

There's no clear indicator of the second way to allocate shared
memory, nor is the first one clearly defined in the new verson of the
comment block.


> + * item is deleted.  However, if one hash table grows very large and then
> + * shrinks, its space cannot be redistributed to other tables.  We could build
> + * a simple hash bucket garbage collector if need be.  Right now, it seems
> + * unnecessary.

I think this new text is outdated, given that we don't have growing
hash tables anymore.

I also think it should've referred to elements, not buckets;
dynahash's buckets cannot readily be deallocated as they're generally
always "in use" (they might be NULL, but they're still accessed in
read operations on missing keys). Elements are put in the freelist if
not used, and those could be released into a memory pool if so desired
(and coded).

> + * In builtin PostgreSQL code, add the callbacks to the list in
> + * src/include/storage/subsystemlist.h.

This refers to an automation system that's introduced a few commits
later, in commit 0005, and therefore probably should be added only in
that commit.

> + * Legacy ShmemInitStruct()/ShmemInitHash() functions
> + * --------------------------------------------------

Should we have checks in place to avoid calls to new APIs from old
callbacks, and vice versa?

> ShmemRequestInternal(...
> +    ShmemRequest *request;
[...]
> +    foreach_ptr(ShmemStructDesc, existing, pending_shmem_requests)
[...]
> +    request = palloc(sizeof(ShmemRequest));
[...]
> +    pending_shmem_requests = lappend(pending_shmem_requests, request);

It looks like you missed my earlier comment about type confusion.
Here, pending_shmem_requests is a List of ShmemRequest pointers, while
the foreach_ptr() uses ShmemStructDesc, which is a type confusion. The
loop checks the 'char *name' field of ShmemStructDesc, which in a
ShmemRequest is the 'ShmemStructDesc *desc'. This bug would cause
issues if different ShmemStructDescs are registered by the same name,
as the ShmemStructDescs wouldn't (necessarily) be strcmp()-equal for
the same name.

> ShmemAttachRequested(void)
> +    /* Call attach callbacks */
> +    foreach(lc, registered_shmem_callbacks)
> +    {
> +        const ShmemCallbacks *callbacks = (const ShmemCallbacks *) lfirst(lc);

This would be more concise with foreach_ptr(const ShmemCallbacks,
callbacks, registered_shmem_callbacks), like in ShmemInitRequested.


> +++ b/src/include/storage/shmem.h
> +/*
> + * Shared memory is reserved and allocated in stages at postmaster startup,
> + * and in EXEC_BACKEND mode, there's some extra work done to "attach" to them
> + * at backend startup.  ShmemCallbacks holds callback functions that are
> + * called at different stages.
> + */
> +typedef struct ShmemCallbacks

Maybe this should also have the opportunity for a (before_)shmem_exit callback?

> + * on-demaind in a backend.  If a subsystem sets this flag, the callbacks are
> + * called immediately after registration, to initialize or attach to the
> + * requested shared memory areas.

Ideally we only immediately call the callbacks if we're under
postmaster, or in a standalone backend; we shouldn't allocate shmem
for some preloaded libraries that set this flag, at least not ahead of
loading all preload libraries.

0003: Maybe this could also test that the protections we're putting in
place against double-registration of shmem areas actually detect the
duplication issue?
Otherwise, LGTM

0004-0014: TBD

While it's mostly mechanical changes, it did make me notice the rather
annoying allocation patterns by XLOGShmemRequest. It allocates various
types of data in one go (which, in principle, is fine) but in doing so
it adds its own alignment tricks etc, and I'm not super stoked about
that. If time allows, could we clean that up?


Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)