From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Teach DSM registry to ERROR if attaching to an uninitialized ent
Date: Tue, 25 Nov 2025 11:45:34 -0600
Message-ID: <aSXrPqV3Bcm2HLV4@nathan> (raw)
In-Reply-To: <CA+TgmobGYDczmVa-UDGPgcyN=63PJzzyQ9ZCofou4VYH8TVLSQ@mail.gmail.com>
References: <aR-fqzfZYSd3TVKa@nathan>
<CA+TgmoZsxbVhoxNGTWSd6=vHvyoqOSDNXJZabBZx+St8uPduLQ@mail.gmail.com>
<aSIR4Z4U887f_KcQ@nathan>
<aSSlqWwLVuXMmN-o@nathan>
<aSSr6j3JzeLcqOWo@nathan>
<CA+TgmoYq7U0hHgOEOqQgf=6cFXz_7Ab8QTRMFqFpLNfav0G4Ug@mail.gmail.com>
<aSTNXyXnr9HYsLo6@nathan>
<CA+Tgmob6knHwEbQv11n_Yy0GyFzPgjhC33LroWso0prt7hCksg@mail.gmail.com>
<aSXQOAZuA4rX9ooU@nathan>
<CA+TgmobGYDczmVa-UDGPgcyN=63PJzzyQ9ZCofou4VYH8TVLSQ@mail.gmail.com>
On Tue, Nov 25, 2025 at 11:25:09AM -0500, Robert Haas wrote:
> The downside is that then we have to rely on PG_CATCH() to make things
> whole. I am not sure that there's any problem with that, but I'm also
> not sure that there isn't. The timing of PG_CATCH() block execution
> often creates bugs, because it runs before (sub)transaction abort.
> That means that there's a real risk that you try to acquire an LWLock
> you already hold, for example. It's a lot easier to be confident that
> cleanup actions will reliably succeed when they run inside the
> transaction abort path that knows the order in which various resources
> should be released. Generally, I think it's PG_CATCH() is fine if
> you're releasing a resource that is decoupled from everything else,
> like using a third-party library's free function to free memory
> allocated by that library. But if you're releasing PostgreSQL
> resources that are layered on top of other PostgreSQL resources, like
> a DSA that depends on DSM and LWLock, I think it's a lot more
> difficult to be certain that you aren't going to end up trying to
> release the same stuff multiple times or in the wrong order. I'm not
> saying you can't make it work, but I've banged my head on this
> particular doorframe enough times that my reflex is to duck.
I gave your idea a try, and I like the result a lot. IMHO it makes the
code much easier to reason about.
--
nathan
Attachments:download all (2 files, .tar.gz)v3-0001-Revert-Teach-DSM-registry-to-ERROR-if-attaching-t.patch (text/plain, 3.7K) download
From 3fddb959db906ff6560d73c08ba7695812020275 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 24 Nov 2025 11:03:52 -0600
Subject: [PATCH v3 1/2] Revert "Teach DSM registry to ERROR if attaching to an
uninitialized entry."
This reverts commit 1165a933aab1355757a43cfd9193b6cce06f573b.
---
src/backend/storage/ipc/dsm_registry.c | 34 +++-----------------------
1 file changed, 4 insertions(+), 30 deletions(-)
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.cindex 7eba8a8cffb..a926b9c3f32 100644--- a/src/backend/storage/ipc/dsm_registry.c+++ b/src/backend/storage/ipc/dsm_registry.c@@ -93,7 +93,6 @@ typedef struct DSMRegistryEntry
{
char name[NAMEDATALEN];
DSMREntryType type;
- bool initialized;
union
{
NamedDSMState dsm;
@@ -217,7 +216,6 @@ 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);
@@ -230,21 +228,13 @@ GetNamedDSMSegment(const char *name, size_t size,
if (init_callback)
(*init_callback) (ret);
-- entry->initialized = true;
}
else if (entry->type != DSMR_ENTRY_TYPE_DSM)
ereport(ERROR,
- (errmsg("requested DSM segment \"%s\" does not match type of existing entry",- name)));- else if (!entry->initialized)- ereport(ERROR,- (errmsg("requested DSM segment \"%s\" failed initialization",- name)));+ (errmsg("requested DSM segment does not match type of existing entry")));
else if (entry->dsm.size != size)
ereport(ERROR,
- (errmsg("requested DSM segment \"%s\" does not match size of existing entry",- name)));+ (errmsg("requested DSM segment size does not match size of existing segment")));
else
{
NamedDSMState *state = &entry->dsm;
@@ -307,7 +297,6 @@ 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);
@@ -319,17 +308,10 @@ 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->type != DSMR_ENTRY_TYPE_DSA)
ereport(ERROR,
- (errmsg("requested DSA \"%s\" does not match type of existing entry",- name)));- else if (!entry->initialized)- ereport(ERROR,- (errmsg("requested DSA \"%s\" failed initialization",- name)));+ (errmsg("requested DSA does not match type of existing entry")));
else
{
NamedDSAState *state = &entry->dsa;
@@ -390,7 +372,6 @@ 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);
@@ -408,17 +389,10 @@ 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->type != DSMR_ENTRY_TYPE_DSH)
ereport(ERROR,
- (errmsg("requested DSHash \"%s\" does not match type of existing entry",- name)));- else if (!entry->initialized)- ereport(ERROR,- (errmsg("requested DSHash \"%s\" failed initialization",- name)));+ (errmsg("requested DSHash does not match type of existing entry")));
else
{
NamedDSHState *dsh_state = &entry->dsh;
--
2.39.5 (Apple Git-154)
v3-0002-handle-ERRORs-in-DSM-registry-functions.patch (text/plain, 7.0K) download
From c40884c967755b3630d0419a7d6585d3f4aae170 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 24 Nov 2025 12:28:23 -0600
Subject: [PATCH v3 2/2] handle ERRORs in DSM registry functions
---
src/backend/storage/ipc/dsm_registry.c | 110 +++++++++++++++----------
1 file changed, 67 insertions(+), 43 deletions(-)
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.cindex a926b9c3f32..5290cc1c8e3 100644--- a/src/backend/storage/ipc/dsm_registry.c+++ b/src/backend/storage/ipc/dsm_registry.c@@ -155,9 +155,10 @@ init_dsm_registry(void)
{
/* Initialize dynamic shared hash table for registry. */
dsm_registry_dsa = dsa_create(LWTRANCHE_DSM_REGISTRY_DSA);
+ dsm_registry_table = dshash_create(dsm_registry_dsa, &dsh_params, NULL);+
dsa_pin(dsm_registry_dsa);
dsa_pin_mapping(dsm_registry_dsa);
- dsm_registry_table = dshash_create(dsm_registry_dsa, &dsh_params, NULL);
/* Store handles in shared memory for other backends to use. */
DSMRegistryCtx->dsah = dsa_get_handle(dsm_registry_dsa);
@@ -188,6 +189,8 @@ GetNamedDSMSegment(const char *name, size_t size,
DSMRegistryEntry *entry;
MemoryContext oldcontext;
void *ret;
+ NamedDSMState *state;+ dsm_segment *seg;
Assert(found);
@@ -210,36 +213,36 @@ GetNamedDSMSegment(const char *name, size_t size,
init_dsm_registry();
entry = dshash_find_or_insert(dsm_registry_table, name, found);
+ state = &entry->dsm;
if (!(*found))
{
- NamedDSMState *state = &entry->dsm;- dsm_segment *seg;-
entry->type = DSMR_ENTRY_TYPE_DSM;
+ state->handle = DSM_HANDLE_INVALID;+ state->size = size;+ }+ else if (entry->type != DSMR_ENTRY_TYPE_DSM)+ ereport(ERROR,+ (errmsg("requested DSM segment does not match type of existing entry")));+ else if (state->size != size)+ ereport(ERROR,+ (errmsg("requested DSM segment size does not match size of existing segment")));++ if (state->handle == DSM_HANDLE_INVALID)+ {+ *found = false;
/* Initialize the segment. */
seg = dsm_create(size, 0);
+ if (init_callback)+ (*init_callback) (dsm_segment_address(seg));+
dsm_pin_segment(seg);
dsm_pin_mapping(seg);
state->handle = dsm_segment_handle(seg);
- state->size = size;- ret = dsm_segment_address(seg);-- if (init_callback)- (*init_callback) (ret);
}
- else if (entry->type != DSMR_ENTRY_TYPE_DSM)- ereport(ERROR,- (errmsg("requested DSM segment does not match type of existing entry")));- else if (entry->dsm.size != size)- ereport(ERROR,- (errmsg("requested DSM segment size does not match size of existing segment")));
else
{
- NamedDSMState *state = &entry->dsm;- dsm_segment *seg;-
/* If the existing segment is not already attached, attach it now. */
seg = dsm_find_mapping(state->handle);
if (seg == NULL)
@@ -250,10 +253,9 @@ GetNamedDSMSegment(const char *name, size_t size,
dsm_pin_mapping(seg);
}
-- ret = dsm_segment_address(seg);
}
+ ret = dsm_segment_address(seg);
dshash_release_lock(dsm_registry_table, entry);
MemoryContextSwitchTo(oldcontext);
@@ -274,6 +276,7 @@ GetNamedDSA(const char *name, bool *found)
DSMRegistryEntry *entry;
MemoryContext oldcontext;
dsa_area *ret;
+ NamedDSAState *state;
Assert(found);
@@ -292,14 +295,28 @@ GetNamedDSA(const char *name, bool *found)
init_dsm_registry();
entry = dshash_find_or_insert(dsm_registry_table, name, found);
+ state = &entry->dsa;
if (!(*found))
{
- NamedDSAState *state = &entry->dsa;-
entry->type = DSMR_ENTRY_TYPE_DSA;
+ state->handle = DSA_HANDLE_INVALID;+ state->tranche = -1;+ }+ else if (entry->type != DSMR_ENTRY_TYPE_DSA)+ ereport(ERROR,+ (errmsg("requested DSA does not match type of existing entry")));++ if (state->tranche == -1)+ {+ *found = false;
/* Initialize the LWLock tranche for the DSA. */
state->tranche = LWLockNewTrancheId(name);
+ }++ if (state->handle == DSA_HANDLE_INVALID)+ {+ *found = false;
/* Initialize the DSA. */
ret = dsa_create(state->tranche);
@@ -309,17 +326,11 @@ GetNamedDSA(const char *name, bool *found)
/* Store handle for other backends to use. */
state->handle = dsa_get_handle(ret);
}
- else if (entry->type != DSMR_ENTRY_TYPE_DSA)+ else if (dsa_is_attached(state->handle))
ereport(ERROR,
- (errmsg("requested DSA does not match type of existing entry")));+ (errmsg("requested DSA already attached to current process")));
else
{
- NamedDSAState *state = &entry->dsa;-- if (dsa_is_attached(state->handle))- ereport(ERROR,- (errmsg("requested DSA already attached to current process")));-
/* Attach to existing DSA. */
ret = dsa_attach(state->handle);
dsa_pin_mapping(ret);
@@ -346,6 +357,7 @@ GetNamedDSHash(const char *name, const dshash_parameters *params, bool *found)
DSMRegistryEntry *entry;
MemoryContext oldcontext;
dshash_table *ret;
+ NamedDSHState *dsh_state;
Assert(params);
Assert(found);
@@ -365,45 +377,57 @@ GetNamedDSHash(const char *name, const dshash_parameters *params, bool *found)
init_dsm_registry();
entry = dshash_find_or_insert(dsm_registry_table, name, found);
+ dsh_state = &entry->dsh;
if (!(*found))
{
- NamedDSHState *dsh_state = &entry->dsh;- dshash_parameters params_copy;- dsa_area *dsa;-
entry->type = DSMR_ENTRY_TYPE_DSH;
+ dsh_state->dsa_handle = DSA_HANDLE_INVALID;+ dsh_state->dsh_handle = DSHASH_HANDLE_INVALID;+ dsh_state->tranche = -1;+ }+ else if (entry->type != DSMR_ENTRY_TYPE_DSH)+ ereport(ERROR,+ (errmsg("requested DSHash does not match type of existing entry")));++ if (dsh_state->tranche == -1)+ {+ *found = false;
/* Initialize the LWLock tranche for the hash table. */
dsh_state->tranche = LWLockNewTrancheId(name);
+ }++ if (dsh_state->dsa_handle == DSA_HANDLE_INVALID)+ {+ dshash_parameters params_copy;+ dsa_area *dsa;++ *found = false;
/* Initialize the DSA for the hash table. */
dsa = dsa_create(dsh_state->tranche);
- dsa_pin(dsa);- dsa_pin_mapping(dsa);
/* Initialize the dshash table. */
memcpy(¶ms_copy, params, sizeof(dshash_parameters));
params_copy.tranche_id = dsh_state->tranche;
ret = dshash_create(dsa, ¶ms_copy, NULL);
+ dsa_pin(dsa);+ dsa_pin_mapping(dsa);+
/* 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);
}
- else if (entry->type != DSMR_ENTRY_TYPE_DSH)+ else if (dsa_is_attached(dsh_state->dsa_handle))
ereport(ERROR,
- (errmsg("requested DSHash does not match type of existing entry")));+ (errmsg("requested DSHash already attached to current process")));
else
{
- NamedDSHState *dsh_state = &entry->dsh;
dsa_area *dsa;
/* XXX: Should we verify params matches what table was created with? */
- if (dsa_is_attached(dsh_state->dsa_handle))- ereport(ERROR,- (errmsg("requested DSHash already attached to current process")));-
/* Attach to existing DSA for the hash table. */
dsa = dsa_attach(dsh_state->dsa_handle);
dsa_pin_mapping(dsa);
--
2.39.5 (Apple Git-154)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, robertmhaas@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Teach DSM registry to ERROR if attaching to an uninitialized ent
In-Reply-To: <aSXrPqV3Bcm2HLV4@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox