Re: introduce dynamic shared memory registry

On Mon, Jan 8, 2024 at 10:53 AM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> On Sat, Jan 6, 2024 at 10:05 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
> wrote:
> >
> > I kept this the same, as I didn't see any need to tie the key size to
> > NAMEDATALEN.
>
> Thanks. A fresh look at the v5 patches left me with the following thoughts:
>
> 1. I think we need to add some notes about this new way of getting
> shared memory for external modules in the <title>Shared Memory and
> LWLocks</title> section in xfunc.sgml? This will at least tell there's
> another way for external modules to get shared memory, not just with
> the shmem_request_hook and shmem_startup_hook. What do you think?
>
> 2. FWIW, I'd like to call this whole feature "Support for named DSM
> segments in Postgres". Do you see anything wrong with this?
>
> 3. IIUC, this feature eventually makes both shmem_request_hook and
> shmem_startup_hook pointless, no? Or put another way, what's the
> significance of shmem request and startup hooks in lieu of this new
> feature? I think it's quite possible to get rid of the shmem request
> and startup hooks (of course, not now but at some point in future to
> not break the external modules), because all the external modules can
> allocate and initialize the same shared memory via
> dsm_registry_init_or_attach and its init_callback. All the external
> modules will then need to call dsm_registry_init_or_attach in their
> _PG_init callbacks and/or in their bg worker's main functions in case
> the modules intend to start up bg workers. Am I right?
>
> 4. With the understanding in comment #3, can pg_stat_statements and
> test_slru.c get rid of its shmem_request_hook and shmem_startup_hook
> and use dsm_registry_init_or_attach? It's not that this patch need to
> remove them now, but just asking if there's something in there that
> makes this new feature unusable.
>

+1, since doing for pg_prewarm, better to do for these modules as well.

A minor comment for v5:

+void *
+dsm_registry_init_or_attach(const char *key, size_t size,

I think the name could be simple as dsm_registry_init() like we use
elsewhere e.g. ShmemInitHash() which doesn't say attach explicitly.

Similarly, I think dshash_find_or_insert() can be as simple as
dshash_search() and
accept HASHACTION like hash_search().

Regards,
Amul