Re: introduce dynamic shared memory registry

On Wed, Dec 20, 2023 at 9:33 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> 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.

With the use of dsm registry for pg_prewarm, do we need this
test_dsm_registry module at all? Because 0002 patch pretty much
demonstrates how to use the DSM registry. With this comment and my
earlier comment on incorporating the use of dsm registry in
worker_spi, I'm trying to make a point to reduce the code for this
feature. However, if others have different opinions, I'm okay with
having a separate test module.

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

I've tried with a shared memory structure size of 10GB on my
development machine and I have seen an intermittent crash (I haven't
got a chance to dive deep into it). I think the shared memory that a
named-DSM segment can get via this DSM registry feature depends on the
total shared memory area that a typical DSM client can allocate today.
I'm not sure it's worth it to limit the shared memory for this feature
given that we don't limit the shared memory via startup hook.

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

Per my understanding, this feature allows one to define and manage
named-DSM segments.

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

Are we expecting, for instance, a 128-bit UUID being used as a key and
hence limiting it to a higher value 256 instead of just NAMEDATALEN?
My thoughts were around saving a few bytes of shared memory space that
can get higher when multiple modules using a DSM registry with
multiple DSM segments.

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

On Thu, Dec 28, 2023 at 9:05 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Wed, Dec 27, 2023 at 01:53:27PM -0600, Nathan Bossart wrote:
> > Here is a new version of the patch. In addition to various small changes,
> > I've rewritten the test suite to use an integer and a lock, added a
> > dsm_registry_find() function, and adjusted autoprewarm to use the registry.
>
> Here's a v3 that fixes a silly mistake in the test.

Some comments on the v3 patch set:

1. Typo: missing "an" before "already-attached".
+ /* Return address of already-attached DSM registry entry. */

2. Do you see any immediate uses of dsm_registry_find()? Especially
given that dsm_registry_init_or_attach() does the necessary work
(returns the DSM address if DSM already exists for a given key) for a
postgres process. If there is no immediate use (the argument to remove
this function goes similar to detach_cb above), I'm sure we can
introduce it when there's one.

3. I think we don't need miscadmin.h inclusion in autoprewarm.c, do we?

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