From 3ddf87e520f74055a641c73da74baa4b05b1758f Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas 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=PgGSEydiW3A@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