On 25/03/2026 20:37, Robert Haas wrote:
> On Sat, Mar 21, 2026 at 8:14 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:>> Shmem callbacks
>> ---------------
>>
>> I separated the request/init/fn callbacks from the structs. There's now
>> a concept of "shmem callbacks", which you register in _PG_init(). For
>> example:
>>
>> static void pgss_shmem_request(void *arg);
>> static void pgss_shmem_init(void *arg);
>>
>> static const ShmemCallbacks pgss_shmem_callbacks = {
>> .request_fn = pgss_shmem_request,
>> .init_fn = pgss_shmem_init,
>> .attach_fn = NULL, /* no special attach actions needed */
>> };
>
> What's the advantage of coupling the functions together this way, vs.
> just registering each callback individually?
One reason is to support allocations after postmaster startup. The
RegisterShmemCallbacks() call ties together all the resources requested
by the request_fn callback, with the the init_fn or attach_fn callbacks
that will later initialize/attach them. The init_fn/attach_fn callbacks
are called only after *all* the resources requested by the request_fn
callback have been initialized, and it holds a lock while doing all that.
If the callbacks were registered separately, shmem.c wouldn't know when
to call the init_fn/attach_fn. There's no problem during postmaster or
backend startup, because we run all init_fn or attach_fn callbacks in
the whole system, after requesting all the resources, but after startup,
you must only call the callbacks related to the newly-requested resources.
Aside from that after-startup allocation issue, though, IMO the
ShmemCallbacks struct makes it more clear that the callbacks are meant
to work together on the same resources.
One way to think of this is that all the resources requested by the
request_fn callback are implicitly part of the same "subsystem", and
need to be initialized/attached to together. We discussed that before,
and I still wonder if we should make that concept of a subsystem more
explicit. If we just renamed ShmemCallbacks to ShmemSubsystem, and give
each subsystem a name, it'd look like this:
static void pgss_shmem_request(void *arg);
static void pgss_shmem_init(void *arg);
static const ShmemSubsystem pgss_shmem_subsystem = {
.name = "pg_stat_statements"
.request_fn = pgss_shmem_request,
.init_fn = pgss_shmem_init,
.attach_fn = NULL, /* no special attach actions needed */
};
static void
pgss_shmem_request(void *arg)
{
ShmemRequestStruct(&pgssSharedStateDesc, &(ShmemRequestStructOpts) {
/*
* name is optional in this design, subsystem's name is used if
* not given
*/
.name = "pg_stat_statements",
.size = sizeof(pgssSharedState),
.ptr = (void **) &pgss,
});
}
static void
pgss_shmem_init(void *arg)
{
/* initialize contents of pgss */
...
}
void
_PG_init(void)
{
RegisterShmemSubsystem(&pgss_shmem_subsystem);
}
Thinking how this might work without such a struct, registering the
callbacks separately, here's an alternative design:
static void pgss_shmem_request(void *arg);
static void pgss_shmem_init(void *arg);
static void
pgss_shmem_request(void *arg)
{
ShmemRequestStruct(&pgssSharedStateDesc, &(ShmemRequestStructOpts) {
.name = "pg_stat_statements",
.size = sizeof(pgssSharedState),
.ptr = (void **) &pgss,
});
ShmemRegisterInitCallback(&pgss_shmem_init);
/* no attach callback needed, but for illustration: */
ShmemRegisterInitCallback(&pgss_shmem_attach);
}
static void
pgss_shmem_init(void *arg)
{
/* initialize contents of pgss */
...
}
void
_PG_init(void)
{
ShmemRegisterRequestCallback(&pgss_shmem_request);
}
In this design, the ShmemRegisterRequestCallback() call still ties
together all the related resources. All the resources requested in the
request-callback are initialized together, and the fact that the
init/attach callbacks are registered within the request callback
associates them with the resources. This feels a little Rube
Goldbergian, with one callback registering more callbacks, but would
also work.
> Also, I don't understand what "arg" is supposed to be doing. It
> doesn't seem to be getting used for anything.
It's an opaque pointer that's passed through to the callbacks. Some
callers might want to pass extra data to the callback. None of the
current callers use it, so maybe it's not needed, but it seemed like
good future-proofing.
>> Shmem requests
>> --------------
>>
>> To register a shmem area, you call ShmemRequestStruct() or
>> ShmmeRequestHash() from the request callback function. For example:
>>
>> 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);
>> }
>>
>> Initialization happens in the init callback, which is called after all
>> the pointers (pgss, pgss_hash) have been set.
>
> I think this is not bad. I suppose it lets you get a complete list of
> all of the descriptors someplace. It seems to avoid double-computing
> the request size, or any possibility of drift between the requested
> size and the allocated size. Having to create the struct and then set
> the size afterward in some cases is a tiny bit awkward-looking, but
> it's not awful. I do wonder if some coding pattern might creep in
> where people create the struct and register it and then try to change
> the size, or other fields, afterward. I'm tempted to propose making
> the struct non-static and having the registration functions copy it,
> so that there can be absolutely no question of getting away with such
> antisocial behavior.
Here's another version, where that now looks like this:
static void
pgss_shmem_request(void *arg)
{
static ShmemHashDesc pgssSharedHashDesc;
static ShmemStructDesc pgssSharedStateDesc;
ShmemRequestHash(&pgssSharedHashDesc, &(ShmemRequestHashOpts) {
.name = "pg_stat_statements hash",
.ptr = &pgss_hash,
.init_size = pgss_max,
.max_size = pgss_max,
.hash_info.keysize = sizeof(pgssHashKey),
.hash_info.entrysize = sizeof(pgssEntry),
.hash_flags = HASH_ELEM | HASH_BLOBS,
});
ShmemRequestStruct(&pgssSharedStateDesc, &(ShmemRequestStructOpts) {
.name = "pg_stat_statements",
.size = sizeof(pgssSharedState),
.ptr = (void **) &pgss,
});
}
Notable differences to that since last version:
I separated the backend-private "handle" and the options structs. So
ShmemRequestStruct/Hash now takes two arguments:
The first argument is a pointer to the "descriptor", which is a
backend-private handle for the shared memory area. It's currently not
very interesting for plain structs because you can't really do anything
with it, but in the future the handle could be used to resize the area
for example. Also, one of the later patches in this patch set refactors
SLRUs to be requested in this fashion too. For SLRUs the handle is
already useful; SLRUs have always had a backend-private "SlruCtl" handle
like that.
The second argument is an "options" struct, which is really just
syntactic sugar to pass multiple arguments, some of which can be left
empty. The options are now copied, so the struct can be short-lived.
Alternatively, the arguments could be passed as normal function
arguments like you suggested earlier. But I quite like this style,
especially with hash tables and SLRUs which take more options and
already used structs to pass the options.
There's one annoying problem with that syntax though: pgindent doesn't
like it and gives errors like this:
Failure in contrib/pg_stat_statements/pg_stat_statements.c: Error@508:
Unbalanced parens
Warning@516: Extra )
Error@517: Unbalanced parens
Warning@521: Extra )
That would be nice to fix in pgindent in any case but I haven't looked
into it yet.
Another idea is to use a macro to hide that from pgindent, which would
make the calls little less verbose anyway:
#define ShmemRequestStruct(desc, ...) ShmemRequestStructWithOpts(desc,
&(ShmemRequestStructOpts) { __VA_ARGS__ })
Then the call would be simply:
ShmemRequestStruct(&pgssSharedStateDesc,
.name = "pg_stat_statements",
.size = sizeof(pgssSharedState),
.ptr = (void **) &pgss,
);
New version attached. I also pushed this to
https://github.com/hlinnaka/postgres/tree/shmem-init-refactor-8.
- Heikki
