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

On 05/04/2026 02:17, Matthias van de Meent wrote:
> Formatting:
>> + ShmemRequestHash(.name = "pg_stat_statements hash",
>> + .nelems = pgss_max,
>> + .hash_info.keysize = sizeof(pgssHashKey),
>> + .hash_info.entrysize = sizeof(pgssEntry),
>> + .hash_flags = HASH_ELEM | HASH_BLOBS,
>> + .ptr = &pgss_hash,
>> + );
> (note that additional unit of indentation for the closing bracket)
>
> Is this malformatting caused by pgindent? If so, could you see if
> there's a better way of defining ShmemRequestHash/Struct that doesn't
> have this indent as output?

Yeah, that's pgindent. Matter of taste, but I think that looks fine. An
alternative is to put the closing bracket on the same line with the last
argument and drop the trailing comma:

ShmemRequestStruct(.name = "pg_stat_statements",
.size = sizeof(pgssSharedState),
.ptr = (void **) &pgss);

That looks OK to me too.

>> + pgss->extent = 0;
>> + pgss->n_writers = 0;
>> + pgss->gc_count = 0;
>> + pgss->stats.dealloc = 0;
>
> Shmem is said to be zero-initialized, should we remove the manual
> zero-initialization?
>
>> + on_shmem_exit(pgss_shmem_shutdown, (Datum) 0);
> See my upthread comment about adding optional on_shmem_exit callbacks
> to ShmemCallbacks.
>
> 0005: LGTM
>
> 0006: I don't think it is a great idea to make the LwLock machinery
> the first to get allocation requests:
> It has the RequestNamedLWLockTranche infrastructure, which can only
> register new requests while process_shmem_requests_in_progress, and
> making it request its memory ahead of everything else is likely to
> cause an undersized tranche to be allocated.

Good catch. I think the easiest fix is to call process_shmem_requests()
before ShmemCallRequestCallbacks() at postmaster startup. That's kind of
how it was before: process_shmem_requests() was called before all the
*ShmemSize() and *ShmemInit() functions in core.

> You could make sure that this isn't an issue by maintaining a flag
> in lwlock.c that's set when the shmem request is made (and reset on
> shmem exit), which must be false when RequestNamedLWLockTranche() is
> called, and if not then it should throw an error.
I'll change it so that the number of locks calculated in
LWLockShmemRequest() is stored in a global variable, and
LWLockShmemInit() has an Assert() to cross-checks with that. That
catches the bug and seems like a good cross-check in general.

This isn't the only place where we need to pass information from the
request callback to the init callback. I've used a global variable for
that here, and also between ProcGlobalShmemRequest() and
ProcGlobalShmemRequest(). An alternative might be to use the callback
arg pointers that are currently unused, but I'm not sure how to make
that ergonomic. The current 'arg' isn't very helpful for that, so
perhaps the signatures should look like this instead:

static void
LWLockShmemRequest(Datum *init_arg)
{
int numLocks;

numLocks = NUM_FIXED_LWLOCKS + NumLWLocksForNamedTranches();

/* pass the calculated numLocks value to LWLockShmemInit() */
*init_arg = Int32GetDatum(numLocks);

...
}

static void
LWLockShmemInit(Datum init_arg)
{
int numLocks = DatumGetIn32(numLocks);

...
}

If you need to pass more than a single Datum, you can allocate a struct
and pass it via PointerGetDatum(). At that point global variables might
feel simpler again though.

> 0010: Not looked at everything yet, but a few comment:
>
>> +++ b/src/include/access/slru.h
>
> With the changes in the signatures for most/all SLRU functions from a
> hidden-by-typedef pointer to a visible pointer type, maybe this could
> be an opportunity to swap them to `const SlruDesc *ctl` wherever
> possible? I don't think there are many backend-local changes that
> happen to SlruDescs once we've properly started the backend. I'm happy
> to provide an incremental patch if you'd like me to spend cycles on it
> if you're busy.

Yeah sounds like a good idea.

>> +++ b/src/backend/access/transam/clog.c
>
>> + SimpleLruRequest(.desc = &XactSlruDesc,
>> + .name = "transaction",
>> + .Dir = "pg_xact",
>> + .long_segment_names = false,
>> +
>> + .nslots = CLOGShmemBuffers(),
>> + .nlsns = CLOG_LSNS_PER_PAGE,
>> +
>> + .sync_handler = SYNC_HANDLER_CLOG,
>> + .PagePrecedes = CLOGPagePrecedes,
>> + .errdetail_for_io_error = clog_errdetail_for_io_error,
>
> That awfully inconsistent field name styling is ... awful, but not
> this patch's fault. If something can be done about it in a cheap
> fashion in this patch, that'd be great, but I won't hold it against
> you if that's skipped.

:-D.

- Heikki