dsm_registry: Add detach and destroy features
Hi,
I had use cases for manually detaching and destroying dsm segment while
developing my extension (emulating Oracle's DBMS_PIPE package). In this
extension user can call user defined procedures that destroy the resources
in the shared memory. There were mentions about adding detach features in
the original discussion (introduce dynamic shared memory registry
</messages/by-id/20231205034647.GA2705267@nathanxps13>),
but it seems like it didn't expand from there.
In the attached patch, dsm_registry not provides two more interfaces:
DetachNamedDSMSegment, and DestroyNamedDSMSegment. Detach function simply
detaches the corresponding dsm_segment. Destroy function calls
dsm_unpin_segment and deletes the dshash entry from the dsm_registry_table.
Destroy function may not "destroy" the dsm segment immediately if there are
other processes attached to the dsm segment, but it will eventually cause
the dsm segment to be destroyed when there are no more processes attached
to it. Because Destroy function deletes the entry from the
dsm_registry_table, it will block new attachment. It will create a new dsm
segment with the same name.
Detach and Destroy functions allows on_detach_callback, which will be
passed on to the dsm segment by calling on_dsm_detach. Because
on_dsm_detach requires the callback function to have dsm_segment as input,
we wrap the library user callback with a function matching to the required
signature.
I also made some fix in GetNamedDSMSegment function where it throws an
exception if the found entry's size field does not match the user input. It
looks like dshash_release_lock needs to be called and MemoryContext needs
to be switched back to the old one.
It's my first time submitting a patch so please let me know if I missed on
something.
Best regards,
Sungwoo Chang
Attachments:
v1-0001-add-detach-and-destroy-for-dsm-registry.patchtext/x-patch; charset=US-ASCII; name=v1-0001-add-detach-and-destroy-for-dsm-registry.patchDownload
From 06ddb42e39b3bd0e411a9bc96467ca6797673b8a Mon Sep 17 00:00:00 2001
From: swchangdev <swchangdev@gmail.com>
Date: Fri, 14 Mar 2025 10:00:12 +0900
Subject: [PATCH] Add detach and destroy feature to dsm_registry
---
src/backend/storage/ipc/dsm_registry.c | 167 +++++++++++++++++-
src/include/storage/dsm_registry.h | 8 +
.../expected/test_dsm_registry.out | 11 ++
.../sql/test_dsm_registry.sql | 2 +
.../test_dsm_registry--1.0.sql | 6 +
.../test_dsm_registry/test_dsm_registry.c | 28 +++
6 files changed, 221 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index 1d4fd31ffed..7915b70e47d 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -47,6 +47,12 @@ typedef struct DSMRegistryEntry
size_t size;
} DSMRegistryEntry;
+typedef struct DSMDetachCallbackContext
+{
+ void (*on_detach_callback) (void *);
+ void *arg;
+} DSMDetachCallbackContext;
+
static const dshash_parameters dsh_params = {
offsetof(DSMRegistryEntry, handle),
sizeof(DSMRegistryEntry),
@@ -121,6 +127,36 @@ init_dsm_registry(void)
LWLockRelease(DSMRegistryLock);
}
+static void
+call_on_detach_callback(dsm_segment *seg, Datum arg)
+{
+ char *ptr = DatumGetPointer(arg);
+ DSMDetachCallbackContext *cb_ctx = (DSMDetachCallbackContext *)ptr;
+ cb_ctx->on_detach_callback(cb_ctx->arg);
+}
+
+static void
+detach_dsm_segment(dsm_handle handle, void (*detach_cb) (void *), void *arg)
+{
+ dsm_segment *seg = dsm_find_mapping(handle);
+
+ /* Detach the segment only if it is already attached */
+ if (seg != NULL)
+ {
+ if (detach_cb != NULL)
+ {
+ DSMDetachCallbackContext *cb_ctx = palloc(sizeof(DSMDetachCallbackContext));
+ cb_ctx->on_detach_callback = detach_cb;
+ cb_ctx->arg = arg;
+
+ on_dsm_detach(seg, call_on_detach_callback, PointerGetDatum(cb_ctx));
+ }
+
+ dsm_unpin_mapping(seg);
+ dsm_detach(seg);
+ }
+}
+
/*
* Initialize or attach a named DSM segment.
*
@@ -172,6 +208,8 @@ GetNamedDSMSegment(const char *name, size_t size,
}
else if (entry->size != size)
{
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
ereport(ERROR,
(errmsg("requested DSM segment size does not match size of "
"existing segment")));
@@ -185,8 +223,11 @@ GetNamedDSMSegment(const char *name, size_t size,
{
seg = dsm_attach(entry->handle);
if (seg == NULL)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
elog(ERROR, "could not map dynamic shared memory segment");
-
+ }
dsm_pin_mapping(seg);
}
@@ -198,3 +239,127 @@ GetNamedDSMSegment(const char *name, size_t size,
return ret;
}
+
+/*
+ * Detach a named DSM segment
+ *
+ * This routine detaches the DSM segment. If the DSM segment was not attached
+ * by this process, then the routine just returns. on_detach_callback is passed
+ * on to dsm_segment by calling on_dsm_detach for the corresponding dsm_segment
+ * before actually detaching.
+ */
+void
+DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot detach a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+}
+
+/*
+ * Attempt to destroy a named DSM segment
+ *
+ * This routine attempts to destroy the DSM segment. We unpin the dsm_segment
+ * and delete the entry from dsm_registry_table. This may not destroy the
+ * dsm_segment instantly, but it would die out once all the other processes
+ * attached to this dsm_segment either exit or manually detach from the
+ * dsm_segment.
+ *
+ * Because we deleted the key from dsm_registry_table, calling
+ * GetNamedDSMSegment with the same key would result into creating a new
+ * dsm_segment instead of retrieving the old unpinned dsm_segment.
+ */
+void
+DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot destroy a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+ dsm_unpin_segment(entry->handle);
+
+ /* dshash_delete_entry calls LWLockRelease internally. We shouldn't
+ * release lock twice */
+ dshash_delete_entry(dsm_registry_table, entry);
+ dshash_delete_key(dsm_registry_table, name);
+
+ MemoryContextSwitchTo(oldcontext);
+}
\ No newline at end of file
diff --git a/src/include/storage/dsm_registry.h b/src/include/storage/dsm_registry.h
index b381e44bc9d..2bd20b82ffd 100644
--- a/src/include/storage/dsm_registry.h
+++ b/src/include/storage/dsm_registry.h
@@ -17,6 +17,14 @@ extern void *GetNamedDSMSegment(const char *name, size_t size,
void (*init_callback) (void *ptr),
bool *found);
+extern void DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
+extern void DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
extern Size DSMRegistryShmemSize(void);
extern void DSMRegistryShmemInit(void);
diff --git a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
index 8ffbd343a05..f5c85a976a9 100644
--- a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
+++ b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
@@ -12,3 +12,14 @@ SELECT get_val_in_shmem();
1236
(1 row)
+SELECT detach_from_tdr_segment();
+ detach_from_tdr_segment
+-------------------------
+ t
+(1 row)
+
+SELECT destroy_tdr_segment();
+ destroy_tdr_segment
+---------------------
+
+(1 row)
diff --git a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
index b3351be0a16..24f90658f83 100644
--- a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
+++ b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
@@ -2,3 +2,5 @@ CREATE EXTENSION test_dsm_registry;
SELECT set_val_in_shmem(1236);
\c
SELECT get_val_in_shmem();
+SELECT detach_from_tdr_segment();
+SELECT destroy_tdr_segment();
\ No newline at end of file
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
index 8c55b0919b1..74d8dc50d8a 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -8,3 +8,9 @@ CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
CREATE FUNCTION get_val_in_shmem() RETURNS INT
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION detach_from_tdr_segment() RETURNS BOOL
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION destroy_tdr_segment() RETURNS VOID
+ AS 'MODULE_PATHNAME' LANGUAGE C;
\ No newline at end of file
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 462a80f8790..bdbb45e745c 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry.c
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -35,6 +35,12 @@ tdr_init_shmem(void *ptr)
state->val = 0;
}
+static void
+reset_tdr_state_to_null(void *arg)
+{
+ tdr_state = NULL;
+}
+
static void
tdr_attach_shmem(void)
{
@@ -74,3 +80,25 @@ get_val_in_shmem(PG_FUNCTION_ARGS)
PG_RETURN_UINT32(ret);
}
+
+PG_FUNCTION_INFO_V1(detach_from_tdr_segment);
+Datum
+detach_from_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DetachNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ reset_tdr_state_to_null, NULL);
+
+ PG_RETURN_BOOL(tdr_state == NULL);
+}
+
+PG_FUNCTION_INFO_V2(destroy_tdr_segment);
+Datum
+destroy_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DestroyNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ NULL, NULL);
+
+ PG_RETURN_VOID();
+}
\ No newline at end of file
--
2.48.1
On Fri, Mar 14, 2025 at 10:46:58AM +0900, Sungwoo Chang wrote:
I also made some fix in GetNamedDSMSegment function where it throws an
exception if the found entry's size field does not match the user input. It
looks like dshash_release_lock needs to be called and MemoryContext needs
to be switched back to the old one.
My expectation is that the error handling takes care of these things. Are
there cases where it does not? In any case, I would expect these errors to
be extremely rare to the point of virtually never happening in practice.
It's my first time submitting a patch so please let me know if I missed on
something.
I'm delighted that folks want to expand on this feature, and I'm very
interested in helping. Other things folks have asked for are a view into
the DSM registry [0]/messages/by-id/4D445D3E-81C5-4135-95BB-D414204A0AB4@gmail.com and a simple way to allocate a DSA or dshash table
with the DSM registry, all of which seem like good improvements to me.
That being said, we are at the very end of the v18 development cycle, and
most folks are probably busy trying to finish their ongoing projects. So
it may be a while before anyone can give this a meaningful review. Please
be sure to add a commitfest entry [1]https://commitfest.postgresql.org/ so it doesn't get lost.
[0]: /messages/by-id/4D445D3E-81C5-4135-95BB-D414204A0AB4@gmail.com
[1]: https://commitfest.postgresql.org/
--
nathan
2025년 3월 14일 (금) 오후 11:36, Nathan Bossart <nathandbossart@gmail.com>님이 작성:
My expectation is that the error handling takes care of these things. Are
there cases where it does not? In any case, I would expect these errors to
be extremely rare to the point of virtually never happening in practice.
I don't see any logic in ereport where it switches back the original
memory context.
I guess not switching back to the original memory context is not so
problematic because
we set it to TopMemoryContext and throw will cause the user to restart a query.
Not releasing the dshash lock could be problematic, so it's better to keep that.
You are right about these errors to be rare, especially the one where
we can't find a
dsm_segment given the dsm handle. However, user faults can cause the
other error,
where the input name and size for dshash lookup don't match.
I'm delighted that folks want to expand on this feature, and I'm very
interested in helping. Other things folks have asked for are a view into
the DSM registry [0] and a simple way to allocate a DSA or dshash table
with the DSM registry, all of which seem like good improvements to me.
I didn't know about the discussions on the view for DSM registry. I'm
happy to work on that.
I think what I can also add along with the view is the user command
for detaching/destroying
the dsm segment (like pg_backend_terminate function).
That being said, we are at the very end of the v18 development cycle, and
most folks are probably busy trying to finish their ongoing projects. So
it may be a while before anyone can give this a meaningful review. Please
be sure to add a commitfest entry [1] so it doesn't get lost.[0] /messages/by-id/4D445D3E-81C5-4135-95BB-D414204A0AB4@gmail.com
[1] https://commitfest.postgresql.org/
Thanks for the heads up. I will add an entry to the commitfest as you advised.
Best regards,
Sungwoo Chang
There was a minor typo in the test module. I built and ran the
test_dsm_registry extension before submitting the patch but perhaps it
got mixed up with an older version.
Attachments:
v2-0001-add-detach-and-destroy-for-dsm-registry.patchtext/x-patch; charset=US-ASCII; name=v2-0001-add-detach-and-destroy-for-dsm-registry.patchDownload
From 06ddb42e39b3bd0e411a9bc96467ca6797673b8a Mon Sep 17 00:00:00 2001
From: swchangdev <swchangdev@gmail.com>
Date: Fri, 14 Mar 2025 10:00:12 +0900
Subject: [PATCH] Add detach and destroy feature to dsm_registry
---
src/backend/storage/ipc/dsm_registry.c | 167 +++++++++++++++++-
src/include/storage/dsm_registry.h | 8 +
.../expected/test_dsm_registry.out | 11 ++
.../sql/test_dsm_registry.sql | 2 +
.../test_dsm_registry--1.0.sql | 6 +
.../test_dsm_registry/test_dsm_registry.c | 28 +++
6 files changed, 221 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index 1d4fd31ffed..7915b70e47d 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -47,6 +47,12 @@ typedef struct DSMRegistryEntry
size_t size;
} DSMRegistryEntry;
+typedef struct DSMDetachCallbackContext
+{
+ void (*on_detach_callback) (void *);
+ void *arg;
+} DSMDetachCallbackContext;
+
static const dshash_parameters dsh_params = {
offsetof(DSMRegistryEntry, handle),
sizeof(DSMRegistryEntry),
@@ -121,6 +127,36 @@ init_dsm_registry(void)
LWLockRelease(DSMRegistryLock);
}
+static void
+call_on_detach_callback(dsm_segment *seg, Datum arg)
+{
+ char *ptr = DatumGetPointer(arg);
+ DSMDetachCallbackContext *cb_ctx = (DSMDetachCallbackContext *)ptr;
+ cb_ctx->on_detach_callback(cb_ctx->arg);
+}
+
+static void
+detach_dsm_segment(dsm_handle handle, void (*detach_cb) (void *), void *arg)
+{
+ dsm_segment *seg = dsm_find_mapping(handle);
+
+ /* Detach the segment only if it is already attached */
+ if (seg != NULL)
+ {
+ if (detach_cb != NULL)
+ {
+ DSMDetachCallbackContext *cb_ctx = palloc(sizeof(DSMDetachCallbackContext));
+ cb_ctx->on_detach_callback = detach_cb;
+ cb_ctx->arg = arg;
+
+ on_dsm_detach(seg, call_on_detach_callback, PointerGetDatum(cb_ctx));
+ }
+
+ dsm_unpin_mapping(seg);
+ dsm_detach(seg);
+ }
+}
+
/*
* Initialize or attach a named DSM segment.
*
@@ -172,6 +208,8 @@ GetNamedDSMSegment(const char *name, size_t size,
}
else if (entry->size != size)
{
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
ereport(ERROR,
(errmsg("requested DSM segment size does not match size of "
"existing segment")));
@@ -185,8 +223,11 @@ GetNamedDSMSegment(const char *name, size_t size,
{
seg = dsm_attach(entry->handle);
if (seg == NULL)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
elog(ERROR, "could not map dynamic shared memory segment");
-
+ }
dsm_pin_mapping(seg);
}
@@ -198,3 +239,127 @@ GetNamedDSMSegment(const char *name, size_t size,
return ret;
}
+
+/*
+ * Detach a named DSM segment
+ *
+ * This routine detaches the DSM segment. If the DSM segment was not attached
+ * by this process, then the routine just returns. on_detach_callback is passed
+ * on to dsm_segment by calling on_dsm_detach for the corresponding dsm_segment
+ * before actually detaching.
+ */
+void
+DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot detach a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+}
+
+/*
+ * Attempt to destroy a named DSM segment
+ *
+ * This routine attempts to destroy the DSM segment. We unpin the dsm_segment
+ * and delete the entry from dsm_registry_table. This may not destroy the
+ * dsm_segment instantly, but it would die out once all the other processes
+ * attached to this dsm_segment either exit or manually detach from the
+ * dsm_segment.
+ *
+ * Because we deleted the key from dsm_registry_table, calling
+ * GetNamedDSMSegment with the same key would result into creating a new
+ * dsm_segment instead of retrieving the old unpinned dsm_segment.
+ */
+void
+DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot destroy a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+ dsm_unpin_segment(entry->handle);
+
+ /* dshash_delete_entry calls LWLockRelease internally. We shouldn't
+ * release lock twice */
+ dshash_delete_entry(dsm_registry_table, entry);
+ dshash_delete_key(dsm_registry_table, name);
+
+ MemoryContextSwitchTo(oldcontext);
+}
\ No newline at end of file
diff --git a/src/include/storage/dsm_registry.h b/src/include/storage/dsm_registry.h
index b381e44bc9d..2bd20b82ffd 100644
--- a/src/include/storage/dsm_registry.h
+++ b/src/include/storage/dsm_registry.h
@@ -17,6 +17,14 @@ extern void *GetNamedDSMSegment(const char *name, size_t size,
void (*init_callback) (void *ptr),
bool *found);
+extern void DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
+extern void DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
extern Size DSMRegistryShmemSize(void);
extern void DSMRegistryShmemInit(void);
diff --git a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
index 8ffbd343a05..f5c85a976a9 100644
--- a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
+++ b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
@@ -12,3 +12,14 @@ SELECT get_val_in_shmem();
1236
(1 row)
+SELECT detach_from_tdr_segment();
+ detach_from_tdr_segment
+-------------------------
+ t
+(1 row)
+
+SELECT destroy_tdr_segment();
+ destroy_tdr_segment
+---------------------
+
+(1 row)
diff --git a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
index b3351be0a16..24f90658f83 100644
--- a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
+++ b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
@@ -2,3 +2,5 @@ CREATE EXTENSION test_dsm_registry;
SELECT set_val_in_shmem(1236);
\c
SELECT get_val_in_shmem();
+SELECT detach_from_tdr_segment();
+SELECT destroy_tdr_segment();
\ No newline at end of file
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
index 8c55b0919b1..74d8dc50d8a 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -8,3 +8,9 @@ CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
CREATE FUNCTION get_val_in_shmem() RETURNS INT
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION detach_from_tdr_segment() RETURNS BOOL
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION destroy_tdr_segment() RETURNS VOID
+ AS 'MODULE_PATHNAME' LANGUAGE C;
\ No newline at end of file
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 462a80f8790..bdbb45e745c 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry.c
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -35,6 +35,12 @@ tdr_init_shmem(void *ptr)
state->val = 0;
}
+static void
+reset_tdr_state_to_null(void *arg)
+{
+ tdr_state = NULL;
+}
+
static void
tdr_attach_shmem(void)
{
@@ -74,3 +80,25 @@ get_val_in_shmem(PG_FUNCTION_ARGS)
PG_RETURN_UINT32(ret);
}
+
+PG_FUNCTION_INFO_V1(detach_from_tdr_segment);
+Datum
+detach_from_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DetachNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ reset_tdr_state_to_null, NULL);
+
+ PG_RETURN_BOOL(tdr_state == NULL);
+}
+
+PG_FUNCTION_INFO_V1(destroy_tdr_segment);
+Datum
+destroy_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DestroyNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ NULL, NULL);
+
+ PG_RETURN_VOID();
+}
\ No newline at end of file
--
2.48.1
Regression test failed because I didn't add an extra new line in the
expected file.
2025년 3월 17일 (월) 오전 9:55, Sungwoo Chang <swchangdev@gmail.com>님이 작성:
Show quoted text
There was a minor typo in the test module. I built and ran the
test_dsm_registry extension before submitting the patch but perhaps it
got mixed up with an older version.
Attachments:
v3-0001-add-detach-and-destroy-for-dsm-registry.patchtext/x-patch; charset=US-ASCII; name=v3-0001-add-detach-and-destroy-for-dsm-registry.patchDownload
From ef58c9f26a73f8b3714ad02053749d2799b9fd4f Mon Sep 17 00:00:00 2001
From: swchangdev <swchangdev@gmail.com>
Date: Fri, 14 Mar 2025 10:00:12 +0900
Subject: [PATCH] Add detach and destroy feature to dsm_registry
---
src/backend/storage/ipc/dsm_registry.c | 167 +++++++++++++++++-
src/include/storage/dsm_registry.h | 8 +
.../expected/test_dsm_registry.out | 12 ++
.../sql/test_dsm_registry.sql | 2 +
.../test_dsm_registry--1.0.sql | 6 +
.../test_dsm_registry/test_dsm_registry.c | 28 +++
6 files changed, 222 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index 1d4fd31ffed..7915b70e47d 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -47,6 +47,12 @@ typedef struct DSMRegistryEntry
size_t size;
} DSMRegistryEntry;
+typedef struct DSMDetachCallbackContext
+{
+ void (*on_detach_callback) (void *);
+ void *arg;
+} DSMDetachCallbackContext;
+
static const dshash_parameters dsh_params = {
offsetof(DSMRegistryEntry, handle),
sizeof(DSMRegistryEntry),
@@ -121,6 +127,36 @@ init_dsm_registry(void)
LWLockRelease(DSMRegistryLock);
}
+static void
+call_on_detach_callback(dsm_segment *seg, Datum arg)
+{
+ char *ptr = DatumGetPointer(arg);
+ DSMDetachCallbackContext *cb_ctx = (DSMDetachCallbackContext *)ptr;
+ cb_ctx->on_detach_callback(cb_ctx->arg);
+}
+
+static void
+detach_dsm_segment(dsm_handle handle, void (*detach_cb) (void *), void *arg)
+{
+ dsm_segment *seg = dsm_find_mapping(handle);
+
+ /* Detach the segment only if it is already attached */
+ if (seg != NULL)
+ {
+ if (detach_cb != NULL)
+ {
+ DSMDetachCallbackContext *cb_ctx = palloc(sizeof(DSMDetachCallbackContext));
+ cb_ctx->on_detach_callback = detach_cb;
+ cb_ctx->arg = arg;
+
+ on_dsm_detach(seg, call_on_detach_callback, PointerGetDatum(cb_ctx));
+ }
+
+ dsm_unpin_mapping(seg);
+ dsm_detach(seg);
+ }
+}
+
/*
* Initialize or attach a named DSM segment.
*
@@ -172,6 +208,8 @@ GetNamedDSMSegment(const char *name, size_t size,
}
else if (entry->size != size)
{
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
ereport(ERROR,
(errmsg("requested DSM segment size does not match size of "
"existing segment")));
@@ -185,8 +223,11 @@ GetNamedDSMSegment(const char *name, size_t size,
{
seg = dsm_attach(entry->handle);
if (seg == NULL)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
elog(ERROR, "could not map dynamic shared memory segment");
-
+ }
dsm_pin_mapping(seg);
}
@@ -198,3 +239,127 @@ GetNamedDSMSegment(const char *name, size_t size,
return ret;
}
+
+/*
+ * Detach a named DSM segment
+ *
+ * This routine detaches the DSM segment. If the DSM segment was not attached
+ * by this process, then the routine just returns. on_detach_callback is passed
+ * on to dsm_segment by calling on_dsm_detach for the corresponding dsm_segment
+ * before actually detaching.
+ */
+void
+DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot detach a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+}
+
+/*
+ * Attempt to destroy a named DSM segment
+ *
+ * This routine attempts to destroy the DSM segment. We unpin the dsm_segment
+ * and delete the entry from dsm_registry_table. This may not destroy the
+ * dsm_segment instantly, but it would die out once all the other processes
+ * attached to this dsm_segment either exit or manually detach from the
+ * dsm_segment.
+ *
+ * Because we deleted the key from dsm_registry_table, calling
+ * GetNamedDSMSegment with the same key would result into creating a new
+ * dsm_segment instead of retrieving the old unpinned dsm_segment.
+ */
+void
+DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot destroy a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+ dsm_unpin_segment(entry->handle);
+
+ /* dshash_delete_entry calls LWLockRelease internally. We shouldn't
+ * release lock twice */
+ dshash_delete_entry(dsm_registry_table, entry);
+ dshash_delete_key(dsm_registry_table, name);
+
+ MemoryContextSwitchTo(oldcontext);
+}
\ No newline at end of file
diff --git a/src/include/storage/dsm_registry.h b/src/include/storage/dsm_registry.h
index b381e44bc9d..2bd20b82ffd 100644
--- a/src/include/storage/dsm_registry.h
+++ b/src/include/storage/dsm_registry.h
@@ -17,6 +17,14 @@ extern void *GetNamedDSMSegment(const char *name, size_t size,
void (*init_callback) (void *ptr),
bool *found);
+extern void DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
+extern void DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
extern Size DSMRegistryShmemSize(void);
extern void DSMRegistryShmemInit(void);
diff --git a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
index 8ffbd343a05..3764aa31bf6 100644
--- a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
+++ b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
@@ -12,3 +12,15 @@ SELECT get_val_in_shmem();
1236
(1 row)
+SELECT detach_from_tdr_segment();
+ detach_from_tdr_segment
+-------------------------
+ t
+(1 row)
+
+SELECT destroy_tdr_segment();
+ destroy_tdr_segment
+---------------------
+
+(1 row)
+
diff --git a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
index b3351be0a16..24f90658f83 100644
--- a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
+++ b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
@@ -2,3 +2,5 @@ CREATE EXTENSION test_dsm_registry;
SELECT set_val_in_shmem(1236);
\c
SELECT get_val_in_shmem();
+SELECT detach_from_tdr_segment();
+SELECT destroy_tdr_segment();
\ No newline at end of file
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
index 8c55b0919b1..74d8dc50d8a 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -8,3 +8,9 @@ CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
CREATE FUNCTION get_val_in_shmem() RETURNS INT
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION detach_from_tdr_segment() RETURNS BOOL
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION destroy_tdr_segment() RETURNS VOID
+ AS 'MODULE_PATHNAME' LANGUAGE C;
\ No newline at end of file
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 462a80f8790..50a38c5fcd2 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry.c
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -35,6 +35,12 @@ tdr_init_shmem(void *ptr)
state->val = 0;
}
+static void
+reset_tdr_state_to_null(void *arg)
+{
+ tdr_state = NULL;
+}
+
static void
tdr_attach_shmem(void)
{
@@ -74,3 +80,25 @@ get_val_in_shmem(PG_FUNCTION_ARGS)
PG_RETURN_UINT32(ret);
}
+
+PG_FUNCTION_INFO_V1(detach_from_tdr_segment);
+Datum
+detach_from_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DetachNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ reset_tdr_state_to_null, NULL);
+
+ PG_RETURN_BOOL(tdr_state == NULL);
+}
+
+PG_FUNCTION_INFO_V1(destroy_tdr_segment);
+Datum
+destroy_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DestroyNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ NULL, NULL);
+
+ PG_RETURN_VOID();
+}
\ No newline at end of file
--
2.48.1
On Tue, Mar 18, 2025 at 10:05:07AM +0900, Sungwoo Chang wrote:
+/* + * Attempt to destroy a named DSM segment + * + * This routine attempts to destroy the DSM segment. We unpin the dsm_segment + * and delete the entry from dsm_registry_table. This may not destroy the + * dsm_segment instantly, but it would die out once all the other processes + * attached to this dsm_segment either exit or manually detach from the + * dsm_segment. + * + * Because we deleted the key from dsm_registry_table, calling + * GetNamedDSMSegment with the same key would result into creating a new + * dsm_segment instead of retrieving the old unpinned dsm_segment. + */
One of the reasons I avoided adding detach/destroy functionality originally
is because this seems difficult to do correctly. How would an extension
ensure that it doesn't end up with one set of backends attached to a new
segment and another attached to an old one that is pending deletion?
--
nathan
---------- Forwarded message ---------
보낸사람: Sungwoo Chang <swchangdev@gmail.com>
Date: 2025년 6월 13일 (금) 오전 8:03
Subject: Re: dsm_registry: Add detach and destroy features
To: Nathan Bossart <nathandbossart@gmail.com>
One of the reasons I avoided adding detach/destroy functionality originally
is because this seems difficult to do correctly. How would an extension
ensure that it doesn't end up with one set of backends attached to a new
segment and another attached to an old one that is pending deletion?
Sorry for the late response.
I used this patch for my extension in a way that you should always
detach after you are done using the shmem segment. So the situation
you described would happen in a brief moment, but once the extension
finishes its task, the shmem segment will be destroyed naturally as
all processes detach from it.
Would this not be applicable in other extensions?
Import Notes
Reply to msg id not found: CAAdDe3P1sFV7h+mu58XePJW7op9+_4ka0dX9G7ic2+uY0XA@mail.gmail.com
On Fri, Jun 13, 2025 at 08:03:00AM +0900, Sungwoo Chang wrote:
One of the reasons I avoided adding detach/destroy functionality originally
is because this seems difficult to do correctly. How would an extension
ensure that it doesn't end up with one set of backends attached to a new
segment and another attached to an old one that is pending deletion?I used this patch for my extension in a way that you should always detach
after you are done using the shmem segment. So the situation you described
would happen in a brief moment, but once the extension finishes its task,
the shmem segment will be destroyed naturally as all processes detach from
it.Would this not be applicable in other extensions?
I suspect detaching/destroying segments would be applicable elsewhere, I'm
just not sure about providing something with the aforementioned behavior.
Could your use-case be handled with a DSA? On the other thread [0]/messages/by-id/aEyX-9k5vlK2lxjz@nathan, we're
talking about adding a GetNamedDSA() function, which returns a DSA that you
can use to allocate and free shared memory as needed. In theory you could
even detach the DSA if you no longer needed it in a backend, although
that's probably unnecessary.
[0]: /messages/by-id/aEyX-9k5vlK2lxjz@nathan
--
nathan
2025년 6월 14일 (토) 오전 6:50, Nathan Bossart <nathandbossart@gmail.com>님이 작성:
I suspect detaching/destroying segments would be applicable elsewhere, I'm
just not sure about providing something with the aforementioned behavior.Could your use-case be handled with a DSA? On the other thread [0], we're
talking about adding a GetNamedDSA() function, which returns a DSA that you
can use to allocate and free shared memory as needed. In theory you could
even detach the DSA if you no longer needed it in a backend, although
that's probably unnecessary.[0] /messages/by-id/aEyX-9k5vlK2lxjz@nathan
--
nathan
My use-case requires access to the shared memory object through a named key.
Even if we migrate the code to NamedDSA, within the DSA we will need some sort
of a map between the named key and the object to access. So, I think NamedDSA
won't be the solution.
How about when we call destroy, we check if there are other processes
attached to it
and if so, we throw an exception? I checked C++ boost interprocess
library [0]https://www.boost.org/doc/libs/1_55_0/doc/html/interprocess/sharedmemorybetweenprocesses.html#interprocess.sharedmemorybetweenprocesses.sharedmemory.removing, and it
looks like that's the way boost does. This way, we can avoid the aforementioned
"partitioned" scenario.
On Mon, Jun 16, 2025 at 09:02:22AM +0900, Sungwoo Chang wrote:
2025년 6월 14일 (토) 오전 6:50, Nathan Bossart <nathandbossart@gmail.com>님이 작성:
Could your use-case be handled with a DSA? On the other thread [0], we're
talking about adding a GetNamedDSA() function, which returns a DSA that you
can use to allocate and free shared memory as needed. In theory you could
even detach the DSA if you no longer needed it in a backend, although
that's probably unnecessary.My use-case requires access to the shared memory object through a named key.
Even if we migrate the code to NamedDSA, within the DSA we will need some sort
of a map between the named key and the object to access. So, I think NamedDSA
won't be the solution.
Right, you'd need some other shared space for the DSA pointers. In the
other thread, I'm using a dshash table (created via GetNamedDSMHash()) to
store those for test_dsm_registry [0]/messages/by-id/aEyX-9k5vlK2lxjz@nathan. There are probably other ways to do
this.
How about when we call destroy, we check if there are other processes
attached to it and if so, we throw an exception? I checked C++ boost
interprocess library [0], and it looks like that's the way boost does.
This way, we can avoid the aforementioned "partitioned" scenario.
That might work.
[0]: /messages/by-id/aEyX-9k5vlK2lxjz@nathan
--
nathan
Hi,
I created a new patch that throws an exception if we try to call
destroy on a dsm segment that is still in use.
I added a function in dsm.h that returns a refcnt of a given dsm
segment. The destroy function uses that refcnt
getter to check if the dsm segment is in use.
2025년 6월 16일 (월) 오후 11:40, Nathan Bossart <nathandbossart@gmail.com>님이 작성:
Show quoted text
On Mon, Jun 16, 2025 at 09:02:22AM +0900, Sungwoo Chang wrote:
2025년 6월 14일 (토) 오전 6:50, Nathan Bossart <nathandbossart@gmail.com>님이 작성:
Could your use-case be handled with a DSA? On the other thread [0], we're
talking about adding a GetNamedDSA() function, which returns a DSA that you
can use to allocate and free shared memory as needed. In theory you could
even detach the DSA if you no longer needed it in a backend, although
that's probably unnecessary.My use-case requires access to the shared memory object through a named key.
Even if we migrate the code to NamedDSA, within the DSA we will need some sort
of a map between the named key and the object to access. So, I think NamedDSA
won't be the solution.Right, you'd need some other shared space for the DSA pointers. In the
other thread, I'm using a dshash table (created via GetNamedDSMHash()) to
store those for test_dsm_registry [0]. There are probably other ways to do
this.How about when we call destroy, we check if there are other processes
attached to it and if so, we throw an exception? I checked C++ boost
interprocess library [0], and it looks like that's the way boost does.
This way, we can avoid the aforementioned "partitioned" scenario.That might work.
[0] /messages/by-id/aEyX-9k5vlK2lxjz@nathan
--
nathan
Attachments:
v4-0001-add-detach-and-destroy-for-dsm_registry.patchapplication/octet-stream; name=v4-0001-add-detach-and-destroy-for-dsm_registry.patchDownload
From ac4da514d86ce356700a1a903f6717d5fa9d2485 Mon Sep 17 00:00:00 2001
From: swchangdev <swchangdev@gmail.com>
Date: Fri, 14 Mar 2025 10:00:12 +0900
Subject: [PATCH] Add detach and destroy feature to dsm_registry
---
src/backend/storage/ipc/dsm.c | 15 ++
src/backend/storage/ipc/dsm_registry.c | 182 +++++++++++++++++-
src/include/storage/dsm.h | 1 +
src/include/storage/dsm_registry.h | 8 +
.../expected/test_dsm_registry.out | 12 ++
.../sql/test_dsm_registry.sql | 2 +
.../test_dsm_registry--1.0.sql | 6 +
.../test_dsm_registry/test_dsm_registry.c | 28 +++
8 files changed, 253 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index f92a52a00e6..1e4ab0557e4 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -1125,6 +1125,21 @@ dsm_segment_handle(dsm_segment *seg)
return seg->handle;
}
+uint32_t dsm_segment_refcnt(dsm_segment *seg)
+{
+ uint32_t refcnt = 0;
+
+ if (seg->control_slot != INVALID_CONTROL_SLOT)
+ {
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ Assert(dsm_control->item[seg->control_slot].handle == seg->handle);
+ refcnt = dsm_control->item[seg->control_slot].refcnt;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+ }
+
+ return refcnt;
+}
+
/*
* Register an on-detach callback for a dynamic shared memory segment.
*/
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index 1d4fd31ffed..8bc3bd28a50 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -47,6 +47,12 @@ typedef struct DSMRegistryEntry
size_t size;
} DSMRegistryEntry;
+typedef struct DSMDetachCallbackContext
+{
+ void (*on_detach_callback) (void *);
+ void *arg;
+} DSMDetachCallbackContext;
+
static const dshash_parameters dsh_params = {
offsetof(DSMRegistryEntry, handle),
sizeof(DSMRegistryEntry),
@@ -121,6 +127,43 @@ init_dsm_registry(void)
LWLockRelease(DSMRegistryLock);
}
+static void
+call_on_detach_callback(dsm_segment *seg, Datum arg)
+{
+ char *ptr = DatumGetPointer(arg);
+ DSMDetachCallbackContext *cb_ctx = (DSMDetachCallbackContext *)ptr;
+ cb_ctx->on_detach_callback(cb_ctx->arg);
+}
+
+static void
+detach_dsm_segment(dsm_handle handle, void (*detach_cb) (void *), void *arg)
+{
+ dsm_segment *seg = dsm_find_mapping(handle);
+
+ /* Detach the segment only if it is already attached */
+ if (seg != NULL)
+ {
+ if (detach_cb != NULL)
+ {
+ DSMDetachCallbackContext *cb_ctx = palloc(sizeof(DSMDetachCallbackContext));
+ cb_ctx->on_detach_callback = detach_cb;
+ cb_ctx->arg = arg;
+
+ on_dsm_detach(seg, call_on_detach_callback, PointerGetDatum(cb_ctx));
+ }
+
+ dsm_unpin_mapping(seg);
+ dsm_detach(seg);
+ }
+}
+
+static bool
+is_dsm_segment_in_use(dsm_handle handle)
+{
+ dsm_segment *seg = dsm_find_mapping(handle);
+ return seg != NULL && dsm_segment_refcnt(seg) > 2;
+}
+
/*
* Initialize or attach a named DSM segment.
*
@@ -172,6 +215,8 @@ GetNamedDSMSegment(const char *name, size_t size,
}
else if (entry->size != size)
{
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
ereport(ERROR,
(errmsg("requested DSM segment size does not match size of "
"existing segment")));
@@ -185,8 +230,11 @@ GetNamedDSMSegment(const char *name, size_t size,
{
seg = dsm_attach(entry->handle);
if (seg == NULL)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
elog(ERROR, "could not map dynamic shared memory segment");
-
+ }
dsm_pin_mapping(seg);
}
@@ -198,3 +246,135 @@ GetNamedDSMSegment(const char *name, size_t size,
return ret;
}
+
+/*
+ * Detach a named DSM segment
+ *
+ * This routine detaches the DSM segment. If the DSM segment was not attached
+ * by this process, then the routine just returns. on_detach_callback is passed
+ * on to dsm_segment by calling on_dsm_detach for the corresponding dsm_segment
+ * before actually detaching.
+ */
+void
+DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot detach a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+}
+
+/*
+ * Attempt to destroy a named DSM segment
+ *
+ * This routine attempts to destroy the DSM segment. We unpin the dsm_segment
+ * and delete the entry from dsm_registry_table. This may not destroy the
+ * dsm_segment instantly, but it would die out once all the other processes
+ * attached to this dsm_segment either exit or manually detach from the
+ * dsm_segment.
+ *
+ * Because we deleted the key from dsm_registry_table, calling
+ * GetNamedDSMSegment with the same key would result into creating a new
+ * dsm_segment instead of retrieving the old unpinned dsm_segment.
+ */
+void
+DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *), void *arg)
+{
+ DSMRegistryEntry *entry;
+ MemoryContext oldcontext;
+
+ if (!name || *name == '\0')
+ ereport(ERROR,
+ (errmsg("DSM segment name cannot be empty")));
+
+ if (strlen(name) >= offsetof(DSMRegistryEntry, handle))
+ ereport(ERROR,
+ (errmsg("DSM segment name too long")));
+
+ if (size == 0)
+ ereport(ERROR,
+ (errmsg("DSM segment size must be nonzero")));
+
+ /* Be sure any local memory allocated by DSM/DSA routines is persistent. */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ /* Connect to the registry. */
+ init_dsm_registry();
+
+ entry = dshash_find(dsm_registry_table, name, true);
+
+ if (entry == NULL)
+ {
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot destroy a DSM segment that does not exist")));
+ }
+
+ if (entry->size != size)
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("requested DSM segment size does not match size of "
+ "existing segment")));
+ }
+
+ if (is_dsm_segment_in_use(entry->handle))
+ {
+ dshash_release_lock(dsm_registry_table, entry);
+ MemoryContextSwitchTo(oldcontext);
+ ereport(ERROR,
+ (errmsg("cannot destroy a DSM segment that is still in use")));
+ }
+
+ detach_dsm_segment(entry->handle, on_detach_callback, arg);
+ dsm_unpin_segment(entry->handle);
+
+ /* dshash_delete_entry calls LWLockRelease internally. We shouldn't
+ * release lock twice */
+ dshash_delete_entry(dsm_registry_table, entry);
+ dshash_delete_key(dsm_registry_table, name);
+
+ MemoryContextSwitchTo(oldcontext);
+}
\ No newline at end of file
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 2302cc7f40b..a9954a07201 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -49,6 +49,7 @@ extern dsm_segment *dsm_find_mapping(dsm_handle handle);
extern void *dsm_segment_address(dsm_segment *seg);
extern Size dsm_segment_map_length(dsm_segment *seg);
extern dsm_handle dsm_segment_handle(dsm_segment *seg);
+extern uint32_t dsm_segment_refcnt(dsm_segment *seg);
/* Cleanup hooks. */
typedef void (*on_dsm_detach_callback) (dsm_segment *, Datum arg);
diff --git a/src/include/storage/dsm_registry.h b/src/include/storage/dsm_registry.h
index b381e44bc9d..2bd20b82ffd 100644
--- a/src/include/storage/dsm_registry.h
+++ b/src/include/storage/dsm_registry.h
@@ -17,6 +17,14 @@ extern void *GetNamedDSMSegment(const char *name, size_t size,
void (*init_callback) (void *ptr),
bool *found);
+extern void DetachNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
+extern void DestroyNamedDSMSegment(const char *name, size_t size,
+ void (*on_detach_callback) (void *),
+ void *arg);
+
extern Size DSMRegistryShmemSize(void);
extern void DSMRegistryShmemInit(void);
diff --git a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
index 8ffbd343a05..3764aa31bf6 100644
--- a/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
+++ b/src/test/modules/test_dsm_registry/expected/test_dsm_registry.out
@@ -12,3 +12,15 @@ SELECT get_val_in_shmem();
1236
(1 row)
+SELECT detach_from_tdr_segment();
+ detach_from_tdr_segment
+-------------------------
+ t
+(1 row)
+
+SELECT destroy_tdr_segment();
+ destroy_tdr_segment
+---------------------
+
+(1 row)
+
diff --git a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
index b3351be0a16..24f90658f83 100644
--- a/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
+++ b/src/test/modules/test_dsm_registry/sql/test_dsm_registry.sql
@@ -2,3 +2,5 @@ CREATE EXTENSION test_dsm_registry;
SELECT set_val_in_shmem(1236);
\c
SELECT get_val_in_shmem();
+SELECT detach_from_tdr_segment();
+SELECT destroy_tdr_segment();
\ No newline at end of file
diff --git a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
index 8c55b0919b1..74d8dc50d8a 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry--1.0.sql
@@ -8,3 +8,9 @@ CREATE FUNCTION set_val_in_shmem(val INT) RETURNS VOID
CREATE FUNCTION get_val_in_shmem() RETURNS INT
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION detach_from_tdr_segment() RETURNS BOOL
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION destroy_tdr_segment() RETURNS VOID
+ AS 'MODULE_PATHNAME' LANGUAGE C;
\ No newline at end of file
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 462a80f8790..50a38c5fcd2 100644
--- a/src/test/modules/test_dsm_registry/test_dsm_registry.c
+++ b/src/test/modules/test_dsm_registry/test_dsm_registry.c
@@ -35,6 +35,12 @@ tdr_init_shmem(void *ptr)
state->val = 0;
}
+static void
+reset_tdr_state_to_null(void *arg)
+{
+ tdr_state = NULL;
+}
+
static void
tdr_attach_shmem(void)
{
@@ -74,3 +80,25 @@ get_val_in_shmem(PG_FUNCTION_ARGS)
PG_RETURN_UINT32(ret);
}
+
+PG_FUNCTION_INFO_V1(detach_from_tdr_segment);
+Datum
+detach_from_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DetachNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ reset_tdr_state_to_null, NULL);
+
+ PG_RETURN_BOOL(tdr_state == NULL);
+}
+
+PG_FUNCTION_INFO_V1(destroy_tdr_segment);
+Datum
+destroy_tdr_segment(PG_FUNCTION_ARGS)
+{
+ DestroyNamedDSMSegment("test_dsm_registry",
+ sizeof(TestDSMRegistryStruct),
+ NULL, NULL);
+
+ PG_RETURN_VOID();
+}
\ No newline at end of file
--
2.34.1
On Wed, Jun 18, 2025 at 09:03:59AM +0900, Sungwoo Chang wrote:
I created a new patch that throws an exception if we try to call
destroy on a dsm segment that is still in use.
I added a function in dsm.h that returns a refcnt of a given dsm
segment. The destroy function uses that refcnt
getter to check if the dsm segment is in use.
Thanks for the patch. I'm still interested to hear whether the proposed
GetNamedDSA() feature could work for your use-case, as I described upthread
[0]: /messages/by-id/aFAszMKD69AyA4c1@nathan
[0]: /messages/by-id/aFAszMKD69AyA4c1@nathan
--
nathan