On Fri, Aug 29, 2025 at 09:51:38PM -0500, Nathan Bossart wrote: > I've also attached a rebased patch that addresses all the latest feedback. > A reworked verison of the test patch is also included, but that's mostly > intended for CI purposes and is still not intended for commit (yet).
And here's an attempt at fixing the alignment problems revealed by cfbot. -- nathan
>From 72260a5f2fd93c77927a50aaf521c9f294d58ac7 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Wed, 27 Aug 2025 15:13:41 -0500 Subject: [PATCH v20 1/2] Move dynamically-allocated tranche names to shared memory. --- contrib/pg_prewarm/autoprewarm.c | 3 +- doc/src/sgml/xfunc.sgml | 15 +- src/backend/postmaster/launch_backend.c | 6 +- src/backend/storage/ipc/dsm_registry.c | 12 +- src/backend/storage/lmgr/lwlock.c | 206 +++++++++--------- src/include/storage/lwlock.h | 24 +- src/test/modules/test_dsa/test_dsa.c | 6 +- .../test_dsm_registry/test_dsm_registry.c | 3 +- .../modules/test_radixtree/test_radixtree.c | 9 +- src/test/modules/test_slru/test_slru.c | 6 +- .../modules/test_tidstore/test_tidstore.c | 3 +- 11 files changed, 120 insertions(+), 173 deletions(-) diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index c01b9c7e6a4..880e897796a 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -864,7 +864,7 @@ apw_init_state(void *ptr) { AutoPrewarmSharedState *state = (AutoPrewarmSharedState *) ptr; - LWLockInitialize(&state->lock, LWLockNewTrancheId()); + LWLockInitialize(&state->lock, LWLockNewTrancheId("autoprewarm")); state->bgworker_pid = InvalidPid; state->pid_using_dumpfile = InvalidPid; } @@ -883,7 +883,6 @@ apw_init_shmem(void) sizeof(AutoPrewarmSharedState), apw_init_state, &found); - LWLockRegisterTranche(apw_state->lock.tranche, "autoprewarm"); return found; } diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index f116d0648e5..da21ef56891 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3759,7 +3759,7 @@ LWLockPadded *GetNamedLWLockTranche(const char *tranche_name) <literal>shmem_request_hook</literal>. To do so, first allocate a <literal>tranche_id</literal> by calling: <programlisting> -int LWLockNewTrancheId(void) +int LWLockNewTrancheId(const char *name) </programlisting> Next, initialize each LWLock, passing the new <literal>tranche_id</literal> as an argument: @@ -3777,17 +3777,8 @@ void LWLockInitialize(LWLock *lock, int tranche_id) </para> <para> - Finally, each backend using the <literal>tranche_id</literal> should - associate it with a <literal>tranche_name</literal> by calling: -<programlisting> -void LWLockRegisterTranche(int tranche_id, const char *tranche_name) -</programlisting> - </para> - - <para> - A complete usage example of <function>LWLockNewTrancheId</function>, - <function>LWLockInitialize</function>, and - <function>LWLockRegisterTranche</function> can be found in + A complete usage example of <function>LWLockNewTrancheId</function> and + <function>LWLockInitialize</function> can be found in <filename>contrib/pg_prewarm/autoprewarm.c</filename> in the <productname>PostgreSQL</productname> source tree. </para> diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c index cd9547b03a3..de1b06df10d 100644 --- a/src/backend/postmaster/launch_backend.c +++ b/src/backend/postmaster/launch_backend.c @@ -101,7 +101,7 @@ typedef struct struct InjectionPointsCtl *ActiveInjectionPoints; #endif int NamedLWLockTrancheRequests; - NamedLWLockTranche *NamedLWLockTrancheArray; + char **NamedLWLockTrancheNames; int *LWLockCounter; LWLockPadded *MainLWLockArray; slock_t *ProcStructLock; @@ -761,7 +761,7 @@ save_backend_variables(BackendParameters *param, #endif param->NamedLWLockTrancheRequests = NamedLWLockTrancheRequests; - param->NamedLWLockTrancheArray = NamedLWLockTrancheArray; + param->NamedLWLockTrancheNames = NamedLWLockTrancheNames; param->LWLockCounter = LWLockCounter; param->MainLWLockArray = MainLWLockArray; param->ProcStructLock = ProcStructLock; @@ -1022,7 +1022,7 @@ restore_backend_variables(BackendParameters *param) #endif NamedLWLockTrancheRequests = param->NamedLWLockTrancheRequests; - NamedLWLockTrancheArray = param->NamedLWLockTrancheArray; + NamedLWLockTrancheNames = param->NamedLWLockTrancheNames; LWLockCounter = param->LWLockCounter; MainLWLockArray = param->MainLWLockArray; ProcStructLock = param->ProcStructLock; diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c index ca12815f4a8..97130925106 100644 --- a/src/backend/storage/ipc/dsm_registry.c +++ b/src/backend/storage/ipc/dsm_registry.c @@ -299,8 +299,7 @@ GetNamedDSA(const char *name, bool *found) entry->type = DSMR_ENTRY_TYPE_DSA; /* Initialize the LWLock tranche for the DSA. */ - state->tranche = LWLockNewTrancheId(); - LWLockRegisterTranche(state->tranche, name); + state->tranche = LWLockNewTrancheId(name); /* Initialize the DSA. */ ret = dsa_create(state->tranche); @@ -321,9 +320,6 @@ GetNamedDSA(const char *name, bool *found) ereport(ERROR, (errmsg("requested DSA already attached to current process"))); - /* Initialize existing LWLock tranche for the DSA. */ - LWLockRegisterTranche(state->tranche, name); - /* Attach to existing DSA. */ ret = dsa_attach(state->handle); dsa_pin_mapping(ret); @@ -378,8 +374,7 @@ GetNamedDSHash(const char *name, const dshash_parameters *params, bool *found) entry->type = DSMR_ENTRY_TYPE_DSH; /* Initialize the LWLock tranche for the hash table. */ - dsh_state->tranche = LWLockNewTrancheId(); - LWLockRegisterTranche(dsh_state->tranche, name); + dsh_state->tranche = LWLockNewTrancheId(name); /* Initialize the DSA for the hash table. */ dsa = dsa_create(dsh_state->tranche); @@ -409,9 +404,6 @@ GetNamedDSHash(const char *name, const dshash_parameters *params, bool *found) ereport(ERROR, (errmsg("requested DSHash already attached to current process"))); - /* Initialize existing LWLock tranche for the hash table. */ - LWLockRegisterTranche(dsh_state->tranche, name); - /* Attach to existing DSA for the hash table. */ dsa = dsa_attach(dsh_state->dsa_handle); dsa_pin_mapping(dsa); diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index a4aecd1fbc3..b4636d86ca6 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -126,8 +126,8 @@ StaticAssertDecl((LW_VAL_EXCLUSIVE & LW_FLAG_MASK) == 0, * in lwlocklist.h. We absorb the names of these tranches, too. * * 3. Extensions can create new tranches, via either RequestNamedLWLockTranche - * or LWLockRegisterTranche. The names of these that are known in the current - * process appear in LWLockTrancheNames[]. + * or LWLockNewTrancheId. These names are stored in shared memory and can be + * accessed via NamedLWLockTrancheNames. * * All these names are user-visible as wait event names, so choose with care * ... and do not forget to update the documentation's list of wait events. @@ -146,11 +146,12 @@ StaticAssertDecl(lengthof(BuiltinTrancheNames) == /* * This is indexed by tranche ID minus LWTRANCHE_FIRST_USER_DEFINED, and - * stores the names of all dynamically-created tranches known to the current - * process. Any unused entries in the array will contain NULL. + * points to the shared memory locations of the names of all + * dynamically-created tranches. Backends inherit the pointer by fork from the + * postmaster (except in the EXEC_BACKEND case, where we have special measures + * to pass it down). */ -static const char **LWLockTrancheNames = NULL; -static int LWLockTrancheNamesAllocated = 0; +char **NamedLWLockTrancheNames = NULL; /* * This points to the main array of LWLocks in shared memory. Backends inherit @@ -184,20 +185,22 @@ typedef struct NamedLWLockTrancheRequest } NamedLWLockTrancheRequest; static NamedLWLockTrancheRequest *NamedLWLockTrancheRequestArray = NULL; -static int NamedLWLockTrancheRequestsAllocated = 0; /* - * NamedLWLockTrancheRequests is both the valid length of the request array, - * and the length of the shared-memory NamedLWLockTrancheArray later on. - * This variable and NamedLWLockTrancheArray are non-static so that - * postmaster.c can copy them to child processes in EXEC_BACKEND builds. + * NamedLWLockTrancheRequests is the valid length of the request array. This + * variable is non-static so that postmaster.c can copy them to child processes + * in EXEC_BACKEND builds. */ int NamedLWLockTrancheRequests = 0; /* points to data in shared memory: */ -NamedLWLockTranche *NamedLWLockTrancheArray = NULL; int *LWLockCounter = NULL; +/* backend-local counter of registered tranches */ +static int LocalLWLockCounter; + +#define MAX_NAMED_TRANCHES 256 + static void InitializeLWLocks(void); static inline void LWLockReportWaitStart(LWLock *lock); static inline void LWLockReportWaitEnd(void); @@ -392,31 +395,28 @@ Size LWLockShmemSize(void) { Size size; - int i; int numLocks = NUM_FIXED_LWLOCKS; /* Calculate total number of locks needed in the main array. */ numLocks += NumLWLocksForNamedTranches(); - /* Space for dynamic allocation counter, plus room for alignment. */ - size = sizeof(int) + LWLOCK_PADDED_SIZE; + /* Space for dynamic allocation counter. */ + size = MAXALIGN(sizeof(int)); - /* Space for the LWLock array. */ - size = add_size(size, mul_size(numLocks, sizeof(LWLockPadded))); + /* Space for named tranches. */ + size = add_size(size, mul_size(MAX_NAMED_TRANCHES, sizeof(char *))); + size = add_size(size, mul_size(MAX_NAMED_TRANCHES, NAMEDATALEN)); - /* space for named tranches. */ - size = add_size(size, mul_size(NamedLWLockTrancheRequests, sizeof(NamedLWLockTranche))); - - /* space for name of each tranche. */ - for (i = 0; i < NamedLWLockTrancheRequests; i++) - size = add_size(size, strlen(NamedLWLockTrancheRequestArray[i].tranche_name) + 1); + /* Space for the LWLock array, plus room for cache line alignment. */ + size = add_size(size, LWLOCK_PADDED_SIZE); + size = add_size(size, mul_size(numLocks, sizeof(LWLockPadded))); return size; } /* * Allocate shmem space for the main LWLock array and all tranches and - * initialize it. We also register extension LWLock tranches here. + * initialize it. */ void CreateLWLocks(void) @@ -432,7 +432,16 @@ CreateLWLocks(void) /* Initialize the dynamic-allocation counter for tranches */ LWLockCounter = (int *) ptr; *LWLockCounter = LWTRANCHE_FIRST_USER_DEFINED; - ptr += sizeof(int); + ptr += MAXALIGN(sizeof(int)); + + /* Initialize tranche names */ + NamedLWLockTrancheNames = (char **) ptr; + ptr += MAX_NAMED_TRANCHES * sizeof(char *); + for (int i = 0; i < MAX_NAMED_TRANCHES; i++) + { + NamedLWLockTrancheNames[i] = ptr; + ptr += NAMEDATALEN; + } /* Ensure desired alignment of LWLock array */ ptr += LWLOCK_PADDED_SIZE - ((uintptr_t) ptr) % LWLOCK_PADDED_SIZE; @@ -441,11 +450,6 @@ CreateLWLocks(void) /* Initialize all LWLocks */ InitializeLWLocks(); } - - /* Register named extension LWLock tranches in the current process. */ - for (int i = 0; i < NamedLWLockTrancheRequests; i++) - LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId, - NamedLWLockTrancheArray[i].trancheName); } /* @@ -454,7 +458,6 @@ CreateLWLocks(void) static void InitializeLWLocks(void) { - int numNamedLocks = NumLWLocksForNamedTranches(); int id; int i; int j; @@ -485,32 +488,18 @@ InitializeLWLocks(void) */ if (NamedLWLockTrancheRequests > 0) { - char *trancheNames; - - NamedLWLockTrancheArray = (NamedLWLockTranche *) - &MainLWLockArray[NUM_FIXED_LWLOCKS + numNamedLocks]; - - trancheNames = (char *) NamedLWLockTrancheArray + - (NamedLWLockTrancheRequests * sizeof(NamedLWLockTranche)); lock = &MainLWLockArray[NUM_FIXED_LWLOCKS]; for (i = 0; i < NamedLWLockTrancheRequests; i++) { NamedLWLockTrancheRequest *request; - NamedLWLockTranche *tranche; - char *name; + int tranche; request = &NamedLWLockTrancheRequestArray[i]; - tranche = &NamedLWLockTrancheArray[i]; - - name = trancheNames; - trancheNames += strlen(request->tranche_name) + 1; - strcpy(name, request->tranche_name); - tranche->trancheId = LWLockNewTrancheId(); - tranche->trancheName = name; + tranche = LWLockNewTrancheId(request->tranche_name); for (j = 0; j < request->num_lwlocks; j++, lock++) - LWLockInitialize(&lock->lock, tranche->trancheId); + LWLockInitialize(&lock->lock, tranche); } } } @@ -562,59 +551,44 @@ GetNamedLWLockTranche(const char *tranche_name) } /* - * Allocate a new tranche ID. + * Allocate a new tranche ID with the provided name. */ int -LWLockNewTrancheId(void) +LWLockNewTrancheId(const char *name) { int result; - /* We use the ShmemLock spinlock to protect LWLockCounter */ - SpinLockAcquire(ShmemLock); - result = (*LWLockCounter)++; - SpinLockRelease(ShmemLock); + if (!name) + ereport(ERROR, + (errcode(ERRCODE_INVALID_NAME), + errmsg("tranche name cannot be NULL"))); - return result; -} + if (strlen(name) >= NAMEDATALEN) + ereport(ERROR, + (errcode(ERRCODE_NAME_TOO_LONG), + errmsg("tranche name too long"), + errdetail("LWLock tranche names must be no longer than %d bytes.", + NAMEDATALEN - 1))); -/* - * Register a dynamic tranche name in the lookup table of the current process. - * - * This routine will save a pointer to the tranche name passed as an argument, - * so the name should be allocated in a backend-lifetime context - * (shared memory, TopMemoryContext, static constant, or similar). - * - * The tranche name will be user-visible as a wait event name, so try to - * use a name that fits the style for those. - */ -void -LWLockRegisterTranche(int tranche_id, const char *tranche_name) -{ - /* This should only be called for user-defined tranches. */ - if (tranche_id < LWTRANCHE_FIRST_USER_DEFINED) - return; - - /* Convert to array index. */ - tranche_id -= LWTRANCHE_FIRST_USER_DEFINED; + /* We use the ShmemLock spinlock to protect LWLockCounter */ + SpinLockAcquire(ShmemLock); - /* If necessary, create or enlarge array. */ - if (tranche_id >= LWLockTrancheNamesAllocated) + if (*LWLockCounter - LWTRANCHE_FIRST_USER_DEFINED >= MAX_NAMED_TRANCHES) { - int newalloc; + SpinLockRelease(ShmemLock); + ereport(ERROR, + (errmsg("maximum number of tranches already registered"), + errdetail("No more than %d tranches may be registered.", + MAX_NAMED_TRANCHES))); + } - newalloc = pg_nextpower2_32(Max(8, tranche_id + 1)); + result = (*LWLockCounter)++; + LocalLWLockCounter = *LWLockCounter; + strlcpy(NamedLWLockTrancheNames[result - LWTRANCHE_FIRST_USER_DEFINED], name, NAMEDATALEN); - if (LWLockTrancheNames == NULL) - LWLockTrancheNames = (const char **) - MemoryContextAllocZero(TopMemoryContext, - newalloc * sizeof(char *)); - else - LWLockTrancheNames = - repalloc0_array(LWLockTrancheNames, const char *, LWLockTrancheNamesAllocated, newalloc); - LWLockTrancheNamesAllocated = newalloc; - } + SpinLockRelease(ShmemLock); - LWLockTrancheNames[tranche_id] = tranche_name; + return result; } /* @@ -637,27 +611,33 @@ RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks) if (!process_shmem_requests_in_progress) elog(FATAL, "cannot request additional LWLocks outside shmem_request_hook"); + if (!tranche_name) + ereport(ERROR, + (errcode(ERRCODE_INVALID_NAME), + errmsg("tranche name cannot be NULL"))); + + if (strlen(tranche_name) >= NAMEDATALEN) + ereport(ERROR, + (errcode(ERRCODE_NAME_TOO_LONG), + errmsg("tranche name too long"), + errdetail("LWLock tranche names must be no longer than %d bytes.", + NAMEDATALEN - 1))); + if (NamedLWLockTrancheRequestArray == NULL) { - NamedLWLockTrancheRequestsAllocated = 16; NamedLWLockTrancheRequestArray = (NamedLWLockTrancheRequest *) MemoryContextAlloc(TopMemoryContext, - NamedLWLockTrancheRequestsAllocated + MAX_NAMED_TRANCHES * sizeof(NamedLWLockTrancheRequest)); } - if (NamedLWLockTrancheRequests >= NamedLWLockTrancheRequestsAllocated) - { - int i = pg_nextpower2_32(NamedLWLockTrancheRequests + 1); - - NamedLWLockTrancheRequestArray = (NamedLWLockTrancheRequest *) - repalloc(NamedLWLockTrancheRequestArray, - i * sizeof(NamedLWLockTrancheRequest)); - NamedLWLockTrancheRequestsAllocated = i; - } + if (NamedLWLockTrancheRequests >= MAX_NAMED_TRANCHES) + ereport(ERROR, + (errmsg("maximum number of tranches already registered"), + errdetail("No more than %d tranches may be registered.", + MAX_NAMED_TRANCHES))); request = &NamedLWLockTrancheRequestArray[NamedLWLockTrancheRequests]; - Assert(strlen(tranche_name) + 1 <= NAMEDATALEN); strlcpy(request->tranche_name, tranche_name, NAMEDATALEN); request->num_lwlocks = num_lwlocks; NamedLWLockTrancheRequests++; @@ -669,6 +649,9 @@ RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks) void LWLockInitialize(LWLock *lock, int tranche_id) { + /* verify the tranche_id is valid */ + (void) GetLWTrancheName(tranche_id); + pg_atomic_init_u32(&lock->state, LW_FLAG_RELEASE_OK); #ifdef LOCK_DEBUG pg_atomic_init_u32(&lock->nwaiters, 0); @@ -709,18 +692,23 @@ GetLWTrancheName(uint16 trancheId) if (trancheId < LWTRANCHE_FIRST_USER_DEFINED) return BuiltinTrancheNames[trancheId]; + /* verify the trancheId is valid */ + if (trancheId >= LocalLWLockCounter) + { + SpinLockAcquire(ShmemLock); + LocalLWLockCounter = *LWLockCounter; + SpinLockRelease(ShmemLock); + + if (trancheId >= LocalLWLockCounter) + elog(ERROR, "tranche %d is not registered", trancheId); + } + /* - * It's an extension tranche, so look in LWLockTrancheNames[]. However, - * it's possible that the tranche has never been registered in the current - * process, in which case give up and return "extension". + * It's an extension tranche, so look in NamedLWLockTrancheNames. */ trancheId -= LWTRANCHE_FIRST_USER_DEFINED; - if (trancheId >= LWLockTrancheNamesAllocated || - LWLockTrancheNames[trancheId] == NULL) - return "extension"; - - return LWLockTrancheNames[trancheId]; + return NamedLWLockTrancheNames[trancheId]; } /* diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index f9cf57f8d26..3877aaa7f03 100644 --- a/src/include/storage/lwlock.h +++ b/src/include/storage/lwlock.h @@ -73,14 +73,7 @@ typedef union LWLockPadded extern PGDLLIMPORT LWLockPadded *MainLWLockArray; -/* struct for storing named tranche information */ -typedef struct NamedLWLockTranche -{ - int trancheId; - char *trancheName; -} NamedLWLockTranche; - -extern PGDLLIMPORT NamedLWLockTranche *NamedLWLockTrancheArray; +extern PGDLLIMPORT char **NamedLWLockTrancheNames; extern PGDLLIMPORT int NamedLWLockTrancheRequests; extern PGDLLIMPORT int *LWLockCounter; @@ -158,18 +151,11 @@ extern LWLockPadded *GetNamedLWLockTranche(const char *tranche_name); /* * There is another, more flexible method of obtaining lwlocks. First, call - * LWLockNewTrancheId just once to obtain a tranche ID; this allocates from - * a shared counter. Next, each individual process using the tranche should - * call LWLockRegisterTranche() to associate that tranche ID with a name. - * Finally, LWLockInitialize should be called just once per lwlock, passing - * the tranche ID as an argument. - * - * It may seem strange that each process using the tranche must register it - * separately, but dynamic shared memory segments aren't guaranteed to be - * mapped at the same address in all coordinating backends, so storing the - * registration in the main shared memory segment wouldn't work for that case. + * LWLockNewTrancheId to obtain a tranche ID; this allocates from a shared + * counter. Second, LWLockInitialize should be called just once per lwlock, + * passing the tranche ID as an argument. */ -extern int LWLockNewTrancheId(void); +extern int LWLockNewTrancheId(const char *name); extern void LWLockRegisterTranche(int tranche_id, const char *tranche_name); extern void LWLockInitialize(LWLock *lock, int tranche_id); diff --git a/src/test/modules/test_dsa/test_dsa.c b/src/test/modules/test_dsa/test_dsa.c index cd24d0f4873..01d5c6fa67f 100644 --- a/src/test/modules/test_dsa/test_dsa.c +++ b/src/test/modules/test_dsa/test_dsa.c @@ -29,8 +29,7 @@ test_dsa_basic(PG_FUNCTION_ARGS) dsa_pointer p[100]; /* XXX: this tranche is leaked */ - tranche_id = LWLockNewTrancheId(); - LWLockRegisterTranche(tranche_id, "test_dsa"); + tranche_id = LWLockNewTrancheId("test_dsa"); a = dsa_create(tranche_id); for (int i = 0; i < 100; i++) @@ -70,8 +69,7 @@ test_dsa_resowners(PG_FUNCTION_ARGS) ResourceOwner childowner; /* XXX: this tranche is leaked */ - tranche_id = LWLockNewTrancheId(); - LWLockRegisterTranche(tranche_id, "test_dsa"); + tranche_id = LWLockNewTrancheId("test_dsa"); /* Create DSA in parent resource owner */ a = dsa_create(tranche_id); diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry.c b/src/test/modules/test_dsm_registry/test_dsm_registry.c index 141c8ed1b34..4cc2ccdac3f 100644 --- a/src/test/modules/test_dsm_registry/test_dsm_registry.c +++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c @@ -48,7 +48,7 @@ init_tdr_dsm(void *ptr) { TestDSMRegistryStruct *dsm = (TestDSMRegistryStruct *) ptr; - LWLockInitialize(&dsm->lck, LWLockNewTrancheId()); + LWLockInitialize(&dsm->lck, LWLockNewTrancheId("test_dsm_registry")); dsm->val = 0; } @@ -61,7 +61,6 @@ tdr_attach_shmem(void) sizeof(TestDSMRegistryStruct), init_tdr_dsm, &found); - LWLockRegisterTranche(tdr_dsm->lck.tranche, "test_dsm_registry"); if (tdr_dsa == NULL) tdr_dsa = GetNamedDSA("test_dsm_registry_dsa", &found); diff --git a/src/test/modules/test_radixtree/test_radixtree.c b/src/test/modules/test_radixtree/test_radixtree.c index 80ad0296164..787162c8793 100644 --- a/src/test/modules/test_radixtree/test_radixtree.c +++ b/src/test/modules/test_radixtree/test_radixtree.c @@ -124,10 +124,9 @@ test_empty(void) rt_iter *iter; uint64 key; #ifdef TEST_SHARED_RT - int tranche_id = LWLockNewTrancheId(); + int tranche_id = LWLockNewTrancheId("test_radix_tree"); dsa_area *dsa; - LWLockRegisterTranche(tranche_id, "test_radix_tree"); dsa = dsa_create(tranche_id); radixtree = rt_create(dsa, tranche_id); #else @@ -167,10 +166,9 @@ test_basic(rt_node_class_test_elem *test_info, int shift, bool asc) uint64 *keys; int children = test_info->nkeys; #ifdef TEST_SHARED_RT - int tranche_id = LWLockNewTrancheId(); + int tranche_id = LWLockNewTrancheId("test_radix_tree"); dsa_area *dsa; - LWLockRegisterTranche(tranche_id, "test_radix_tree"); dsa = dsa_create(tranche_id); radixtree = rt_create(dsa, tranche_id); #else @@ -304,10 +302,9 @@ test_random(void) int num_keys = 100000; uint64 *keys; #ifdef TEST_SHARED_RT - int tranche_id = LWLockNewTrancheId(); + int tranche_id = LWLockNewTrancheId("test_radix_tree"); dsa_area *dsa; - LWLockRegisterTranche(tranche_id, "test_radix_tree"); dsa = dsa_create(tranche_id); radixtree = rt_create(dsa, tranche_id); #else diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c index 32750930e43..8c0367eeee4 100644 --- a/src/test/modules/test_slru/test_slru.c +++ b/src/test/modules/test_slru/test_slru.c @@ -232,11 +232,9 @@ test_slru_shmem_startup(void) (void) MakePGDirectory(slru_dir_name); /* initialize the SLRU facility */ - test_tranche_id = LWLockNewTrancheId(); - LWLockRegisterTranche(test_tranche_id, "test_slru_tranche"); + test_tranche_id = LWLockNewTrancheId("test_slru_tranche"); - test_buffer_tranche_id = LWLockNewTrancheId(); - LWLockRegisterTranche(test_tranche_id, "test_buffer_tranche"); + test_buffer_tranche_id = LWLockNewTrancheId("test_buffer_tranche"); TestSlruCtl->PagePrecedes = test_slru_page_precedes_logically; SimpleLruInit(TestSlruCtl, "TestSLRU", diff --git a/src/test/modules/test_tidstore/test_tidstore.c b/src/test/modules/test_tidstore/test_tidstore.c index eb16e0fbfa6..0c8f43867e5 100644 --- a/src/test/modules/test_tidstore/test_tidstore.c +++ b/src/test/modules/test_tidstore/test_tidstore.c @@ -103,8 +103,7 @@ test_create(PG_FUNCTION_ARGS) { int tranche_id; - tranche_id = LWLockNewTrancheId(); - LWLockRegisterTranche(tranche_id, "test_tidstore"); + tranche_id = LWLockNewTrancheId("test_tidstore"); tidstore = TidStoreCreateShared(tidstore_max_size, tranche_id); -- 2.39.5 (Apple Git-154)
>From 65c80071e74c36328873dfa9cf6ce8f537d83850 Mon Sep 17 00:00:00 2001 From: Sami Imseih <sims...@amazon.com> Date: Fri, 22 Aug 2025 00:35:45 -0500 Subject: [PATCH v20 2/2] Tests for LWLock tranche registration improvements --- src/test/modules/Makefile | 3 +- src/test/modules/meson.build | 1 + src/test/modules/test_tranches/.gitignore | 4 + src/test/modules/test_tranches/Makefile | 23 +++ src/test/modules/test_tranches/meson.build | 33 +++ .../test_tranches/t/001_test_tranches.pl | 120 +++++++++++ .../test_tranches/test_tranches--1.0.sql | 35 ++++ .../modules/test_tranches/test_tranches.c | 191 ++++++++++++++++++ .../test_tranches/test_tranches.control | 6 + 9 files changed, 415 insertions(+), 1 deletion(-) create mode 100644 src/test/modules/test_tranches/.gitignore create mode 100644 src/test/modules/test_tranches/Makefile create mode 100644 src/test/modules/test_tranches/meson.build create mode 100644 src/test/modules/test_tranches/t/001_test_tranches.pl create mode 100644 src/test/modules/test_tranches/test_tranches--1.0.sql create mode 100644 src/test/modules/test_tranches/test_tranches.c create mode 100644 src/test/modules/test_tranches/test_tranches.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 903a8ac151a..fd7aa4675c5 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -44,7 +44,8 @@ SUBDIRS = \ test_tidstore \ unsafe_tests \ worker_spi \ - xid_wraparound + xid_wraparound \ + test_tranches ifeq ($(enable_injection_points),yes) diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 93be0f57289..52883dfb496 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -45,3 +45,4 @@ subdir('typcache') subdir('unsafe_tests') subdir('worker_spi') subdir('xid_wraparound') +subdir('test_tranches') diff --git a/src/test/modules/test_tranches/.gitignore b/src/test/modules/test_tranches/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_tranches/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_tranches/Makefile b/src/test/modules/test_tranches/Makefile new file mode 100644 index 00000000000..5c9e78084da --- /dev/null +++ b/src/test/modules/test_tranches/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_tranches/Makefile + +MODULE_big = test_tranches +OBJS = \ + $(WIN32RES) \ + test_tranches.o +PGFILEDESC = "test_tranches - test code LWLock tranche management" + +EXTENSION = test_tranches +DATA = test_tranches--1.0.sql + +TAP_TESTS = 1 + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_tranches +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_tranches/meson.build b/src/test/modules/test_tranches/meson.build new file mode 100644 index 00000000000..976ce9f0a25 --- /dev/null +++ b/src/test/modules/test_tranches/meson.build @@ -0,0 +1,33 @@ +# Copyright (c) 2024-2025, PostgreSQL Global Development Group + +test_tranches_sources = files( + 'test_tranches.c', +) + +if host_system == 'windows' + test_tranches_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_tranches', + '--FILEDESC', 'test_tranches - test code LWLock tranche management',]) +endif + +test_tranches = shared_module('test_tranches', + test_tranches_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_tranches + +test_install_data += files( + 'test_tranches.control', + 'test_tranches--1.0.sql', +) + +tests += { + 'name': 'test_tranches', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'tap': { + 'tests': [ + 't/001_test_tranches.pl', + ], + }, +} \ No newline at end of file diff --git a/src/test/modules/test_tranches/t/001_test_tranches.pl b/src/test/modules/test_tranches/t/001_test_tranches.pl new file mode 100644 index 00000000000..477035bdb99 --- /dev/null +++ b/src/test/modules/test_tranches/t/001_test_tranches.pl @@ -0,0 +1,120 @@ +use strict; +use warnings FATAL => 'all'; + +use List::Util qw(min); +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# +# Create the cluster without any requested tranches +# +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init(); +$node->append_conf('postgresql.conf', + qq(shared_preload_libraries='test_tranches')); +$node->append_conf('postgresql.conf', + qq(test_tranches.requested_named_tranches=0)); +$node->start(); +$node->safe_psql('postgres', q(CREATE EXTENSION test_tranches)); + +my $first_user_defined = $node->safe_psql('postgres', q(select test_tranches_get_first_user_defined())); +my @user_defined = ($first_user_defined .. $first_user_defined + 5); +my ($second_user_defined, + $third_user_defined, + $fourth_user_defined, + $fifth_user_defined, + $sixth_user_defined) = @user_defined[1..5]; +# +# Run lookup tests to ensure the correct tranche names are returned +# and an error is raised for unregistered tranches +# +$node->safe_psql('postgres', "select test_tranches_new(5);"); +my ($result, $stdout, $stderr) = $node->psql('postgres', + qq{ + select test_tranches_lookup(1); + select test_tranches_lookup($first_user_defined); + select test_tranches_lookup($second_user_defined); + select test_tranches_lookup($third_user_defined); + select test_tranches_lookup($fourth_user_defined); + select test_tranches_lookup($fifth_user_defined); + select test_tranches_lookup($sixth_user_defined); + }, + on_error_stop => 0); +like("$stderr", qr/ERROR: tranche 100 is not registered/, "unregistered tranche error"); +like("$stdout", + qr/ShmemIndex.*test_lock__0.*test_lock__1.*test_lock__2.*test_lock__3.*test_lock__4/s, + "match tranche names without requested tranches"); + +# +# Test the error for long tranche names +# +my $good_tranche_name = 'A' x 63; +my $bad_tranche_name = 'B' x 64; # MAX_NAMED_TRANCHES_NAME_LEN +$node->safe_psql('postgres', qq{select test_tranches_new_tranche('$good_tranche_name');}); +($result, $stdout, $stderr) = $node->psql('postgres', + qq{ + select test_tranches_new_tranche('$bad_tranche_name'); + }, + on_error_stop => 0); +like("$stderr", qr/ERROR: tranche name too long/, "tranche name too long"); + +$node->restart(); + +# +# Test the error when > MAX_NAMED_TRANCHES named tranches registered. +# +$node->safe_psql('postgres', qq{select test_tranches_new(255)}); +$node->safe_psql('postgres', qq{select test_tranches_new(1)}); +($result, $stdout, $stderr) = $node->psql('postgres', qq{select test_tranches_new(1);}, on_error_stop => 0); +like("$stderr", qr/ERROR: maximum number of tranches already registered/, "too many tranches registered"); + +# +# Repeat the lookup test with 2 requested tranches +# +$node->append_conf('postgresql.conf', + qq(test_tranches.requested_named_tranches=2)); +$node->restart(); + +$first_user_defined = $first_user_defined; +@user_defined = ($first_user_defined .. $first_user_defined + 5); +($second_user_defined, + $third_user_defined, + $fourth_user_defined, + $fifth_user_defined, + $sixth_user_defined) = @user_defined[1..5]; + +$node->safe_psql('postgres', "select test_tranches_new(3);"); +($result, $stdout, $stderr) = $node->psql('postgres', + qq{ + select test_tranches_lookup(1); + select test_tranches_lookup($first_user_defined); + select test_tranches_lookup($second_user_defined); + select test_tranches_lookup($third_user_defined); + select test_tranches_lookup($fourth_user_defined); + select test_tranches_lookup($fifth_user_defined); + select test_tranches_lookup($sixth_user_defined); + }, + on_error_stop => 0); +like("$stderr", qr/ERROR: tranche 100 is not registered/, "unregistered tranche error"); +like("$stdout", qr/ShmemIndex.*test_lock_0.*test_lock_1.*test_lock__2.*test_lock__3.*test_lock__4/s, + "match tranche names with requested tranches"); + +# +# Test lwlock initialize +# +$node->safe_psql('postgres', qq{select test_tranches_lwlock_initialize($first_user_defined)}); +($result, $stdout, $stderr) = $node->psql('postgres', + qq{select test_tranches_lwlock_initialize($sixth_user_defined)}, + on_error_stop => 0); +like("$stderr", qr/ERROR: tranche 100 is not registered/, "LWLock intialization error on invalid tranche name"); + +# +# Test error for NULL tranche name +# +($result, $stdout, $stderr) = $node->psql('postgres', + qq{select test_tranches_new_tranche(NULL)}, + on_error_stop => 0); +like("$stderr", qr/ERROR: tranche name cannot be NULL/, "NULL tranche name"); + +done_testing(); diff --git a/src/test/modules/test_tranches/test_tranches--1.0.sql b/src/test/modules/test_tranches/test_tranches--1.0.sql new file mode 100644 index 00000000000..f504098e04a --- /dev/null +++ b/src/test/modules/test_tranches/test_tranches--1.0.sql @@ -0,0 +1,35 @@ +-- test_tranches--1.0.sql + +CREATE FUNCTION test_tranches_new(bigint) +RETURNS void +AS 'MODULE_PATHNAME', 'test_tranches_new' +LANGUAGE C STRICT; + +CREATE FUNCTION test_tranches_lookup(int) +RETURNS text +AS 'MODULE_PATHNAME', 'test_tranches_lookup' +LANGUAGE C STRICT; + +CREATE FUNCTION test_tranches_get_named_lwlock(text, int) +RETURNS int +AS 'MODULE_PATHNAME', 'test_tranches_get_named_lwlock' +LANGUAGE C STRICT; + +CREATE FUNCTION test_tranches_get_first_user_defined() +RETURNS int +AS 'MODULE_PATHNAME', 'test_tranches_get_first_user_defined' +LANGUAGE C STRICT; + +CREATE FUNCTION test_tranches_lwlock_initialize(int) +RETURNS void +AS 'MODULE_PATHNAME', 'test_tranches_lwlock_initialize' +LANGUAGE C STRICT; + +/* + * Function is CALLED ON NULL INPUT to allow NULL input + * for testing + */ +CREATE FUNCTION test_tranches_new_tranche(text) +RETURNS int +AS 'MODULE_PATHNAME', 'test_tranches_new_tranche' +LANGUAGE C; \ No newline at end of file diff --git a/src/test/modules/test_tranches/test_tranches.c b/src/test/modules/test_tranches/test_tranches.c new file mode 100644 index 00000000000..7d3031ace9f --- /dev/null +++ b/src/test/modules/test_tranches/test_tranches.c @@ -0,0 +1,191 @@ +#include "postgres.h" + +#include "fmgr.h" +#include "miscadmin.h" +#include "storage/dsm_registry.h" +#include "storage/ipc.h" +#include "storage/lwlock.h" +#include "storage/shmem.h" +#include "utils/builtins.h" +#include "utils/guc.h" +#include "utils/injection_point.h" +#include "utils/wait_classes.h" + +PG_MODULE_MAGIC; + +/* hooks */ +static shmem_request_hook_type prev_shmem_request_hook = NULL; +static shmem_startup_hook_type prev_shmem_startup_hook = NULL; +static void test_tranches_shmem_request(void); +static void test_tranches_shmem_startup(void); + +/* GUC */ +static int test_tranches_requested_named_tranches = 0; + +typedef struct testTranchesSharedState +{ + int next_index; +} testTranchesSharedState; + +static testTranchesSharedState * test_lwlock_ss = NULL; + +/* + * LWLock wait event masks. Copied from src/backend/utils/activity/wait_event.c + */ +#define WAIT_EVENT_CLASS_MASK 0xFF000000 + +/* + * Module load callback + */ +void +_PG_init(void) +{ + prev_shmem_request_hook = shmem_request_hook; + shmem_request_hook = test_tranches_shmem_request; + prev_shmem_startup_hook = shmem_startup_hook; + shmem_startup_hook = test_tranches_shmem_startup; + + DefineCustomIntVariable("test_tranches.requested_named_tranches", + "Sets the number of locks created during shmem request", + NULL, + &test_tranches_requested_named_tranches, + 2, + 0, + UINT16_MAX, + PGC_POSTMASTER, + 0, + NULL, + NULL, + NULL); +} + +static void +test_tranches_shmem_startup(void) +{ + bool found; + + if (prev_shmem_startup_hook) + prev_shmem_startup_hook(); + + test_lwlock_ss = NULL; + + test_lwlock_ss = ShmemInitStruct("test_tranches", + sizeof(testTranchesSharedState), + &found); + if (!found) + { + test_lwlock_ss->next_index = test_tranches_requested_named_tranches; + } +} + +static Size +test_tranches_memsize(void) +{ + Size size; + + size = MAXALIGN(sizeof(testTranchesSharedState)); + + return size; +} + +static void +test_tranches_shmem_request(void) +{ + int i = 0; + + if (prev_shmem_request_hook) + prev_shmem_request_hook(); + + RequestAddinShmemSpace(test_tranches_memsize()); + + for (i = 0; i < test_tranches_requested_named_tranches; i++) + { + char name[15]; + + snprintf(name, sizeof(name), "test_lock_%d", i); + RequestNamedLWLockTranche(name, i); + } +} + +PG_FUNCTION_INFO_V1(test_tranches_new); +Datum +test_tranches_new(PG_FUNCTION_ARGS) +{ + int64 num = PG_GETARG_INT64(0); + int i; + + for (i = test_lwlock_ss->next_index; i < num + test_lwlock_ss->next_index; i++) + { + char name[50]; + + snprintf(name, 50, "test_lock__%d", i); + + LWLockNewTrancheId(name); + } + + test_lwlock_ss->next_index = i; + + PG_RETURN_VOID(); +} + +PG_FUNCTION_INFO_V1(test_tranches_new_tranche); +Datum +test_tranches_new_tranche(PG_FUNCTION_ARGS) +{ + char *tranche_name = NULL; + + if (!PG_ARGISNULL(0)) + tranche_name = text_to_cstring(PG_GETARG_TEXT_PP(0)); + + PG_RETURN_INT32(LWLockNewTrancheId(tranche_name)); +} + +PG_FUNCTION_INFO_V1(test_tranches_lookup); +Datum +test_tranches_lookup(PG_FUNCTION_ARGS) +{ + const char *tranche_name = GetLWLockIdentifier(PG_WAIT_LWLOCK & WAIT_EVENT_CLASS_MASK, PG_GETARG_INT32(0)); + + if (tranche_name) + PG_RETURN_TEXT_P(cstring_to_text(tranche_name)); + else + PG_RETURN_NULL(); +} + +PG_FUNCTION_INFO_V1(test_tranches_get_named_lwlock); +Datum +test_tranches_get_named_lwlock(PG_FUNCTION_ARGS) +{ + int i; + LWLockPadded *locks; + + locks = GetNamedLWLockTranche(text_to_cstring(PG_GETARG_TEXT_PP(0))); + + for (i = 0; i < PG_GETARG_INT32(1); i++) + { + LWLock *lock = &locks[i].lock; + + LWLockAcquire(lock, LW_SHARED); + LWLockRelease(lock); + } + + PG_RETURN_INT32(i); +} + +PG_FUNCTION_INFO_V1(test_tranches_get_first_user_defined); +Datum +test_tranches_get_first_user_defined(PG_FUNCTION_ARGS) +{ + return LWTRANCHE_FIRST_USER_DEFINED; +} + +PG_FUNCTION_INFO_V1(test_tranches_lwlock_initialize); +Datum +test_tranches_lwlock_initialize(PG_FUNCTION_ARGS) +{ + LWLock lock; + + LWLockInitialize(&lock, PG_GETARG_INT32(0)); + + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_tranches/test_tranches.control b/src/test/modules/test_tranches/test_tranches.control new file mode 100644 index 00000000000..8e6254a7e43 --- /dev/null +++ b/src/test/modules/test_tranches/test_tranches.control @@ -0,0 +1,6 @@ +# test_tranches.control + +comment = 'Test LWLock tranch names tracking' +default_version = '1.0' +relocatable = false +module_pathname = '$libdir/test_tranches' \ No newline at end of file -- 2.39.5 (Apple Git-154)