Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
Date: Wed, 19 Jul 2023 10:57:39 -0500
Message-Id: <CU69THFPG2CN.MK16RYW510YD@gonk>
To: "Masahiro Ikeda" <ikedamsh@oss.nttdata.com>, "Michael Paquier"
 <michael@paquier.xyz>, <andres@anarazel.de>
From: "Tristan Partin" <tristan@neon.tech>
Cc: <pgsql-hackers@lists.postgresql.org>
Subject: Re: Support to define custom wait events for extensions
References: <CTE7JPUIMMBL.2YLT1IWHPUDZ8@gonk>
 <a9eae9af-3834-ceed-33bc-d36c914fef53@oss.nttdata.com>
 <135feae32cd28f9fb555fe46acb939a0@oss.nttdata.com>
 <f3c10c343e6a6080a6b57f0ed25d9e52@oss.nttdata.com>
 <c18393a428412ede7acd2355c27b3c1f@oss.nttdata.com>
 <ZK0IluceH+Hd5h22@paquier.xyz>
 <eaf82a85c0ef1b55dc3b651d3f7b867a@oss.nttdata.com>
 <ZK5oZ6wzVrgk5wuT@paquier.xyz> <ZK9BdDI6KMxmiG+5@paquier.xyz>
 <cb7d5257d1b1a25aa405c219732aaf8c@oss.nttdata.com>
 <ZK9iLbi2l2rrELfV@paquier.xyz>
 <97449ae1a3b2189934890f6d78b18216@oss.nttdata.com>
 <ca567bd4076dfc7e8b0d39afe5b0cc22@oss.nttdata.com>
In-Reply-To: <ca567bd4076dfc7e8b0d39afe5b0cc22@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/CU69THFPG2CN.MK16RYW510YD%40gonk>
Precedence: bulk

Thanks for continuing to work on this patchset. I only have
prose-related comments.

> To support custom wait events, it add 2 APIs to define new wait events
> for extensions dynamically.

Remove the "it" here.

> The APIs are
> * WaitEventExtensionNew()
> * WaitEventExtensionRegisterName()
=20
> These are similar to the existing LWLockNewTrancheId() and
> LWLockRegisterTranche().

This sentence seems like it could be removed given the API names have
changed during the development of this patch.

> First, extensions should call WaitEventExtensionNew() to get one
> or more new wait event, which IDs are allocated from a shared
> counter.  Next, each individual process can use the wait event with
> WaitEventExtensionRegisterName() to associate that a wait event
> string to the associated name.

This portion of the commit message is a copy-paste of the function
comment. Whatever you do in the function comment (which I commented on
below), just do here as well.

> +     so an wait event might be reported as just <quote><literal>extensio=
n</literal></quote>
> +     rather than the extension-assigned name.

s/an/a

> +   <sect2 id=3D"xfunc-addin-wait-events">
> +    <title>Custom Wait Events for Add-ins</title>

This would be the second use of "Add-ins" ever, according to my search.
Should this be "Extensions" instead?

> +    <para>
> +    Add-ins can define custom wait events that the wait event type is

s/that/where

> +    <literal>Extension</literal>.
> +    </para>
> +    <para>
> +    First, add-ins should get new one or more wait events by calling:

"one or more" doesn't seem to make sense grammatically here.

> +<programlisting>
> +    uint32 WaitEventExtensionNew(void)
> +</programlisting>
> +    Next, each individual process can use them to associate that

Remove "that".

> +    a wait event string to the associated name by calling:
> +<programlisting>
> +    void WaitEventExtensionRegisterName(uint32 wait_event_info, const ch=
ar *wait_event_name);
> +</programlisting>
> +    An example can be found in
> +    <filename>src/test/modules/test_custom_wait_events/test_custom_wait_=
events.c</filename>
> +    in the PostgreSQL source tree.
> +    </para>
> +   </sect2>

> + * Register a dynamic wait event name for extension in the lookup table
> + * of the current process.

Inserting an "an" before "extension" would make this read better.

> +/*
> + * Return the name of an wait event ID for extension.
> + */

s/an/a

> +       /*
> +        * It's an user-defined wait event, so look in WaitEventExtension=
Names[].
> +        * However, it's possible that the name has never been registered=
 by
> +        * calling WaitEventExtensionRegisterName() in the current proces=
s, in
> +        * which case give up and return "extension".
> +        */

s/an/a

"extension" seems very similar to "Extension". Instead of returning a
string here, could we just error? This seems like a programmer error on
the part of the extension author.

> + * Extensions can define their wait events. First, extensions should cal=
l
> + * WaitEventExtensionNew() to get one or more wait events, which IDs are
> + * allocated from a shared counter.  Next, each individual process can u=
se
> + * them with WaitEventExtensionRegisterName() to associate that a wait
> + * event string to the associated name.

An "own" before "wait events" in the first sentence would increase
clarity. "where" instead of "which" in the next sentence. Remove "that"
after "associate" in the third sentence.

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