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));