Date: Mon, 10 Nov 2025 12:05:14 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	Rahila Syed <rahilasyed90@gmail.com>,
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve LWLock tranche name visibility across backends
Message-ID: <aRIpWji1L87hhBVe@nathan>
References: <aLiQeizOQKbKF-ae@nathan>
 <aLmzwC2dRbqk14y6@nathan>
 <CAA5RZ0t6s2Z38oRq883Ocq7J4xjn1FJF17Q4YR6++vrw5KYCtQ@mail.gmail.com>
 <aLn4n-5UFzKHFdR5@nathan>
 <dd36d384-55df-4fc2-825c-5bc56c950fa9@gmail.com>
 <CAA5RZ0u8+sC_FBwoaUeLhTi5bmjba9Yk3N-TbdsTm7c1no0zcg@mail.gmail.com>
 <CAA5RZ0v4kwF06o4a+Xybd++ipC7TYkn879Wz_2BZK412+ULXgg@mail.gmail.com>
 <CAA5RZ0va93+wk3_6UYi52=b=xqbKtXvaM-Cj5moTir7nM-LA=Q@mail.gmail.com>
 <aRISKd2MEeIqRcOh@nathan>
 <aRIXtvkYn1mmyNG5@nathan>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="DWDGkHIbZGie56xR"
Content-Disposition: inline
In-Reply-To: <aRIXtvkYn1mmyNG5@nathan>
Archived-At: <https://www.postgresql.org/message-id/aRIpWji1L87hhBVe%40nathan>
Precedence: bulk


--DWDGkHIbZGie56xR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Nov 10, 2025 at 10:49:58AM -0600, Nathan Bossart wrote:
> On Mon, Nov 10, 2025 at 10:26:17AM -0600, Nathan Bossart wrote:
>> It's probably a good idea to avoid tranche leaks, but IMHO there's room for
>> improvement in the DSM registry, too.  IIUC the problem is that the DSM
>> segment is still being added to the registry and found by other backends
>> despite the initialization callback failing.  My first instinct is that we
>> should keep track of whether the DSM segments/DSAs/dshash tables in the
>> registry have been fully initialized and to just ERROR in other backends
>> when attaching if they aren't.  That shouldn't really happen in practice,
>> but it'd be good to avoid the strange errors, anyway.

0001 does this.  When applied, Alexander's reproduction steps fail with

	ERROR:  requested DSM segment failed initialization

This one should probably get back-patched to v17, where the DSM registry
was introduced.

> BTW if we really wanted to avoid leaking tranches in test_dsa, we'd need to
> store the ID in shared memory.  Your patch helps in the case where a single
> backend is repeatedly calling test_dsa_resowners(), but other backends
> would still allocate their own tranche.  I don't see a strong need to fix
> this on back-branches, given these functions run exactly once as part of a
> test, but fixing it on master seems worthwhile so that extension authors
> don't copy/paste this broken code.

0002 does this.

-- 
nathan

--DWDGkHIbZGie56xR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v2-0001-DSM-registry-ERROR-if-entry-was-not-initialized.patch

From 0b4ccd2fd0393ed43edf39f018b2dabd9ca2b001 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 10 Nov 2025 11:43:51 -0600
Subject: [PATCH v2 1/2] DSM registry: ERROR if entry was not initialized.

---
 src/backend/storage/ipc/dsm_registry.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index a926b9c3f32..ec8b48ad9a0 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -93,6 +93,7 @@ typedef struct DSMRegistryEntry
 {
 	char		name[NAMEDATALEN];
 	DSMREntryType type;
+	bool		initialized;
 	union
 	{
 		NamedDSMState dsm;
@@ -216,6 +217,7 @@ GetNamedDSMSegment(const char *name, size_t size,
 		dsm_segment *seg;
 
 		entry->type = DSMR_ENTRY_TYPE_DSM;
+		entry->initialized = false;
 
 		/* Initialize the segment. */
 		seg = dsm_create(size, 0);
@@ -228,7 +230,12 @@ GetNamedDSMSegment(const char *name, size_t size,
 
 		if (init_callback)
 			(*init_callback) (ret);
+
+		entry->initialized = true;
 	}
+	else if (!entry->initialized)
+		ereport(ERROR,
+				(errmsg("requested DSM segment failed initialization")));
 	else if (entry->type != DSMR_ENTRY_TYPE_DSM)
 		ereport(ERROR,
 				(errmsg("requested DSM segment does not match type of existing entry")));
@@ -297,6 +304,7 @@ GetNamedDSA(const char *name, bool *found)
 		NamedDSAState *state = &entry->dsa;
 
 		entry->type = DSMR_ENTRY_TYPE_DSA;
+		entry->initialized = false;
 
 		/* Initialize the LWLock tranche for the DSA. */
 		state->tranche = LWLockNewTrancheId(name);
@@ -308,7 +316,12 @@ GetNamedDSA(const char *name, bool *found)
 
 		/* Store handle for other backends to use. */
 		state->handle = dsa_get_handle(ret);
+
+		entry->initialized = true;
 	}
+	else if (!entry->initialized)
+		ereport(ERROR,
+				(errmsg("requested DSA failed initialization")));
 	else if (entry->type != DSMR_ENTRY_TYPE_DSA)
 		ereport(ERROR,
 				(errmsg("requested DSA does not match type of existing entry")));
@@ -372,6 +385,7 @@ GetNamedDSHash(const char *name, const dshash_parameters *params, bool *found)
 		dsa_area   *dsa;
 
 		entry->type = DSMR_ENTRY_TYPE_DSH;
+		entry->initialized = false;
 
 		/* Initialize the LWLock tranche for the hash table. */
 		dsh_state->tranche = LWLockNewTrancheId(name);
@@ -389,7 +403,12 @@ GetNamedDSHash(const char *name, const dshash_parameters *params, bool *found)
 		/* Store handles for other backends to use. */
 		dsh_state->dsa_handle = dsa_get_handle(dsa);
 		dsh_state->dsh_handle = dshash_get_hash_table_handle(ret);
+
+		entry->initialized = true;
 	}
+	else if (!entry->initialized)
+		ereport(ERROR,
+				(errmsg("requested DSHash failed initialization")));
 	else if (entry->type != DSMR_ENTRY_TYPE_DSH)
 		ereport(ERROR,
 				(errmsg("requested DSHash does not match type of existing entry")));
-- 
2.39.5 (Apple Git-154)


--DWDGkHIbZGie56xR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v2-0002-test_dsa-Avoid-leaking-LWLock-tranches.patch

From 37ef7e1fd21cc1aa93cdc1af4fd84a26516faaa3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 10 Nov 2025 12:03:47 -0600
Subject: [PATCH v2 2/2] test_dsa: Avoid leaking LWLock tranches.

---
 src/test/modules/test_dsa/test_dsa.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/src/test/modules/test_dsa/test_dsa.c b/src/test/modules/test_dsa/test_dsa.c
index 01d5c6fa67f..21e4ce6a745 100644
--- a/src/test/modules/test_dsa/test_dsa.c
+++ b/src/test/modules/test_dsa/test_dsa.c
@@ -13,25 +13,35 @@
 #include "postgres.h"
 
 #include "fmgr.h"
+#include "storage/dsm_registry.h"
 #include "storage/lwlock.h"
 #include "utils/dsa.h"
 #include "utils/resowner.h"
 
 PG_MODULE_MAGIC;
 
+static void
+init_tranche(void *ptr)
+{
+	int		   *tranche_id = (int *) ptr;
+
+	*tranche_id = LWLockNewTrancheId("test_dsa");
+}
+
 /* Test basic DSA functionality */
 PG_FUNCTION_INFO_V1(test_dsa_basic);
 Datum
 test_dsa_basic(PG_FUNCTION_ARGS)
 {
-	int			tranche_id;
+	int		   *tranche_id;
+	bool		found;
 	dsa_area   *a;
 	dsa_pointer p[100];
 
-	/* XXX: this tranche is leaked */
-	tranche_id = LWLockNewTrancheId("test_dsa");
+	tranche_id = GetNamedDSMSegment("test_dsa", sizeof(int),
+									init_tranche, &found);
 
-	a = dsa_create(tranche_id);
+	a = dsa_create(*tranche_id);
 	for (int i = 0; i < 100; i++)
 	{
 		p[i] = dsa_allocate(a, 1000);
@@ -62,17 +72,18 @@ PG_FUNCTION_INFO_V1(test_dsa_resowners);
 Datum
 test_dsa_resowners(PG_FUNCTION_ARGS)
 {
-	int			tranche_id;
+	int		   *tranche_id;
+	bool		found;
 	dsa_area   *a;
 	dsa_pointer p[10000];
 	ResourceOwner oldowner;
 	ResourceOwner childowner;
 
-	/* XXX: this tranche is leaked */
-	tranche_id = LWLockNewTrancheId("test_dsa");
+	tranche_id = GetNamedDSMSegment("test_dsa", sizeof(int),
+									init_tranche, &found);
 
 	/* Create DSA in parent resource owner */
-	a = dsa_create(tranche_id);
+	a = dsa_create(*tranche_id);
 
 	/*
 	 * Switch to child resource owner, and do a bunch of allocations in the
-- 
2.39.5 (Apple Git-154)


--DWDGkHIbZGie56xR--