> On 22 Mar 2026, at 01:14, Heikki Linnakangas <[email protected]> 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 = {
> .name = "pg_stat_statements hash",
> .ptr = &pgss_hash,
> .hash_info.keysize = sizeof(pgssHashKey),
> .hash_info.entrysize = sizeof(pgssEntry),
> .hash_flags = HASH_ELEM | HASH_BLOBS,
> };
> static ShmemStructDesc pgssSharedStateDesc = {
> .name = "pg_stat_statements",
> .size = sizeof(pgssSharedState),
> .ptr = (void **) &pgss,
> };
>
> pgssSharedHashDesc.init_size = pgss_max;
> pgssSharedHashDesc.max_size = 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:
====
+ 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 = (ShmemStructDesc *) lfirst(lc);
+
+ if (strcmp(existing->name, desc->name) == 0)
+ ereport(ERROR,
+ (errmsg("shared memory struct \"%s\" is already registered",
+ desc->name)));
+ }
+
+ requested_shmem_areas = 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
=====
In the 0014 commit message, s/ProgGlobal/ProcGlobal/
- if (IsUnderPostmaster && !process_shared_preload_libraries_in_progress)
+ if (shmem_startup_state == 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 = palloc_object(ControlFileData);
+ LocalControlFile = palloc_object(ControlFileData);
+ ControlFile = LocalControlFile;
I'm likely missing something obvious but is the LocalControlFile still needed?
--
Daniel Gustafsson
