public inbox for [email protected]  

help / color / mirror / Atom feed

From: Masahiro Ikeda <[email protected]>
Subject: [PATCH 2/3] Support to define custom wait events for extensions.
Date: Thu, 15 Jun 2023 12:57:29 +0900

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

view thread (53+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected]
  Subject: Re: [PATCH 2/3] Support to define custom wait events for extensions.
  In-Reply-To: <no-message-id-1856910@localhost>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox