Re: Support to define custom wait events for extensions

On Fri, Jul 28, 2023 at 6:36 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> I have spent more time polishing the docs and the comments.  This v9
> looks in a rather committable shape now with docs, tests and core
> routines in place.

Thanks. Here are some comments on v9 patch:

1.
-     so an <literal>LWLock</literal> wait event might be reported as
-     just <quote><literal>extension</literal></quote> rather than the
-     extension-assigned name.
+     if the extension's library is not loaded; so a custom wait event might
+     be reported as just <quote><literal>???</literal></quote>
+     rather than the custom name assigned.

Trying to understand why '???' is any better than 'extension' for a
registered custom wait event of an unloaded extension?

PS: Looked at other instances where '???' is being used for
representing an unknown "thing".

2. Have an example of how a custom wait event is displayed in the
example in the docs "Here is an example of how wait events can be
viewed:". We can use the worker_spi wait event type there.

3.
-     so an <literal>LWLock</literal> wait event might be reported as
-     just <quote><literal>extension</literal></quote> rather than the
-     extension-assigned name.

+     <xref linkend="wait-event-lwlock-table"/>. In some cases, the name
+     assigned by an extension will not be available in all server processes
+     if the extension's library is not loaded; so a custom wait event might
+     be reported as just <quote><literal>???</literal></quote>

Are we missing to explicitly say what wait event will be reported for
an LWLock when the extension library is not loaded?

4.
+     Add-ins can define custom wait events under the wait event type

I see a few instances of Add-ins/add-in in xfunc.sgml. Isn't it better
to use the word extension given that glossary defines what an
extension is https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-EXTENSION?

5.
+} WaitEventExtensionCounter;
+
+/* pointer to the shared memory */
+static WaitEventExtensionCounter *waitEventExtensionCounter;

How about naming the structure variable as
WaitEventExtensionCounterData and pointer as
WaitEventExtensionCounter? This keeps all the static variable names
consistent WaitEventExtensionNames, WaitEventExtensionNamesAllocated
and WaitEventExtensionCounter.

6.
+    /* Check the wait event class. */
+    Assert((wait_event_info & 0xFF000000) == PG_WAIT_EXTENSION);
+
+    /* This should only be called for user-defined wait event. */
+    Assert(eventId >= NUM_BUILTIN_WAIT_EVENT_EXTENSION);

Maybe, we must turn the above asserts into ereport(ERROR) to protect
against an extension sending in an unregistered wait_event_info?
Especially, the first Assert((wait_event_info & 0xFF000000) ==
PG_WAIT_EXTENSION); checks that the passed in wait_event_info is
previously returned by WaitEventExtensionNew. IMO, these assertions
better fit for errors.

7.
+ * Extensions can define their own wait events in this categiry.  First,
Typo - s/categiry/category

8.
+ First,
+ * they should call WaitEventExtensionNew() to get one or more wait event
+ * IDs that are allocated from a shared counter.

Can WaitEventExtensionNew() be WaitEventExtensionNew(int num_ids, int
*result) to get the required number of wait event IDs in one call
similar to RequestNamedLWLockTranche? Currently, an extension needs to
call WaitEventExtensionNew() N number of times to get N wait event
IDs. Maybe the existing WaitEventExtensionNew() is good, but just a
thought.

9.
# The expected result is a special pattern here with a newline coming from the
# first query where the shared memory state is set.
$result = $node->poll_query_until(
    'postgres',
    qq[SELECT worker_spi_init(); SELECT wait_event FROM
pg_stat_activity WHERE backend_type ~ 'worker_spi';],
    qq[
worker_spi_main]);

This test doesn't have to be that complex with the result being a
special pattern, SELECT worker_spi_init(); can just be within a
separate safe_psql.

10.
+    wsstate = ShmemInitStruct("custom_wait_event",

Name the shared memory just "worker_spi" to make it generic and
extensible. Essentially, it is a woker_spi shared memory area part of
it is for custom wait event id.

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