Re: introduce dynamic shared memory registry

On Tue, Dec 5, 2023 at 9:17 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> Every once in a while, I find myself wanting to use shared memory in a
> loadable module without requiring it to be loaded at server start via
> shared_preload_libraries.  The DSM API offers a nice way to create and
> manage dynamic shared memory segments, so creating a segment after server
> start is easy enough.  However, AFAICT there's no easy way to teach other
> backends about the segment without storing the handles in shared memory,
> which puts us right back at square one.
>
> The attached 0001 introduces a "DSM registry" to solve this problem.  The
> API provides an easy way to allocate/initialize a segment or to attach to
> an existing one.  The registry itself is just a dshash table that stores
> the handles keyed by a module-specified string.  0002 adds a test for the
> registry that demonstrates basic usage.

+1 for something like this.

> I don't presently have any concrete plans to use this for anything, but I
> thought it might be useful for extensions for caching, etc. and wanted to
> see whether there was any interest in the feature.

Isn't the worker_spi best place to show the use of the DSM registry
instead of a separate test extension? Note the custom wait event
feature that added its usage code to worker_spi. Since worker_spi
demonstrates typical coding patterns, having just set_val_in_shmem()
and get_val_in_shmem() in there makes this patch simple and shaves
some code off.

Comments on the v1 patch set:

1. IIUC, this feature lets external modules create as many DSM
segments as possible with different keys right? If yes, is capping the
max number of DSMs a good idea?

2. Why does this feature have to deal with DSMs? Why not DSAs? With
DSA and an API that gives the DSA handle to the external modules, the
modules can dsa_allocate and dsa_free right? Do you see any problem
with it?

3.
+typedef struct DSMRegistryEntry
+{
+    char        key[256];

Key length 256 feels too much, can it be capped at NAMEDATALEN 64
bytes (similar to some of the key lengths for hash_create()) to start
with?

4. Do we need on_dsm_detach for each DSM created?
dsm_backend_shutdown

5.
+ *
+ * *ptr should initially be set to NULL.  If it is not NULL, this routine will
+ * assume that the segment has already been attached to the current session.
+ * Otherwise, this routine will set *ptr appropriately.

+    /* Quick exit if the value is already set. */
+    if (*ptr)
+        return;

Instead of the above assumption and quick exit condition, can it be
something like if (dsm_find_mapping(dsm_segment_handle(*ptr)) != NULL)
return;?

6.
+static pg_atomic_uint32 *val;

Any specific reason for it to be an atomic variable?

7.
+static pg_atomic_uint32 *val;

Instead of a run-of-the-mill example with just an integer val that
gets stored in shared memory, can it be something more realistic, a
struct with 2 or more variables or a struct to store linked list
(slist_head or dlist_head) in shared memory or such?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com