Re: introduce dynamic shared memory registry

On Sun, Jan 14, 2024 at 3:11 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> Here is a new version of the patch set with these changes.

Thanks. Here are some comments on v7-0002.

1.
+GetNamedDSMSegment(const char *name, size_t size,
+                   void (*init_callback) (void *ptr), bool *found)
+{
+
+    Assert(name);
+    Assert(size);
+    Assert(found);

I've done some input validation to GetNamedDSMSegment():

With an empty key name (""), it works, but do we need that in
practice? Can't we error out saying the name can't be empty?

With a size 0, an assertion is fine, but in production (without the
assertion), I'm seeing the following errors.

2024-01-16 04:49:28.961 UTC client backend[864369]
pg_regress/test_dsm_registry ERROR:  could not resize shared memory
segment "/PostgreSQL.3701090278" to 0 bytes: Invalid argument
2024-01-16 04:49:29.264 UTC postmaster[864357] LOG:  server process
(PID 864370) was terminated by signal 11: Segmentation fault

I think it's better for GetNamedDSMSegment() to error out on empty
'name' and size 0. This makes the user-facing function
GetNamedDSMSegment more concrete.

2.
+void *
+GetNamedDSMSegment(const char *name, size_t size,
+                   void (*init_callback) (void *ptr), bool *found)

+    Assert(found);

Why is input parameter 'found' necessary to be passed by the caller?
Neither the test module added, nor the pg_prewarm is using the found
variable. The function will anyway create the DSM segment if one with
the given name isn't found. IMO, found is an optional parameter for
the caller. So, the assert(found) isn't necessary.

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