On Fri, Dec 29, 2023 at 08:53:54PM +0530, Bharath Rupireddy wrote:
> 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.
I don't have a strong opinion here, but I lean towards still having a
dedicated test suite, if for no other reason that to guarantee some
coverage even if the other in-tree uses disappear.
> 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.
I would be interested to see more details about the crashes you are seeing.
Is this unique to the registry or an existing problem with DSM segments?
> 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.
I'm not really expecting folks to use more than, say, 16 characters for the
key, but I intentionally set it much higher in case someone did have a
reason to use longer keys. I'll lower it to 64 in the next revision unless
anyone else objects.
> 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.
See [0]. FWIW I tend to agree that we could leave this one out for now.
> 3. I think we don't need miscadmin.h inclusion in autoprewarm.c, do we?
I'll take a look at this one.
[0] https://postgr.es/m/ZYIu_JL-usgd3E1q%40paquier.xyz
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
