Starting new thread for this thing that Matthias noticed in my
work-in-progress patch at
https://www.postgresql.org/message-id/CAEze2WjgCROMMXY0+j8FFdm3iFcr7By-+6Mwiz=pggseydi...@mail.gmail.com.
On 05/04/2026 02:17, Matthias van de Meent wrote:
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. 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.
Good catch, RequestNamedLWLocktranche() was quite broken with the patch.
I'm surprised it didn't cause test failures. We even have unit tests for
that at src/test/modules/test_lwlock_tranches.
Looking at src/test/modules/test_lwlock_tranches, I realized that we
don't currently check that the tranche name registered with
RequestNamedLWLocktranche() is unique. If two extensions registered a
tranche with same name, we'd allocate two separate tranches for them,
but GetNamedLWLockTranche() would always return the first one.
Attached patches add a uniqueness check, and improves
test_lwlock_tranches so that it actually uses the requested LWLocks. And
I couldn't resist doing some more refactoring of the test while I was at
it; IMO it's more readable now.
Barring objections, I will commit these shortly.
- Heikki
From 3ddf87e520f74055a641c73da74baa4b05b1758f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Sun, 5 Apr 2026 17:28:04 +0300
Subject: [PATCH v1 1/2] Improve test_lwlock_tranches
While working on refactoring how shmem is allocated, I made a mistake
where the main LWLock array did not reserve space for the LWLocks
allocated with RequestNamedLWLockTranche(), and the test still
passed. Matthias spotted that before it got committed, but in order to
catch such mistakes in the future, add checks in test_lwlock_tranches
that the locks allocated with RequestNamedLWLockTranche() can be
acquired and released.
Another change is to stop requesting multiple tranches with the same
name with RequestNamedLWLockTranche(). As soon as I started to test
using the locks I realized that's bogus, and the next commit will
forbid it. Keep test coverage for duplicates requested with
LWLockNewTrancheId() for now, but make it more clear that that's what
the test does.
Discussion: https://www.postgresql.org/message-id/CAEze2WjgCROMMXY0+j8FFdm3iFcr7By-+6Mwiz=pggseydi...@mail.gmail.com
---
.../expected/test_lwlock_tranches.out | 61 ++++++++++---
.../sql/test_lwlock_tranches.sql | 44 +++++++--
.../test_lwlock_tranches--1.0.sql | 7 +-
.../test_lwlock_tranches.c | 90 +++++++++++--------
4 files changed, 146 insertions(+), 56 deletions(-)
diff --git a/src/test/modules/test_lwlock_tranches/expected/test_lwlock_tranches.out b/src/test/modules/test_lwlock_tranches/expected/test_lwlock_tranches.out
index 688a20bca5d..fc25e42c497 100644
--- a/src/test/modules/test_lwlock_tranches/expected/test_lwlock_tranches.out
+++ b/src/test/modules/test_lwlock_tranches/expected/test_lwlock_tranches.out
@@ -1,25 +1,60 @@
CREATE EXTENSION test_lwlock_tranches;
-SELECT test_lwlock_tranches();
- test_lwlock_tranches
+-- Test lock tranches created with RequestNamedLWLockTranche()
+SELECT test_startup_lwlocks();
+ test_startup_lwlocks
----------------------
(1 row)
-SELECT test_lwlock_tranche_creation(NULL);
+-- Test creating new lock tranches with LWLockNewTrancheId()
+CREATE TEMP TABLE test_tranches(tranche_name text, tranche_id int);
+INSERT INTO test_tranches VALUES
+ ('test_tranche_a', test_lwlock_tranche_create('test_tranche_a')),
+ ('test_tranche_b', test_lwlock_tranche_create('test_tranche_b'));
+-- You can create multiple tranches with the same name. It's confusing, so
+-- not recommended, but test it.
+INSERT INTO test_tranches VALUES
+ ('test_tranche_duplicate', test_lwlock_tranche_create('test_tranche_duplicate')),
+ ('test_tranche_duplicate', test_lwlock_tranche_create('test_tranche_duplicate'));
+-- Check that all tranches were assigned different tranche IDs, including
+-- the ones with duplicate names
+select count(distinct tranche_id) from test_tranches;
+ count
+-------
+ 4
+(1 row)
+
+-- Test GetLWLockIdentifier() on tranches created with LWLockNewTrancheId()
+select tranche_name, test_lwlock_get_lwlock_identifier(tranche_id) as lookup_result
+from test_tranches;
+ tranche_name | lookup_result
+------------------------+------------------------
+ test_tranche_a | test_tranche_a
+ test_tranche_b | test_tranche_b
+ test_tranche_duplicate | test_tranche_duplicate
+ test_tranche_duplicate | test_tranche_duplicate
+(4 rows)
+
+-- negative tests
+SELECT test_lwlock_tranche_create(NULL);
ERROR: tranche name cannot be NULL
-SELECT test_lwlock_tranche_creation(repeat('a', 64));
+SELECT test_lwlock_tranche_create(repeat('a', 64));
ERROR: tranche name too long
DETAIL: LWLock tranche names must be no longer than 63 bytes.
-SELECT test_lwlock_tranche_creation('test');
-ERROR: maximum number of tranches already registered
-DETAIL: No more than 256 tranches may be registered.
-SELECT test_lwlock_tranche_lookup('test_lwlock_tranches_startup');
- test_lwlock_tranche_lookup
-----------------------------
-
-(1 row)
-
SELECT test_lwlock_tranche_lookup('bogus');
ERROR: requested tranche is not registered
+SELECT test_lwlock_tranche_lookup('test_tranche_a');
+ERROR: requested tranche was not registered with RequestNamedLWLockTranche()
SELECT test_lwlock_initialize(65535);
ERROR: tranche 65535 is not registered
+-- Test what happens when you use up all the slots.
+--
+-- MAX_USER_DEFINED_TRANCHES is 256. Two locks were created with
+-- RequestNamedLWLockTranche() when the library was loaded, and we created
+-- four more.
+insert into test_tranches
+ select 'test_tranche_consume_all', test_lwlock_tranche_create('test_tranche_consume_all')
+ from generate_series(1, 256 - 2 - 4);
+select test_lwlock_tranche_create('out-of-tranches');
+ERROR: maximum number of tranches already registered
+DETAIL: No more than 256 tranches may be registered.
diff --git a/src/test/modules/test_lwlock_tranches/sql/test_lwlock_tranches.sql b/src/test/modules/test_lwlock_tranches/sql/test_lwlock_tranches.sql
index 4d085b8f25b..6f5fbcc3f23 100644
--- a/src/test/modules/test_lwlock_tranches/sql/test_lwlock_tranches.sql
+++ b/src/test/modules/test_lwlock_tranches/sql/test_lwlock_tranches.sql
@@ -1,8 +1,42 @@
CREATE EXTENSION test_lwlock_tranches;
-SELECT test_lwlock_tranches();
-SELECT test_lwlock_tranche_creation(NULL);
-SELECT test_lwlock_tranche_creation(repeat('a', 64));
-SELECT test_lwlock_tranche_creation('test');
-SELECT test_lwlock_tranche_lookup('test_lwlock_tranches_startup');
+
+-- Test lock tranches created with RequestNamedLWLockTranche()
+SELECT test_startup_lwlocks();
+
+-- Test creating new lock tranches with LWLockNewTrancheId()
+CREATE TEMP TABLE test_tranches(tranche_name text, tranche_id int);
+INSERT INTO test_tranches VALUES
+ ('test_tranche_a', test_lwlock_tranche_create('test_tranche_a')),
+ ('test_tranche_b', test_lwlock_tranche_create('test_tranche_b'));
+
+-- You can create multiple tranches with the same name. It's confusing, so
+-- not recommended, but test it.
+INSERT INTO test_tranches VALUES
+ ('test_tranche_duplicate', test_lwlock_tranche_create('test_tranche_duplicate')),
+ ('test_tranche_duplicate', test_lwlock_tranche_create('test_tranche_duplicate'));
+
+-- Check that all tranches were assigned different tranche IDs, including
+-- the ones with duplicate names
+select count(distinct tranche_id) from test_tranches;
+
+-- Test GetLWLockIdentifier() on tranches created with LWLockNewTrancheId()
+select tranche_name, test_lwlock_get_lwlock_identifier(tranche_id) as lookup_result
+from test_tranches;
+
+-- negative tests
+SELECT test_lwlock_tranche_create(NULL);
+SELECT test_lwlock_tranche_create(repeat('a', 64));
SELECT test_lwlock_tranche_lookup('bogus');
+SELECT test_lwlock_tranche_lookup('test_tranche_a');
SELECT test_lwlock_initialize(65535);
+
+-- Test what happens when you use up all the slots.
+--
+-- MAX_USER_DEFINED_TRANCHES is 256. Two locks were created with
+-- RequestNamedLWLockTranche() when the library was loaded, and we created
+-- four more.
+insert into test_tranches
+ select 'test_tranche_consume_all', test_lwlock_tranche_create('test_tranche_consume_all')
+ from generate_series(1, 256 - 2 - 4);
+
+select test_lwlock_tranche_create('out-of-tranches');
diff --git a/src/test/modules/test_lwlock_tranches/test_lwlock_tranches--1.0.sql b/src/test/modules/test_lwlock_tranches/test_lwlock_tranches--1.0.sql
index 34821981521..9111580db48 100644
--- a/src/test/modules/test_lwlock_tranches/test_lwlock_tranches--1.0.sql
+++ b/src/test/modules/test_lwlock_tranches/test_lwlock_tranches--1.0.sql
@@ -3,14 +3,17 @@
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION test_lwlock_tranches" to load this file. \quit
-CREATE FUNCTION test_lwlock_tranches() RETURNS VOID
+CREATE FUNCTION test_startup_lwlocks() RETURNS VOID
AS 'MODULE_PATHNAME' LANGUAGE C;
-CREATE FUNCTION test_lwlock_tranche_creation(tranche_name TEXT) RETURNS VOID
+CREATE FUNCTION test_lwlock_tranche_create(tranche_name TEXT) RETURNS INT
AS 'MODULE_PATHNAME' LANGUAGE C;
CREATE FUNCTION test_lwlock_tranche_lookup(tranche_name TEXT) RETURNS VOID
AS 'MODULE_PATHNAME' LANGUAGE C;
+CREATE FUNCTION test_lwlock_get_lwlock_identifier(event_id INT) RETURNS TEXT
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
CREATE FUNCTION test_lwlock_initialize(tranche_id INT) RETURNS VOID
AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_lwlock_tranches/test_lwlock_tranches.c b/src/test/modules/test_lwlock_tranches/test_lwlock_tranches.c
index e58bc49d420..9585578f18e 100644
--- a/src/test/modules/test_lwlock_tranches/test_lwlock_tranches.c
+++ b/src/test/modules/test_lwlock_tranches/test_lwlock_tranches.c
@@ -21,14 +21,6 @@
PG_MODULE_MAGIC;
-#define STARTUP_TRANCHE_NAME "test_lwlock_tranches_startup"
-#define DYNAMIC_TRANCHE_NAME "test_lwlock_tranches_dynamic"
-
-#define NUM_STARTUP_TRANCHES (32)
-#define NUM_DYNAMIC_TRANCHES (256 - NUM_STARTUP_TRANCHES)
-
-#define GET_TRANCHE_NAME(a) GetLWLockIdentifier(PG_WAIT_LWLOCK, (a))
-
static shmem_request_hook_type prev_shmem_request_hook;
static void test_lwlock_tranches_shmem_request(void);
@@ -45,36 +37,46 @@ test_lwlock_tranches_shmem_request(void)
if (prev_shmem_request_hook)
prev_shmem_request_hook();
- for (int i = 0; i < NUM_STARTUP_TRANCHES; i++)
- RequestNamedLWLockTranche(STARTUP_TRANCHE_NAME, 1);
+ /*
+ * Request some tranches with RequestNamedLWLockTranche() for
+ * test_startup_lwlocks()
+ */
+ RequestNamedLWLockTranche("test_lwlock_tranches_startup", 1);
+ RequestNamedLWLockTranche("test_lwlock_tranches_startup10", 10);
}
/*
- * Checks that GetLWLockIdentifier() returns the expected value for tranches
- * registered via RequestNamedLWLockTranche() and LWLockNewTrancheId().
+ * Test locks requested with RequestNamedLWLockTranche
*/
-PG_FUNCTION_INFO_V1(test_lwlock_tranches);
+PG_FUNCTION_INFO_V1(test_startup_lwlocks);
Datum
-test_lwlock_tranches(PG_FUNCTION_ARGS)
+test_startup_lwlocks(PG_FUNCTION_ARGS)
{
- int dynamic_tranches[NUM_DYNAMIC_TRANCHES];
-
- for (int i = 0; i < NUM_DYNAMIC_TRANCHES; i++)
- dynamic_tranches[i] = LWLockNewTrancheId(DYNAMIC_TRANCHE_NAME);
-
- for (int i = 0; i < NUM_STARTUP_TRANCHES; i++)
- {
- if (strcmp(GET_TRANCHE_NAME(LWTRANCHE_FIRST_USER_DEFINED + i),
- STARTUP_TRANCHE_NAME) != 0)
- elog(ERROR, "incorrect startup lock tranche name");
- }
-
- for (int i = 0; i < NUM_DYNAMIC_TRANCHES; i++)
- {
- if (strcmp(GET_TRANCHE_NAME(dynamic_tranches[i]),
- DYNAMIC_TRANCHE_NAME) != 0)
- elog(ERROR, "incorrect dynamic lock tranche name");
- }
+ LWLockPadded *lwlock_startup;
+ LWLockPadded *lwlock_startup10;
+
+ /* Check that the locks can be used */
+ lwlock_startup = GetNamedLWLockTranche("test_lwlock_tranches_startup");
+ lwlock_startup10 = GetNamedLWLockTranche("test_lwlock_tranches_startup10");
+
+ LWLockAcquire(&lwlock_startup->lock, LW_EXCLUSIVE);
+ for (int i = 0; i < 10; i++)
+ LWLockAcquire(&lwlock_startup10[i].lock, LW_EXCLUSIVE);
+
+ LWLockRelease(&lwlock_startup->lock);
+ for (int i = 0; i < 10; i++)
+ LWLockRelease(&lwlock_startup10[i].lock);
+
+ /*
+ * Check that GetLWLockIdentifier() returns the expected value for
+ * tranches
+ */
+ if (strcmp("test_lwlock_tranches_startup",
+ GetLWLockIdentifier(PG_WAIT_LWLOCK, LWTRANCHE_FIRST_USER_DEFINED)) != 0)
+ elog(ERROR, "incorrect startup lock tranche name");
+ if (strcmp("test_lwlock_tranches_startup10",
+ GetLWLockIdentifier(PG_WAIT_LWLOCK, LWTRANCHE_FIRST_USER_DEFINED + 1)) != 0)
+ elog(ERROR, "incorrect startup lock tranche name");
PG_RETURN_VOID();
}
@@ -82,15 +84,16 @@ test_lwlock_tranches(PG_FUNCTION_ARGS)
/*
* Wrapper for LWLockNewTrancheId().
*/
-PG_FUNCTION_INFO_V1(test_lwlock_tranche_creation);
+PG_FUNCTION_INFO_V1(test_lwlock_tranche_create);
Datum
-test_lwlock_tranche_creation(PG_FUNCTION_ARGS)
+test_lwlock_tranche_create(PG_FUNCTION_ARGS)
{
char *tranche_name = PG_ARGISNULL(0) ? NULL : TextDatumGetCString(PG_GETARG_DATUM(0));
+ int tranche_id;
- (void) LWLockNewTrancheId(tranche_name);
+ tranche_id = LWLockNewTrancheId(tranche_name);
- PG_RETURN_VOID();
+ PG_RETURN_INT32(tranche_id);
}
/*
@@ -107,6 +110,21 @@ test_lwlock_tranche_lookup(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+PG_FUNCTION_INFO_V1(test_lwlock_get_lwlock_identifier);
+Datum
+test_lwlock_get_lwlock_identifier(PG_FUNCTION_ARGS)
+{
+ int eventId = PG_GETARG_INT32(0);
+ const char *tranche_name;
+
+ tranche_name = GetLWLockIdentifier(PG_WAIT_LWLOCK, eventId);
+
+ if (tranche_name)
+ PG_RETURN_TEXT_P(cstring_to_text(tranche_name));
+ else
+ PG_RETURN_NULL();
+}
+
/*
* Wrapper for LWLockInitialize().
*/
--
2.47.3
From 70d389faeaebe40fee42d915d776925d7b5e5067 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Sun, 5 Apr 2026 17:28:08 +0300
Subject: [PATCH v1 2/2] Check that the tranche name is unique in
RequestNamedLWLockTranche
You could request two tranches with same name, but things would get
confusing when you called GetNamedLWLockTranche() to get the LWLocks
allocated for them; it would always return the first tranche with the
name. That doesn't make sense, so forbid duplicates.
We still allow duplicates with LWLockNewTrancheId(). That works better
as you don't use the name to look up the tranche ID later. It's still
confusing in wait events, for example, but it's not dangerous in the
same way.
---
src/backend/storage/lmgr/lwlock.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 5cb696490d6..98138cb09d1 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -649,6 +649,13 @@ RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks)
errdetail("No more than %d tranches may be registered.",
MAX_USER_DEFINED_TRANCHES)));
+ /* Check that the name isn't already in use */
+ foreach_ptr(NamedLWLockTrancheRequest, existing, NamedLWLockTrancheRequests)
+ {
+ if (strcmp(existing->tranche_name, tranche_name) == 0)
+ elog(ERROR, "requested tranche \"%s\" is already registered", tranche_name);
+ }
+
if (IsPostmasterEnvironment)
oldcontext = MemoryContextSwitchTo(PostmasterContext);
else
--
2.47.3
