Re: Better shared data structure management and resizable shared data structures

On Tue, Mar 31, 2026 at 1:45 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 30/03/2026 07:50, Ashutosh Bapat wrote:
> > On Sat, Mar 28, 2026 at 4:47 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >> 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.
> >
> > 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 SQL.
+ * 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 checked.

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 != 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[] = {
+#define PG_SHMEM_SUBSYSTEM(subsystem_callbacks) &subsystem_callbacks,
+#include "storage/subsystemlist.h"
+#undef PG_SHMEM_SUBSYSTEM
+ };
+
+ for (int i = 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.

-- 
Best Wishes,
Ashutosh Bapat