Re: Support to define custom wait events for extensions

On Fri, Jul 28, 2023 at 12:43:36PM +0530, Bharath Rupireddy wrote:
> 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".

You are right that I am making things inconsistent here. Having a
behavior close to the existing LWLock and use "extension" when the
event cannot be found makes the most sense. I have been a bit
confused with the wording though of this part of the docs, though, as
LWLocks don't directly use the custom wait event APIs.

> 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.

Fine by me, added one.

> 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?

Yes, see answer to point 1.

> 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?

An extension is an Add-in, and may be loaded, but it is possible to
have modules that just need to be LOAD'ed (with command line or just
shared_preload_libraries) so an add-in may not always be an extension.
I am not completely sure if add-ins is the best term, but it covers
both, and that's consistent with the existing docs. Perhaps the same
area of the docs should be refreshed, but that looks like a separate
patch for me. For now, I'd rather use a consistent term, and this one
sounds OK to me.

[1]: https://www.postgresql.org/docs/devel/extend-extensions.html.

> 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.

Hmm, good point on consistency here, especially to use an upper-case
character for the first characters of waitEventExtensionCounter..
Err.. 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.

Okay by me that it may be a better idea to use ereport(ERROR) in the
long run, so changed this way. I have introduced a
WAIT_EVENT_CLASS_MASK and a WAIT_EVENT_ID_MASK as we now use
0xFF000000 and 0x0000FFFF in three places of this file. This should
just be a patch on its own.

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

Thanks, missed that.

> 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.

Yes, this was mentioned upthread. I am not completely sure yet how
much we need to do for this interface, but surely it would be faster
to have a Multiple() interface that returns an array made of N numbers
requested (rather than a rank of them). For now, I'd rather just aim
for simplicity for the basics.

> 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.

No, it cannot because we need the custom wait event string to be
loaded in the same connection as the one querying pg_stat_activity.
A different thing that can be done here is to use background_psql()
with a query_until(), though I am not sure that this is worth doing
here.

> 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.

Right, this is misleading. This could be something like a "worker_spi
State", for instance. I have switched to this term.

Attached is a new version.
--
Michael