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