Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Thu, 15 Jun 2023 11:13:57 -0500
Subject: Re: Support to define custom wait events for extensions
To: "Masahiro Ikeda" <ikedamsh@oss.nttdata.com>,
 <pgsql-hackers@lists.postgresql.org>
From: "Tristan Partin" <tristan@neon.tech>
Message-Id: <CTDCV6CIJAKR.TQOTL86K9628@gonk>
References: <b9f5411acda0cf15c8fbb767702ff43e@oss.nttdata.com>
In-Reply-To: <b9f5411acda0cf15c8fbb767702ff43e@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/CTDCV6CIJAKR.TQOTL86K9628%40gonk>
Precedence: bulk

We had this on our list of things to do at Neon, so it is a nice
surprise that you brought up an initial patchset :). It was also my
first time looking up the word tranche.

From 59a118402e5e59685fb9e0fb086872e25a405736 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <masahiro.ikeda.us@hco.ntt.co.jp>
Date: Thu, 15 Jun 2023 12:57:29 +0900
Subject: [PATCH 2/3] Support to define custom wait events for extensions.

> Currently, only one PG_WAIT_EXTENSION event can be used as a
> wait event for extensions. Therefore, in environments with multiple
> extensions are installed, it could take time to identify bottlenecks.

"extensions are installed" should be "extensions installed".

> +#define NUM_BUILDIN_WAIT_EVENT_EXTENSION       \
> +       (WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED - WAIT_EVENT_EXTENSION)

Should that be NUM_BUILTIN_WAIT_EVENT_EXTENSION?

> +               NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> +                       MemoryContextAlloc(TopMemoryContext,
> +                                                          NamedExtension=
WaitEventTrancheRequestsAllocated
> +                                                          * sizeof(Named=
ExtensionWaitEventTrancheRequest));

I can't tell from reading other Postgres code when one should cast the
return value of MemoryContextAlloc(). Seems unnecessary to me.

> +       if (NamedExtensionWaitEventTrancheRequestArray =3D=3D NULL)
> +       {
> +               NamedExtensionWaitEventTrancheRequestsAllocated =3D 16;
> +               NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> +                       MemoryContextAlloc(TopMemoryContext,
> +                                                          NamedExtension=
WaitEventTrancheRequestsAllocated
> +                                                          * sizeof(Named=
ExtensionWaitEventTrancheRequest));
> +       }
> +
> +       if (NamedExtensionWaitEventTrancheRequests >=3D NamedExtensionWai=
tEventTrancheRequestsAllocated)
> +       {
> +               int                     i =3D pg_nextpower2_32(NamedExten=
sionWaitEventTrancheRequests + 1);
> +
> +               NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> +                       repalloc(NamedExtensionWaitEventTrancheRequestArr=
ay,
> +                                        i * sizeof(NamedExtensionWaitEve=
ntTrancheRequest));
> +               NamedExtensionWaitEventTrancheRequestsAllocated =3D i;
> +       }

Do you think this code would look better in an if/else if?

> +               int                     i =3D pg_nextpower2_32(NamedExten=
sionWaitEventTrancheRequests + 1);

In the Postgres codebase, is an int always guaranteed to be at least 32
bits? I feel like a fixed-width type would be better for tracking the
length of the array, unless Postgres prefers the Size type.

> +       Assert(strlen(tranche_name) + 1 <=3D NAMEDATALEN);
> +       strlcpy(request->tranche_name, tranche_name, NAMEDATALEN);

A sizeof(request->tranche_name) would keep this code more in-sync if
size of tranche_name were to ever change, though I see sizeof
expressions in the codebase are not too common. Maybe just remove the +1
and make it less than rather than a less than or equal? Seems like it
might be worth noting in the docs of the function that the event name
has to be less than NAMEDATALEN, but maybe this is something extension
authors are inherently aware of?

---

What's the Postgres policy on the following?

for (int i =3D 0; ...)
for (i =3D 0; ...)

You are using 2 different patterns in WaitEventShmemInit() and
InitializeExtensionWaitEventTranches().

> +       /*
> +        * Copy the info about any named tranches into shared memory (so =
that
> +        * other processes can see it), and initialize the requested wait=
 events.
> +        */
> +       if (NamedExtensionWaitEventTrancheRequests > 0)

Removing this if would allow one less indentation level. Nothing would
have to change about the containing code either since the for loop will
then not run

> +       ExtensionWaitEventCounter =3D (int *) ((char *) NamedExtensionWai=
tEventTrancheArray - sizeof(int));

From 65e25d4b27bbb6d0934872310c24ee19f89e9631 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <masahiro.ikeda.us@hco.ntt.co.jp>
Date: Thu, 15 Jun 2023 13:16:00 +0900
Subject: [PATCH 3/3] Add docs to define custom wait events

> +    <para>
> +     wait events are reserved by calling:
> +<programlisting>
> +void RequestNamedExtensionWaitEventTranche(const char *tranche_name)
> +</programlisting>
> +     from your <literal>shmem_request_hook</literal>.  This will ensure =
that
> +     wait event is available under the name <literal>tranche_name</liter=
al>,
> +     which the wait event type is <literal>Extension</literal>.
> +     Use <function>GetNamedExtensionWaitEventTranche</function>
> +     to get a wait event information.
> +    </para>
> +    <para>
> +     To avoid possible race-conditions, each backend should use the LWLo=
ck
> +     <function>AddinShmemInitLock</function> when connecting to and init=
ializing
> +     its allocation of shared memory, same as LWLocks reservations above=
.
> +    </para>

Should "wait" be capitalized in the first sentence?

"This will ensure that wait event is available" should have an "a"
before "wait".

Nice patch.

--=20
Tristan Partin
Neon (https://neon.tech)