Re: introduce dynamic shared memory registry

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].

> +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]. 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] https://postgr.es/m/20231219160117.GB831499%40nathanxps13
[1] https://postgr.es/m/20231220153342.GA833819%40nathanxps13

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