On Wed, 1 Apr 2026 at 20:17, Heikki Linnakangas <[email protected]> wrote:
>
> Yet another version attached (also available at:
> https://github.com/hlinnaka/postgres/tree/shmem-init-refactor-9). The
> main change is the shape of the ShmemRequest*() calls:
I didn't read the whole thread, as it's quite long, but did look at
the patchset for a while to figure out where it's going.
0005:
A few assorted comments:
While I do think it's an improvement over the current APIs, the
improvement seems to be mostly concentrated in the RequestStruct/Hash
department, with only marginal improvements in RegisterShmemCallbacks.
I feel like it's missing the important part: I'd like
direct-from-_PG_init() ShmemRequestStruct/Hash calls. If
ShmemRequestStruct/Hash had a size callback as alternative to the size
field (which would then be called after preload_libraries finishes)
then that would be sufficient for most shmem allocations, and it'd
simplify shmem management for most subsystems.
We'd still need the shmem lifecycle hooks/RegisterShmemCallbacks to
allow conditionally allocated shmem areas (e.g. those used in aio),
but I think that, in general, we shouldn't need a separate callback
function just to get started registering shmem structures.
I also noticed that ShmemCallbacks.%_arg are generally undocumented,
and I couldn't find any users in core (at the end of the patchset)
that actually use the argument. Could it be I missed something?
I don't understand the use of ShmemStructDesc. They generally/always
are private to request_fn(), and their fields are used exclusively
inside the shmem mechanisms, with no reads of its fields that can't
already be deduced from context. Why do we need that struct
everywhere?
> +++ b/src/backend/storage/ipc/shmem.c
[...]
> + /* Check that it's not already registered in this process */
> + foreach_ptr(ShmemStructDesc, existing, pending_shmem_requests)
> + {
> + if (strcmp(existing->name, options->name) == 0)
> + ereport(ERROR,
> + (errmsg("shared memory struct \"%s\" is already
> registered",
> + options->name)));
> + }
> +
> + request = palloc(sizeof(ShmemRequest));
> + request->options = options;
> + request->desc = desc;
> + request->kind = kind;
> + pending_shmem_requests = lappend(pending_shmem_requests, request);
Apparently, pending_shmem_requests is a list of ShmemRequest, but the
iteration just above on the same list assumes ShmemStructDesc, which
seems wrong to me.
00017:
I like this idea, but I think it missed its chance to make good on an
opportunity to reduce waste in alignments: We know which structs we're
going to allocate at which alignments, so we could save space by
packing the structs. I don't expect it to save much, but it could be a
few 100 of kbs with a few BLCKSZ-aligned allocations.
Kind regards,
Matthias van de Meent
Databricks (https://www.databricks.com)
