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