Re: Support to define custom wait events for extensions

Hi,

On 2023-07-11 16:45:26 +0900, Michael Paquier wrote:
> +/* ----------
> + * Wait Events - Extension
> + *
> + * Use this category when the server process is waiting for some condition
> + * defined by an extension module.
> + *
> + * Extensions can define custom wait events. First, call
> + * WaitEventExtensionNewTranche() just once to obtain a new wait event
> + * tranche. The ID is allocated from a shared counter. Next, each
> + * individual process using the tranche should call
> + * WaitEventExtensionRegisterTranche() to associate that wait event with
> + * a name.

What does "tranche" mean here? For LWLocks it makes some sense, it's used for
a set of lwlocks, not an individual one. But here that doesn't really seem to
apply?

> + * It may seem strange that each process using the tranche must register it
> + * separately, but dynamic shared memory segments aren't guaranteed to be
> + * mapped at the same address in all coordinating backends, so storing the
> + * registration in the main shared memory segment wouldn't work for that case.
> + */

I don't really see how this applies to wait events? There's no pointers
here...

> +typedef enum
> +{
> + WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
> + WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED
> +} WaitEventExtension;
> +
> +extern void WaitEventExtensionShmemInit(void);
> +extern Size WaitEventExtensionShmemSize(void);
> +
> +extern uint32 WaitEventExtensionNewTranche(void);
> +extern void WaitEventExtensionRegisterTranche(uint32 wait_event_info,

> -slock_t *ShmemLock; /* spinlock for shared memory and LWLock
> +slock_t *ShmemLock; /* spinlock for shared memory, LWLock
> + * allocation, and named extension wait event
> * allocation */

I'm doubtful that it's a good idea to reuse the spinlock further. Given that
the patch adds WaitEventExtensionShmemInit(), why not just have a lock in
there?

> +/*
> + * Allocate a new event ID and return the wait event info.
> + */
> +uint32
> +WaitEventExtensionNewTranche(void)
> +{
> + uint16 eventId;
> +
> + SpinLockAcquire(ShmemLock);
> + eventId = (*WaitEventExtensionCounter)++;
> + SpinLockRelease(ShmemLock);
> +
> + return PG_WAIT_EXTENSION | eventId;
> +}

It seems quite possible to run out space in WaitEventExtensionCounter, so this
should error out in that case.

> +/*
> + * Register a dynamic tranche name in the lookup table of the current process.
> + *
> + * This routine will save a pointer to the wait event tranche name passed
> + * as an argument, so the name should be allocated in a backend-lifetime context
> + * (shared memory, TopMemoryContext, static constant, or similar).
> + *
> + * The "wait_event_name" will be user-visible as a wait event name, so try to
> + * use a name that fits the style for those.
> + */
> +void
> +WaitEventExtensionRegisterTranche(uint32 wait_event_info,
> + const char *wait_event_name)
> +{
> + uint16 eventId;
> +
> + /* Check wait event class. */
> + Assert((wait_event_info & 0xFF000000) == PG_WAIT_EXTENSION);
> +
> + eventId = wait_event_info & 0x0000FFFF;
> +
> + /* This should only be called for user-defined tranches. */
> + if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
> + return;

Why not assert out in that case then?

> +/*
> + * Return the name of an Extension wait event ID.
> + */
> +static const char *
> +GetWaitEventExtensionIdentifier(uint16 eventId)
> +{
> + /* Build-in tranche? */

*built

> + if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
> + return "Extension";
> +
> + /*
> + * It's an extension tranche, so look in WaitEventExtensionTrancheNames[].
> + * However, it's possible that the tranche has never been registered by
> + * calling WaitEventExtensionRegisterTranche() in the current process, in
> + * which case give up and return "Extension".
> + */
> + eventId -= NUM_BUILTIN_WAIT_EVENT_EXTENSION;
> +
> + if (eventId >= WaitEventExtensionTrancheNamesAllocated ||
> + WaitEventExtensionTrancheNames[eventId] == NULL)
> + return "Extension";

I'd return something different here, otherwise something that's effectively a
bug is not distinguishable from the built

> +++ b/src/test/modules/test_custom_wait_events/t/001_basic.pl
> @@ -0,0 +1,34 @@
> +# Copyright (c) 2023, PostgreSQL Global Development Group
> +
> +use strict;
> +use warnings;
> +
> +use PostgreSQL::Test::Cluster;
> +use PostgreSQL::Test::Utils;
> +use Test::More;
> +
> +my $node = PostgreSQL::Test::Cluster->new('main');
> +
> +$node->init;
> +$node->append_conf(
> + 'postgresql.conf',
> + "shared_preload_libraries = 'test_custom_wait_events'"
> +);
> +$node->start;

I think this should also test registering a wait event later.

> @@ -0,0 +1,188 @@
> +/*--------------------------------------------------------------------------
> + *
> + * test_custom_wait_events.c
> + * Code for testing custom wait events
> + *
> + * This code initializes a custom wait event in shmem_request_hook() and
> + * provide a function to launch a background worker waiting forever
> + * with the custom wait event.

Isn't this vast overkill? Why not just have a simple C function that waits
with a custom wait event, until cancelled? That'd maybe 1/10th of the LOC.

Greetings,

Andres Freund