Subject: Re: reorganize "Shared Memory and LWLocks" section of docs

From: Aleksander Alekseev <[email-masked]>
Date: Fri, 12 Jan 2024 17:12:28 +0300

Hi,

> I recently began trying to write documentation for the dynamic shared
> memory registry feature [0], and I noticed that the "Shared Memory and
> LWLocks" section of the documentation might need some improvement.

I know that feeling.

> Thoughts?

"""
Any registered shmem_startup_hook will be executed shortly after each
backend attaches to shared memory.
"""

IMO the word "each" here can give the wrong impression as if there are
certain guarantees about synchronization between backends. Maybe we
should change this to simply "... will be executed shortly after
[the?] backend attaches..."

"""
should ensure that only one process allocates a new tranche_id
(LWLockNewTrancheId) and initializes each new LWLock
(LWLockInitialize).
"""

Personally I think that reminding the corresponding function name here
is redundant and complicates reading just a bit. But maybe it's just
me.

Except for these nitpicks the patch looks good.

-- 
Best regards,
Aleksander Alekseev

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 12 Jan 2024 09:46:50 -0600

Thanks for reviewing.

> [the?] backend attaches..."

I see what you mean, but I don't think the problem is the word "each."  I
think the problem is the use of passive voice.  What do you think about
something like

	Each backend will execute the registered shmem_startup_hook shortly
	after it attaches to shared memory.

> is redundant and complicates reading just a bit. But maybe it's just

Yeah, I waffled on this one.  I don't mind removing it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 12 Jan 2024 11:23:50 -0600

> Yeah, I waffled on this one.  I don't mind removing it.

Here is a new version of the patch with these changes.

>From 7cf22727a96757bf212ec106bd471bf55a6981b9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Jan 2024 21:55:25 -0600
Subject: [PATCH v2 1/1] reorganize shared memory and lwlocks documentation

---
 doc/src/sgml/xfunc.sgml | 182 +++++++++++++++++++++++++---------------
 1 file changed, 114 insertions(+), 68 deletions(-)

diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 89116ae74c..0ba52b41d4 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3397,90 +3397,136 @@ CREATE FUNCTION make_array(anyelement) RETURNS anyarray
    </sect2>
 
    <sect2 id="xfunc-shared-addin">
-    <title>Shared Memory and LWLocks</title>
+    <title>Shared Memory</title>
 
-    <para>
-     Add-ins can reserve LWLocks and an allocation of shared memory on server
-     startup.  The add-in's shared library must be preloaded by specifying
-     it in
-     <xref linkend="guc-shared-preload-libraries"/><indexterm><primary>shared_preload_libraries</primary></indexterm>.
-     The shared library should register a <literal>shmem_request_hook</literal>
-     in its <function>_PG_init</function> function.  This
-     <literal>shmem_request_hook</literal> can reserve LWLocks or shared memory.
-     Shared memory is reserved by calling:
+    <sect3 id="xfunc-shared-addin-at-startup">
+     <title>Requesting Shared Memory at Startup</title>
+
+     <para>
+      Add-ins can reserve shared memory on server startup.  To do so, the
+      add-in's shared library must be preloaded by specifying it in
+      <xref linkend="guc-shared-preload-libraries"/><indexterm><primary>shared_preload_libraries</primary></indexterm>.
+      The shared library should also register a
+      <literal>shmem_request_hook</literal> in its
+      <function>_PG_init</function> function.  This
+      <literal>shmem_request_hook</literal> can reserve shared memory by
+      calling:
 <programlisting>
 void RequestAddinShmemSpace(int size)
 </programlisting>
-     from your <literal>shmem_request_hook</literal>.
-    </para>
-    <para>
-     LWLocks are reserved by calling:
+      Each backend sould obtain a pointer to the reserved shared memory by
+      calling:
+<programlisting>
+void *ShmemInitStruct(const char *name, Size size, bool *foundPtr)
+</programlisting>
+      If this function sets <literal>foundPtr</literal> to
+      <literal>false</literal>, the caller should proceed to initialize the
+      contents of the reserved shared memory.  If <literal>foundPtr</literal>
+      is set to <literal>true</literal>, the shared memory was already
+      initialized by another backend, and the caller need not initialize
+      further.
+     </para>
+
+     <para>
+      To avoid race conditions, each backend should use the LWLock
+      <function>AddinShmemInitLock</function> when initializing its allocation
+      of shared memory, as shown here:
+<programlisting>
+static mystruct *ptr = NULL;
+bool        found;
+
+LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+ptr = ShmemInitStruct("my struct name", size, &amp;found);
+if (!found)
+{
+    ... initialize contents of shared memory ...
+    ptr->locks = GetNamedLWLockTranche("my tranche name");
+}
+LWLockRelease(AddinShmemInitLock);
+</programlisting>
+      <literal>shmem_startup_hook</literal> provides a convenient place for the
+      initialization code, but it is not strictly required that all such code
+      be placed in this hook.  Each backend will execute the registered
+      <literal>shmem_startup_hook</literal> shortly after it attaches to shared
+      memory.  Note that add-ins should still acquire
+      <function>AddinShmemInitLock</function> within this hook, as shown in the
+      example above.
+     </para>
+
+     <para>
+      An example of a <literal>shmem_request_hook</literal> and
+      <literal>shmem_startup_hook</literal> can be found in
+      <filename>contrib/pg_stat_statements/pg_stat_statements.c</filename> in
+      the <productname>PostgreSQL</productname> source tree.
+     </para>
+    </sect3>
+   </sect2>
+
+   <sect2 id="xfunc-addin-lwlocks">
+    <title>LWLocks</title>
+
+    <sect3 id="xfunc-addin-lwlocks-at-startup">
+     <title>Requesting LWLocks at Startup</title>
+
+     <para>
+      Add-ins can reserve LWLocks on server startup.  Like with shared memory,
+      the add-in's shared library must be preloaded by specifying it in
+      <xref linkend="guc-shared-preload-libraries"/><indexterm><primary>shared_preload_libraries</primary></indexterm>,
+      and the shared library should register a
+      <literal>shmem_request_hook</literal> in its
+      <function>_PG_init</function> function.  This
+      <literal>shmem_request_hook</literal> can reserve LWLocks by calling:
 <programlisting>
 void RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks)
 </programlisting>
-     from your <literal>shmem_request_hook</literal>.  This will ensure that an array of
-     <literal>num_lwlocks</literal> LWLocks is available under the name
-     <literal>tranche_name</literal>.  Use <function>GetNamedLWLockTranche</function>
-     to get a pointer to this array.
-    </para>
-    <para>
-     An example of a <literal>shmem_request_hook</literal> can be found in
-     <filename>contrib/pg_stat_statements/pg_stat_statements.c</filename> in the
-     <productname>PostgreSQL</productname> source tree.
-    </para>
-    <para>
-     There is another, more flexible method of obtaining LWLocks. First,
-     allocate a <literal>tranche_id</literal> from a shared counter by
-     calling:
+      This ensures that an array of <literal>num_lwlocks</literal> LWLocks is
+      available under the name <literal>tranche_name</literal>.  A pointer to
+      this array can be obtained by calling:
 <programlisting>
-int LWLockNewTrancheId(void)
+LWLockPadded *GetNamedLWLockTranche(const char *tranche_name)
 </programlisting>
-     Next, each individual process using the <literal>tranche_id</literal>
-     should associate it with a <literal>tranche_name</literal> by calling:
+     </para>
+    </sect3>
+
+    <sect3 id="xfunc-addin-lwlocks-after-startup">
+     <title>Requesting LWLocks After Startup</title>
+
+     <para>
+      There is another, more flexible method of obtaining LWLocks that can be
+      done after server startup and outside a
+      <literal>shmem_request_hook</literal>.  To do so, first allocate a
+      <literal>tranche_id</literal> by calling:
 <programlisting>
-void LWLockRegisterTranche(int tranche_id, const char *tranche_name)
+int LWLockNewTrancheId(void)
 </programlisting>
-     It is also required to call <function>LWLockInitialize</function> once
-     per LWLock, passing the <literal>tranche_id</literal> as argument:
+      Next, initialize each LWLock, passing the new
+      <literal>tranche_id</literal> as an argument:
 <programlisting>
 void LWLockInitialize(LWLock *lock, int tranche_id)
 </programlisting>
-     A complete usage example of <function>LWLockNewTrancheId</function>,
-     <function>LWLockInitialize</function> and
-     <function>LWLockRegisterTranche</function> can be found in
-     <filename>contrib/pg_prewarm/autoprewarm.c</filename> in the
-     <productname>PostgreSQL</productname> source tree.
-    </para>
-    <para>
-     To avoid possible race-conditions, each backend should use the LWLock
-     <function>AddinShmemInitLock</function> when connecting to and initializing
-     its allocation of shared memory, as shown here:
-<programlisting>
-static mystruct *ptr = NULL;
+      Similar to shared memory, each backend should ensure that only one
+      process allocates a new <literal>tranche_id</literal> and initializes
+      each new LWLock.  One way to do this is to only call these functions in
+      your shared memory initialization code with the
+      <function>AddinShmemInitLock</function> held exclusively.
+     </para>
 
-if (!ptr)
-{
-        bool    found;
-
-        LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
-        ptr = ShmemInitStruct("my struct name", size, &amp;found);
-        if (!found)
-        {
-                initialize contents of shmem area;
-                acquire any requested LWLocks using:
-                ptr->locks = GetNamedLWLockTranche("my tranche name");
-        }
-        LWLockRelease(AddinShmemInitLock);
-}
+     <para>
+      Finally, each backend using the <literal>tranche_id</literal> should
+      associate it with a <literal>tranche_name</literal> by calling:
+<programlisting>
+void LWLockRegisterTranche(int tranche_id, const char *tranche_name)
 </programlisting>
-    </para>
-    <para>
-     It is convenient to use <literal>shmem_startup_hook</literal> which allows
-     placing all the code responsible for initializing shared memory in one
-     place. When using <literal>shmem_startup_hook</literal> the extension
-     still needs to acquire <function>AddinShmemInitLock</function> in order to
-     work properly on all the supported platforms.
-    </para>
+     </para>
+
+     <para>
+      A complete usage example of <function>LWLockNewTrancheId</function>,
+      <function>LWLockInitialize</function>, and
+      <function>LWLockRegisterTranche</function> can be found in
+      <filename>contrib/pg_prewarm/autoprewarm.c</filename> in the
+      <productname>PostgreSQL</productname> source tree.
+     </para>
+    </sect3>
    </sect2>
 
    <sect2 id="xfunc-addin-wait-events">
-- 
2.25.1

---

From: Aleksander Alekseev <[email-masked]>
Date: Sat, 13 Jan 2024 13:49:08 +0300

Hi,

Thanks for the updated patch.

> >       after it attaches to shared memory.

That's much better, thanks.

I think the patch could use another pair of eyes, ideally from a
native English speaker. But if no one will express any objections for
a while I suggest merging it.

-- 
Best regards,
Aleksander Alekseev

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 13 Jan 2024 15:28:15 -0600

> a while I suggest merging it.

Great.  I've attached a v3 with a couple of fixes suggested in the other
thread [0].  I'll wait a little while longer in case anyone else wants to
take a look.

[0] https://postgr.es/m/ZaF6UpYImGqVIhVp%40toroid.org

>From b56931edc4488a7376b27ba0e5519cc3a61b4899 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Jan 2024 21:55:25 -0600
Subject: [PATCH v3 1/1] reorganize shared memory and lwlocks documentation

---

index 89116ae74c..82e1dadcca 100644

-    <para>
+      Each backend should obtain a pointer to the reserved shared memory by
+      Add-ins can reserve LWLocks on server startup.  As with shared memory,
 
-if (!ptr)
-{
-        bool    found;
-

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 16 Jan 2024 10:02:15 +0530

> thread [0].  I'll wait a little while longer in case anyone else wants to

The v3 patch looks good to me except for a nitpick: the input
parameter for RequestAddinShmemSpace is 'Size' not 'int'

 <programlisting>

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

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 16 Jan 2024 08:20:19 -0600

>  void RequestAddinShmemSpace(int size)

Hah, I think this mistake is nearly old enough to vote (e0dece1, 5f78aa5).
Good catch.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 16 Jan 2024 09:52:52 -0600

> Hah, I think this mistake is nearly old enough to vote (e0dece1, 5f78aa5).

I fixed this in v4.

>From 402eaf87776fb6a9d212da66947f47c63bd53f2a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Jan 2024 21:55:25 -0600
Subject: [PATCH v4 1/1] reorganize shared memory and lwlocks documentation

---
 doc/src/sgml/xfunc.sgml | 184 +++++++++++++++++++++++++---------------
 1 file changed, 115 insertions(+), 69 deletions(-)

index 89116ae74c..ede2a5dea6 100644

-    <para>
-void RequestAddinShmemSpace(int size)
+void RequestAddinShmemSpace(Size size)
 
-if (!ptr)
-{
-        bool    found;
-

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 17 Jan 2024 06:48:37 +0530

> I fixed this in v4.

LGTM.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 19 Jan 2024 11:22:06 -0600

> LGTM.

Committed.  Thanks for reviewing!