On Mon, Jan 8, 2024 at 10:53 AM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote:
> On Sat, Jan 6, 2024 at 10:05 PM Nathan Bossart <nathandboss...@gmail.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
