introduce dynamic shared memory registry

On 12/4/23 22:46, Nathan Bossart 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.

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.

Notwithstanding any dragons there may be, and not having actually looked
at the the patches, I love the concept! +<many>

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

On Tue, Dec 5, 2023 at 10:35 AM Joe Conway <mail@joeconway.com> wrote:

Notwithstanding any dragons there may be, and not having actually looked
at the the patches, I love the concept! +<many>

Seems fine to me too. I haven't looked at the patches or searched for
dragons either, though.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Dec 04, 2023 at 09:46:47PM -0600, Nathan Bossart wrote:

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.

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.

Yes, tracking that in a more central way can have many usages, so your
patch sounds like a good idea. Note that we have one case in core
that be improved and make use of what you have here: autoprewarm.c.

The module supports the launch of dynamic workers but the library may
not be loaded with shared_preload_libraries, meaning that it can
allocate a chunk of shared memory worth a size of
AutoPrewarmSharedState without having requested it in a
shmem_request_hook. AutoPrewarmSharedState could be moved to a DSM
and tracked with the shared hash table introduced by the patch instead
of acquiring AddinShmemInitLock while eating the plate of other
facilities that asked for a chunk of shmem, leaving any conflict
handling to dsm_registry_table.

+dsm_registry_init_or_attach(const char *key, void **ptr, size_t size,
+                           void (*init_callback) (void *ptr))

This is shaped around dshash_find_or_insert(), but it looks like you'd
want an equivalent for dshash_find(), as well.
--
Michael

On 5/12/2023 10:46, Nathan Bossart wrote:

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.

I am delighted that you commenced this thread.
Designing extensions, every time I feel pain introducing one shared
value or some global stat, the extension must be required to be loadable
on startup only. It reduces the flexibility of even very lightweight
extensions, which look harmful to use in a cloud.

--
regards,
Andrei Lepikhov
Postgres Professional

On 18/12/2023 13:39, Andrei Lepikhov wrote:

On 5/12/2023 10:46, Nathan Bossart wrote:

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.

I am delighted that you commenced this thread.
Designing extensions, every time I feel pain introducing one shared
value or some global stat, the extension must be required to be loadable
on startup only. It reduces the flexibility of even very lightweight
extensions, which look harmful to use in a cloud.

After looking into the code, I have some comments:
1. The code looks good; I didn't find possible mishaps. Some proposed
changes are in the attachment.
2. I think a separate file for this feature looks too expensive.
According to the gist of that code, it is a part of the DSA module.
3. The dsm_registry_init_or_attach routine allocates a DSM segment. Why
not create dsa_area for a requestor and return it?

--
regards,
Andrei Lepikhov
Postgres Professional

Hi!

This patch looks like a good solution for a pain in the ass, I'm too for
this patch to be committed.
Have looked through the code and agree with Andrei, the code looks good.
Just a suggestion - maybe it is worth adding a function for detaching the
segment,
for cases when we unload and/or re-load the extension?

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

On Mon, Dec 18, 2023 at 3:32 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:

2. I think a separate file for this feature looks too expensive.
According to the gist of that code, it is a part of the DSA module.

-1. I think this is a totally different thing than DSA. More files
aren't nearly as expensive as the confusion that comes from smushing
unrelated things together.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Dec 18, 2023 at 03:32:08PM +0700, Andrei Lepikhov wrote:

3. The dsm_registry_init_or_attach routine allocates a DSM segment. Why not
create dsa_area for a requestor and return it?

My assumption is that most modules just need a fixed-size segment, and if
they really needed a DSA segment, the handle, tranche ID, etc. could just
be stored in the DSM segment. Maybe that assumption is wrong, though...

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Dec 18, 2023 at 12:05:28PM +0300, Nikita Malakhov wrote:

Just a suggestion - maybe it is worth adding a function for detaching the
segment,
for cases when we unload and/or re-load the extension?

Hm. We don't presently have a good way to unload a library, but you can
certainly DROP EXTENSION, in which case you might expect the segment to go
away or at least be reset. But even today, once a preloaded library is
loaded, it stays loaded and its shared memory remains regardless of whether
you CREATE/DROP extension. Can you think of problems with keeping the
segment attached?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Dec 19, 2023 at 10:49:23AM -0500, Robert Haas wrote:

On Mon, Dec 18, 2023 at 3:32 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:

2. I think a separate file for this feature looks too expensive.
According to the gist of that code, it is a part of the DSA module.

-1. I think this is a totally different thing than DSA. More files
aren't nearly as expensive as the confusion that comes from smushing
unrelated things together.

Agreed. I think there's a decent chance that more functionality will be
added to this registry down the line, in which case it will be even more
important that this stuff stays separate from the tools it is built with.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Dec 08, 2023 at 04:36:52PM +0900, Michael Paquier wrote:

Yes, tracking that in a more central way can have many usages, so your
patch sounds like a good idea. Note that we have one case in core
that be improved and make use of what you have here: autoprewarm.c.

I'll add a patch for autoprewarm.c. Even if we don't proceed with that
change, it'll be a good demonstration.

+dsm_registry_init_or_attach(const char *key, void **ptr, size_t size,
+                           void (*init_callback) (void *ptr))

This is shaped around dshash_find_or_insert(), but it looks like you'd
want an equivalent for dshash_find(), as well.

What is the use-case for only verifying the existence of a segment?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Dec 19, 2023 at 10:19:11AM -0600, Nathan Bossart wrote:

On Fri, Dec 08, 2023 at 04:36:52PM +0900, Michael Paquier wrote:

Yes, tracking that in a more central way can have many usages, so your
patch sounds like a good idea. Note that we have one case in core
that be improved and make use of what you have here: autoprewarm.c.

I'll add a patch for autoprewarm.c. Even if we don't proceed with that
change, it'll be a good demonstration.

Cool, thanks. It could just be a separate change on top of the main
one.

+dsm_registry_init_or_attach(const char *key, void **ptr, size_t size,
+                           void (*init_callback) (void *ptr))

This is shaped around dshash_find_or_insert(), but it looks like you'd
want an equivalent for dshash_find(), as well.

What is the use-case for only verifying the existence of a segment?

One case I was thinking about is parallel aggregates that can define
combining and serial/deserial functions, where some of the operations
could happen in shared memory, requiring a DSM, and where each process
doing some aggregate combining would expect a DSM to exist before
making use of it. So a registry wrapper for dshash_find() could be
used as a way to perform sanity checks with what's stored in the
registry.
--
Michael

On Tue, Dec 19, 2023 at 10:14:44AM -0600, Nathan Bossart wrote:

On Tue, Dec 19, 2023 at 10:49:23AM -0500, Robert Haas wrote:

On Mon, Dec 18, 2023 at 3:32 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:

2. I think a separate file for this feature looks too expensive.
According to the gist of that code, it is a part of the DSA module.

-1. I think this is a totally different thing than DSA. More files
aren't nearly as expensive as the confusion that comes from smushing
unrelated things together.

Agreed. I think there's a decent chance that more functionality will be
added to this registry down the line, in which case it will be even more
important that this stuff stays separate from the tools it is built with.

+1 for keeping a clean separation between both.
--
Michael

On 20/12/2023 07:04, Michael Paquier wrote:

On Tue, Dec 19, 2023 at 10:14:44AM -0600, Nathan Bossart wrote:

On Tue, Dec 19, 2023 at 10:49:23AM -0500, Robert Haas wrote:

On Mon, Dec 18, 2023 at 3:32 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:

2. I think a separate file for this feature looks too expensive.
According to the gist of that code, it is a part of the DSA module.

-1. I think this is a totally different thing than DSA. More files
aren't nearly as expensive as the confusion that comes from smushing
unrelated things together.

Agreed. I think there's a decent chance that more functionality will be
added to this registry down the line, in which case it will be even more
important that this stuff stays separate from the tools it is built with.

+1 for keeping a clean separation between both.

Thanks, I got the reason.
In that case, maybe change the test case to make it closer to real-life
usage - with locks and concurrent access (See attachment)?

--
regards,
Andrei Lepikhov
Postgres Professional

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

On Wed, Dec 20, 2023 at 11:02:58AM +0200, Andrei Lepikhov wrote:

In that case, maybe change the test case to make it closer to real-life
usage - with locks and concurrent access (See attachment)?

I'm not following why we should make this test case more complicated. It
is only intended to test the DSM registry machinery, and setting/retrieving
an atomic variable seems like a realistic use-case to me.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi, all

I see most xxxShmemInit functions have the logic to handle IsUnderPostmaster env.
Do we need to consider it in DSMRegistryShmemInit() too? For example, add some assertions.
Others LGTM.

Zhang Mingli
www.hashdata.xyz

On Wed, Dec 20, 2023 at 03:28:38PM +0530, Bharath Rupireddy wrote:

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.

I don't agree. The test case really isn't that complicated, and I'd rather
have a dedicated test suite for this feature that we can build on instead
of trying to squeeze it into something unrelated.

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?

Why? Even if it is a good idea, what limit could we choose that wouldn't
be arbitrary and eventually cause problems down the road?

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?

Please see upthread discussion [0]/messages/by-id/20231219160117.GB831499@nathanxps13.

+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?

Why is it too much?

4. Do we need on_dsm_detach for each DSM created?

Presently, I've designed this such that the DSM remains attached for the
lifetime of a session (and stays present even if all attached sessions go
away) to mimic what you get when you allocate shared memory during startup.
Perhaps there's a use-case for having backends do some cleanup before
exiting, in which case a detach_cb might be useful. IMHO we should wait
for a concrete use-case before adding too many bells and whistles, though.

+ * *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;?

Yeah, I think something like that could be better. One of the things I
dislike about the v1 API is that it depends a little too much on the caller
doing exactly the right things, and I think it's possible to make it a
little more robust.

+static pg_atomic_uint32 *val;

Any specific reason for it to be an atomic variable?

A regular integer would probably be fine for testing, but I figured we
might as well ensure correctness for when this code is inevitably
copy/pasted somewhere.

+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?

This is the second time this has come up [1]/messages/by-id/20231220153342.GA833819@nathanxps13. The intent of this test is
to verify the DSM registry behavior, not how folks are going to use the
shared memory it manages, so I'm really not inclined to make this more
complicated without a good reason. I don't mind changing this if I'm
outvoted on this one, though.

[0]: /messages/by-id/20231219160117.GB831499@nathanxps13
[1]: /messages/by-id/20231220153342.GA833819@nathanxps13

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Dec 21, 2023 at 12:03:18AM +0800, Zhang Mingli wrote:

I see most xxxShmemInit functions have the logic to handle IsUnderPostmaster env.
Do we need to consider it in DSMRegistryShmemInit() too? For example, add some assertions.
Others LGTM.

Good point. I _think_ the registry is safe to set up and use in
single-user mode but not in a regular postmaster process. It'd probably be
wise to add some assertions along those lines, but even if we didn't, I
think the DSM code has existing assertions that will catch it. In any
case, I'd like to avoid requiring folks to add special
single-user-mode-only logic if we can avoid it.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com