MIME-Version: 1.0
References: 
 <CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com>
 <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>
 <b2451225-0a10-4ded-abef-4e9efbde7a8e@iki.fi>
 <CAExHW5tCC0T1ky=Jnq-AvMxa67Adaw7aQ4iQAO=BSdHcbSNBVg@mail.gmail.com>
 <791c3f18-f4de-4d84-ac6b-c7ccc074dd38@iki.fi>
In-Reply-To: <791c3f18-f4de-4d84-ac6b-c7ccc074dd38@iki.fi>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Wed, 1 Apr 2026 17:29:13 +0530
Message-ID: 
 <CAExHW5tS7GncN90oJWOSzW_3F1EHL9xwe59L7Req3nUVgmObUw@mail.gmail.com>
Subject: Re: Better shared data structure management and resizable shared data
 structures
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>,
	pgsql-hackers <pgsql-hackers@postgresql.org>, chaturvedipalak1911@gmail.com
Content-Type: multipart/mixed; boundary="000000000000ce45a6064e64d0ba"
Archived-At: 
 <https://www.postgresql.org/message-id/CAExHW5tS7GncN90oJWOSzW_3F1EHL9xwe59L7Req3nUVgmObUw%40mail.gmail.com>
Precedence: bulk

--000000000000ce45a6064e64d0ba
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 31, 2026 at 1:45=E2=80=AFAM Heikki Linnakangas <hlinnaka@iki.fi=
> wrote:
>
> On 30/03/2026 07:50, Ashutosh Bapat wrote:
> > On Sat, Mar 28, 2026 at 4:47=E2=80=AFAM Heikki Linnakangas <hlinnaka@ik=
i.fi> wrote:
> >> On 27/03/2026 09:01, Ashutosh Bapat wrote:
> >>> /* Restore basic shared memory pointers */
> >>> if (UsedShmemSegAddr !=3D NULL)
> >>> + {
> >>> InitShmemAllocator(UsedShmemSegAddr);
> >>> + ShmemCallRequestCallbacks();
> >>>
> >>> It's not clear how we keep the list of registered callbacks across th=
e
> >>> 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.
> >
> > RegisterShmemCallbacks() may be called after the startup, and it will
> > add new areas to the shared memory. How are those registries synced
> > across the backends? From your answer below, those registries are not
> > synced across backends. They will be wiped out by the restart and
> > won't be registered again. Is that right? I think we need to document
> > this fact and also the need to call RegisterShmemCallbacks() from all
> > the backends where the new areas are required after the startup.
>
> Correct. Ok, I'll add a note to comment on RegisterShmemCallbacks() to
> call that out more explicitly, hope it helps.
>
> - Heikki
>

Continuing review starting
0007
-------
Subject: [PATCH v8 07/16] Add test module to test after-startup shmem
allocations

I like the idea.

+ *
+ * XXX This module provides interface functions for C functionality to SQL=
, to
+ * make it possible to test AIO related behavior in a targeted way from SQ=
L.
+ * It'd not generally be safe to export these functions to SQL, but for a =
test
+ * that's fine.

This mentions test_aio - needs to be rewritten for test_shmem.

+
+#include "access/relation.h"
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "storage/shmem.h"

I don't think we need access/relation.h. Others seem ok, but I haven't chec=
ked.

In order to better test the difference between EXEC_BACKEND and
non-EXEC_BACKEND builds, please consider incorporating the attached
patch v8-0009-edits.diff

0008
------
- LWLockRelease(AddinShmemInitLock);
+ /* The hash table must be initialized already */
+ Assert(pgss_hash !=3D NULL);

Does it make sense to also Assert(pgss)? A broader question is do we
want to make it a pattern that every user of ShmemRequest*() also
Assert()s that the pointer is non-NULL in the init callback? It is a
test that the ShmemRequest*(), which is far from, init_fn is working
correctly.

/*
- * If we're in the postmaster (or a standalone backend...), set up a shmem
- * exit hook to dump the statistics to disk.
+ * Set up a shmem exit hook to dump the statistics to disk on postmaster
+ * (or standalone backend) exit.
*/
- if (!IsUnderPostmaster)
- on_shmem_exit(pgss_shmem_shutdown, (Datum) 0);
-
- /*
- * Done if some other process already completed our initialization.
- */
- if (found)
- return;
+ on_shmem_exit(pgss_shmem_shutdown, (Datum) 0);
Given that the structures are registered only at the startup, this
function will be called only from Postmaster, but given that the
structures can be registered and initialized after startup in any
backend, it's better to at least Assert(!IsUnderPostmaster) at the
beginning of this function. The code below is not expected to be
called in any backend too. So Assert(IsUnderPostmaster) at the
beginning of the function would be good safety catch too.

/*
+ * Load any pre-existing statistics from file.
+ *
* Note: we don't bother with locks here, because there should be no other
* processes running when this code is reached.
*/

I was a bit worried that the code next to read stat files is being
crammed in init_fn, but given that the contents of the files are used
to initialize the shared hash table, I think this is fine.

0009
-------
+void
+RegisterBuiltinShmemCallbacks(void)
+{
+ const ShmemCallbacks *builtin_subsystems[] =3D {
+#define PG_SHMEM_SUBSYSTEM(subsystem_callbacks) &subsystem_callbacks,
+#include "storage/subsystemlist.h"
+#undef PG_SHMEM_SUBSYSTEM
+ };
+
+ for (int i =3D 0; i < lengthof(builtin_subsystems); i++)
+ RegisterShmemCallbacks(builtin_subsystems[i]);
+}
+

I don't think we need to use a separate array here, we can just call
RegisterShmemCallbacks() directly in the macro as attached.

0011
------
+ InjectionPointAttach("aio-process-completion-before-shared",
+ "test_aio",
+ "inj_io_short_read",
+ NULL,
+ 0);
+ InjectionPointLoad("aio-process-completion-before-shared");
+
+ InjectionPointAttach("aio-worker-after-reopen",
+ "test_aio",
+ "inj_io_reopen",
+ NULL,
+ 0);
+ InjectionPointLoad("aio-worker-after-reopen");

Attaching and loading an injection point shouldn't be part of the
shared memory initialization. It doens't feel like it should be part
of shmem_startup_hook as well. So not a fault of this patch. I am
wondering why can't it be done in the tests themselves?

0012
------
@@ -663,6 +663,8 @@ SubPostmasterMain(int argc, char *argv[])
*/
LocalProcessControlFile(false);
+ RegisterBuiltinShmemCallbacks();
+

Shouldn't this be part of the previous patch?

-void
-InitProcGlobal(void)
+static void
+ProcGlobalShmemInit(void *arg)
{

I have reviewed most of this patch in earlier versions of this
patchset except this part, which is better than its last version.

Will continue to review the rest of the patches tomorrow.

--=20
Best Wishes,
Ashutosh Bapat

--000000000000ce45a6064e64d0ba
Content-Type: application/octet-stream; name="v8-0007-edits.diff.nocibot"
Content-Disposition: attachment; filename="v8-0007-edits.diff.nocibot"
Content-Transfer-Encoding: base64
Content-ID: <f_mnfyu1p90>
X-Attachment-Id: f_mnfyu1p90

ZGlmZiAtLWdpdCBhL3NyYy90ZXN0L21vZHVsZXMvdGVzdF9zaG1lbS90LzAwMV9sYXRlX3NobWVt
X2FsbG9jLnBsIGIvc3JjL3Rlc3QvbW9kdWxlcy90ZXN0X3NobWVtL3QvMDAxX2xhdGVfc2htZW1f
YWxsb2MucGwKaW5kZXggODRlYzg0MWI1NDIuLmMxNTRmNTc2ODJhIDEwMDY0NAotLS0gYS9zcmMv
dGVzdC9tb2R1bGVzL3Rlc3Rfc2htZW0vdC8wMDFfbGF0ZV9zaG1lbV9hbGxvYy5wbAorKysgYi9z
cmMvdGVzdC9tb2R1bGVzL3Rlc3Rfc2htZW0vdC8wMDFfbGF0ZV9zaG1lbV9hbGxvYy5wbApAQCAt
MzIsMTIgKzMyLDE4IEBAICRub2RlLT5zdGFydDsKIAogIyBXaGVuIGxvYWRlZCB2aWEgc2hhcmVk
X3ByZWxvYWRfbGlicmFyaWVzLCB0aGUgYXR0YWNoIGNhbGxiYWNrIGlzCiAjIGNhbGxlZCBvciBu
b3QsIGRlcGVuZGluZyBvbiB3aGV0aGVyIHRoaXMgaXMgYW4gRVhFQ19CQUNLRU5EIGJ1aWxkLgor
bXkgJGV4ZWNfYmFja2VuZCA9ICRub2RlLT5zYWZlX3BzcWwoInBvc3RncmVzIiwgIlNIT1cgZGVi
dWdfZXhlY19iYWNrZW5kOyIpIGVxICdvbic7CiAkYXR0YWNoX2NvdW50MSA9ICRub2RlLT5zYWZl
X3BzcWwoInBvc3RncmVzIiwgIlNFTEVDVCBnZXRfdGVzdF9zaG1lbV9hdHRhY2hfY291bnQoKTsi
KTsKICRhdHRhY2hfY291bnQyID0gJG5vZGUtPnNhZmVfcHNxbCgicG9zdGdyZXMiLCAiU0VMRUNU
IGdldF90ZXN0X3NobWVtX2F0dGFjaF9jb3VudCgpOyIpOwogCi1vaygkYXR0YWNoX2NvdW50MSA9
PSAwICYmICRhdHRhY2hfY291bnQyID09IDAgfHwKLSAgICRhdHRhY2hfY291bnQyID49ICRhdHRh
Y2hfY291bnQxLAotICAgImxvYWRlZCB2aWEgc2hhcmVkX3ByZWxvYWRfbGlicmFyaWVzIik7Citp
ZiAoJGV4ZWNfYmFja2VuZCkKK3sKKyAgIGNtcF9vaygkYXR0YWNoX2NvdW50MiwgJz4nLCAkYXR0
YWNoX2NvdW50MSwgImF0dGFjaCBjYWxsYmFjayBpcyBjYWxsZWQgaW4gZWFjaCBiYWNrZW5kIHdo
ZW4gbG9hZGVkIHZpYSBzaGFyZWRfcHJlbG9hZF9saWJyYXJpZXMiKTsKK30KK2Vsc2UKK3sKKyAg
IG9rKCRhdHRhY2hfY291bnQxID09IDAgJiYgJGF0dGFjaF9jb3VudDIgPT0gMCwgImF0dGFjaCBj
YWxsYmFjayBpcyBub3QgY2FsbGVkIHdoZW4gbG9hZGVkIHZpYSBzaGFyZWRfcHJlbG9hZF9saWJy
YXJpZXMiKTsKK30KIAogJG5vZGUtPnN0b3A7CiBkb25lX3Rlc3RpbmcoKTsKZGlmZiAtLWdpdCBh
L3NyYy90ZXN0L21vZHVsZXMvdGVzdF9zaG1lbS90ZXN0X3NobWVtLmMgYi9zcmMvdGVzdC9tb2R1
bGVzL3Rlc3Rfc2htZW0vdGVzdF9zaG1lbS5jCmluZGV4IDFkN2YzMWIzN2M3Li5kNTJjZDE4Nzdm
OCAxMDA2NDQKLS0tIGEvc3JjL3Rlc3QvbW9kdWxlcy90ZXN0X3NobWVtL3Rlc3Rfc2htZW0uYwor
KysgYi9zcmMvdGVzdC9tb2R1bGVzL3Rlc3Rfc2htZW0vdGVzdF9zaG1lbS5jCkBAIC0xOCw3ICsx
OCw2IEBACiAKICNpbmNsdWRlICJwb3N0Z3Jlcy5oIgogCi0jaW5jbHVkZSAiYWNjZXNzL3JlbGF0
aW9uLmgiCiAjaW5jbHVkZSAiZm1nci5oIgogI2luY2x1ZGUgIm1pc2NhZG1pbi5oIgogI2luY2x1
ZGUgInN0b3JhZ2Uvc2htZW0uaCIK
--000000000000ce45a6064e64d0ba
Content-Type: application/octet-stream; name="v8-0009-edits.diff.nocibot"
Content-Disposition: attachment; filename="v8-0009-edits.diff.nocibot"
Content-Transfer-Encoding: base64
Content-ID: <f_mnfyu1pf1>
X-Attachment-Id: f_mnfyu1pf1

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3N0b3JhZ2UvaXBjL2lwY2kuYyBiL3NyYy9iYWNrZW5k
L3N0b3JhZ2UvaXBjL2lwY2kuYwppbmRleCAyMTdmNTI5MTI3MC4uMmEzN2JiMTU5MzYgMTAwNjQ0
Ci0tLSBhL3NyYy9iYWNrZW5kL3N0b3JhZ2UvaXBjL2lwY2kuYworKysgYi9zcmMvYmFja2VuZC9z
dG9yYWdlL2lwYy9pcGNpLmMKQEAgLTE4MywxNCArMTgzLDkgQEAgQ3JlYXRlU2hhcmVkTWVtb3J5
QW5kU2VtYXBob3Jlcyh2b2lkKQogdm9pZAogUmVnaXN0ZXJCdWlsdGluU2htZW1DYWxsYmFja3Mo
dm9pZCkKIHsKLQljb25zdCBTaG1lbUNhbGxiYWNrcyAqYnVpbHRpbl9zdWJzeXN0ZW1zW10gPSB7
Ci0jZGVmaW5lIFBHX1NITUVNX1NVQlNZU1RFTShzdWJzeXN0ZW1fY2FsbGJhY2tzKSAmc3Vic3lz
dGVtX2NhbGxiYWNrcywKKyNkZWZpbmUgUEdfU0hNRU1fU1VCU1lTVEVNKHN1YnN5c3RlbV9jYWxs
YmFja3MpIFJlZ2lzdGVyU2htZW1DYWxsYmFja3MoJihzdWJzeXN0ZW1fY2FsbGJhY2tzKSk7CiAj
aW5jbHVkZSAic3RvcmFnZS9zdWJzeXN0ZW1saXN0LmgiCiAjdW5kZWYgUEdfU0hNRU1fU1VCU1lT
VEVNCi0JfTsKLQotCWZvciAoaW50IGkgPSAwOyBpIDwgbGVuZ3Rob2YoYnVpbHRpbl9zdWJzeXN0
ZW1zKTsgaSsrKQotCQlSZWdpc3RlclNobWVtQ2FsbGJhY2tzKGJ1aWx0aW5fc3Vic3lzdGVtc1tp
XSk7CiB9CiAKIC8qCg==
--000000000000ce45a6064e64d0ba--