Re: Support to define custom wait events for extensions

On 2023-07-12 09:36, Andres Freund wrote:
> 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?

Thanks for useful comments.
OK, I will change to WaitEventExtensionNewId() and
WaitEventExtensionRegisterName().

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

Yes, I'll fix the comments.

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

OK, I'll create a new spinlock for the purpose.

>> +/*
>> + * 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.

OK, I'll do so.

>> +/*
>> + * 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?

OK, I'll add the assertion for eventID.

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

I will fix it.

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

It is a good idea. It would be good to name it "unknown wait event", the
same as
pgstat_get_wait_activity(), etc.

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

I see. I wasn't expecting that.

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

Are you saying that processing in the background worker is overkill?

If my understanding is correct, it is difficult to implement the test
without a background worker for this purpose. This is because the test
will execute commands serially, while a function waiting is executed in
a backend process, it is not possible to connect to another backend and
check the wait events on pg_stat_activity view.

Please let me know if my understanding is wrong.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION