Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.2\))
Subject: Re: Better shared data structure management and resizable shared data
 structures
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <113724ab-0028-493f-9605-6e8570f0939f@iki.fi>
Date: Thu, 26 Mar 2026 19:31:12 +0100
Cc: Robert Haas <robertmhaas@gmail.com>,
 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 chaturvedipalak1911@gmail.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <92C8000B-4EE3-44FC-ABDD-6081E0B46D17@yesql.se>
References: 
 <CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com>
 <CAExHW5uEK+eeG7e2g6uWh7POrFpfp+dqfaa=_3miMN17zgeaJw@mail.gmail.com>
 <CAExHW5vz+PUHHUuzGRwtyx-mPLQk3nCZXxrFqnruRadEFrO5Xg@mail.gmail.com>
 <CAExHW5so6VSxBC-1V=35229Z1+dw5vhw8HxHg9ry7UzceKcXzA@mail.gmail.com>
 <26c766d6-db0f-43d3-a618-44f8d40a3121@iki.fi>
 <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>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Archived-At: 
 <https://www.postgresql.org/message-id/92C8000B-4EE3-44FC-ABDD-6081E0B46D17%40yesql.se>
Precedence: bulk

> On 22 Mar 2026, at 01:14, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> Attachd is new version with lots of changes again. I've experimented =
with different ways that the interface could look like, like with the =
"adjust" callback we discussed earlier.

I had a look at this version today, mainly 0008 and onwards, and I quite =
like
the API.  It is a bit verbose in places, but the improved readability =
outweighs
that IMHO.

> * The request_fn callback is called in postmaster startup, at the same =
stage as the old shmem_request callback was. But in EXEC_BACKEND mode, =
it's *also* called in each backend.

Should the request_fn be told, via an argument, from where it is called? =
 It
can be figured out but it's cleaner if all implementations will do it in =
the
same way.  I don't have a direct case in mind where it would be needed, =
but I
was recently digging into SSL passphrase reloading which has failure =
cases
precisely becasue of this so am thinking out loud to avoid similar =
problems
here.

> static void
> pgss_shmem_request(void *arg)
> {
>    static ShmemHashDesc pgssSharedHashDesc =3D {
>       .name =3D "pg_stat_statements hash",
>        .ptr =3D &pgss_hash,
>        .hash_info.keysize =3D sizeof(pgssHashKey),
>        .hash_info.entrysize =3D sizeof(pgssEntry),
>        .hash_flags =3D HASH_ELEM | HASH_BLOBS,
>    };
>    static ShmemStructDesc pgssSharedStateDesc =3D {
>        .name =3D "pg_stat_statements",
>        .size =3D sizeof(pgssSharedState),
>        .ptr =3D (void **) &pgss,
>    };
>=20
>    pgssSharedHashDesc.init_size =3D pgss_max;
>    pgssSharedHashDesc.max_size =3D pgss_max;
>    ShmemRequestHash(&pgssSharedHashDesc);
>    ShmemRequestStruct(&pgssSharedStateDesc);
> }

Roberts suggestion upthread to copy the structure to ensure that =
changing any
part of the struct after registration isn't causing subtle bugs seem =
like a
good improvement.

> I split this into more incremental patches. The first few are just =
refactorings that are probably useful on their own.

Reviewing these I wasn't able to spot any issues, but below are a few =
comments
on mostly 0008 but also a few others:

0008:
=3D=3D=3D=3D

+      doesn'' immediately allocate or initialize the memory, it merely

s/doesn''/doesn't/


+      registers the space to be allocated later in the startup =
sequence.  When
+      the memory is allocated, it is initialized to zero.  To any more =
complex
+      initialization, set the <function>init_fn()</function> callback, =
which

A word is missing here, perhaps: s/To any more complex/To perform any =
more complex/ ?


+      An example of allocating shared memory can be found in
       =
<filename>contrib/pg_stat_statements/pg_stat_statements.c</filename> in
       the <productname>PostgreSQL</productname> source tree.

While not the fault of this patch, I wonder if directing readers to a =
3000 line
C file which is growing increasingly complicated is all that helpful.  =
Maybe we
should (as a separate piece of work) construct more direct =
examples/tutorials
for this?


+      shared memory available. The system reserves a somes memory for

s/a somes/some/


+      another backend. The callbacks will be held while holding an =
internal
+      lock, which prevents concurrent two backends from initializating =
the

"will be held" reads a bit odd, perhaps "will be called" or "will be =
executed"?


+ * Nowadays, there is also third way to allocate shared memory called =
Dynamic

"a third way"


+ * ShmemInitStruct()/ShmemInitHash() is another way of registring shmem =
areas.

s/registring/registering/


+/*
+ * # of additional entries to reserve in the shmem index table, for =
allocations
+ * after postmaster startup (not a hard limit)
+ */
+#define SHMEM_INDEX_ADDITIONAL_SIZE         (64)

This comment no longer contains the word "estimated", so the "not a hard =
limit"
portion is harder to understand.  Does mean one can change the define =
freely
and recompile without crashes due to wrong number, or can the hash grow
dynamically during runtime?  Both interpretations are quite possible.


+   /*
+    * When we call the shmem_request callbacks, we enter the =
SB_REQUESTING
+    * phase. All ShmemRequestStruct calls happen in this state.
+    */
+   SB_REQUESTING,

Daft question, but what does the "B" stand for?


+   ShmemInitCallback init_fn;
+   void       *init_fn_arg;

init_fn_arg seems quite useful bu is under-documented, perhaps add =
something to
xfunc.sgml would be worthwhile? The same can be said for request_fn_arg.


+   /* Check that it's not already registered in this process */
+   foreach(lc, requested_shmem_areas)
+   {
+       ShmemStructDesc *existing =3D (ShmemStructDesc *) lfirst(lc);
+
+       if (strcmp(existing->name, desc->name) =3D=3D 0)
+           ereport(ERROR,
+                   (errmsg("shared memory struct \"%s\" is already =
registered",
+                           desc->name)));
+   }
+
+   requested_shmem_areas =3D lappend(requested_shmem_areas, desc);

As a side-note, I wish we had a list_append_unique flavour which would =
invoke a
function pointer instead of just list_member() to avoid boilerplate like =
this.


+   if (found)
+   {
+       /* Already present, just attach to it */
+       if (!attach_allowed)
+           elog(ERROR, "shared memory struct \"%s\" is already =
initialized", desc->name);

I guess it depends a lot on the caller, but couldn't it be argued that =
this
case is a lot like !init_allowed and thus a FATAL?


+/*
+ * ShmemGetRequestedSize() --- estimate the total size of all =
registered shared
+ *                              memory structures.
+ *
+ * This is called once at postmaster startup, before the shared memory =
segment
+ * has been created.

It's actually called twice, once for ShmemGUCs as well.</nitpickery>


+           /* out of memory; remove the failed ShmemIndex entry */
+           hash_search(ShmemIndex, desc->name, HASH_REMOVE, NULL);
+           ereport(ERROR,
+                   (errcode(ERRCODE_OUT_OF_MEMORY),
+                    errmsg("not enough shared memory for data =
structure"
+                           " \"%s\" (%zu bytes requested)",
+                           desc->name, desc->size)));

When shmem_startup_state is set to SB_LATE_ATTACH_OR_INIT, would it be
worthwhile to add an errhint to move allocation to init phase instead?


Various
=3D=3D=3D=3D=3D

In the 0014 commit message, s/ProgGlobal/ProcGlobal/


-   if (IsUnderPostmaster && =
!process_shared_preload_libraries_in_progress)
+   if (shmem_startup_state =3D=3D SB_DONE && IsUnderPostmaster)

This hunk in 0014 makes an assertion a few lines down pointless as it =
checks
the same as the if conditional.


- * have been created by initdb, and CLOGShmemInit must have been
+ * have been created by initdb, and CLOGShmemInit must have been XXX

Stray XXX in 0015.


-   ControlFile =3D palloc_object(ControlFileData);
+   LocalControlFile =3D palloc_object(ControlFileData);
+   ControlFile =3D LocalControlFile;

I'm likely missing something obvious but is the LocalControlFile still =
needed?

--
Daniel Gustafsson