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
