Re: Support to define custom wait events for extensions

On 2023-06-16 01:13, Tristan Partin wrote:
> We had this on our list of things to do at Neon, so it is a nice
> surprise that you brought up an initial patchset :). It was also my
> first time looking up the word tranche.

What a coincidence! I came up with the idea when I used Neon with
postgres_fdw. As a Neon user, I also feel the feature is important.

Same as you. Thanks to Michael and Drouvot, I got to know the word 
tranche
and the related existing code.

> From 59a118402e5e59685fb9e0fb086872e25a405736 Mon Sep 17 00:00:00 2001
> From: Masahiro Ikeda <masahiro.ikeda.us@hco.ntt.co.jp>
> Date: Thu, 15 Jun 2023 12:57:29 +0900
> Subject: [PATCH 2/3] Support to define custom wait events for 
> extensions.
> 
>> Currently, only one PG_WAIT_EXTENSION event can be used as a
>> wait event for extensions. Therefore, in environments with multiple
>> extensions are installed, it could take time to identify bottlenecks.
> 
> "extensions are installed" should be "extensions installed".
> 
>> +#define NUM_BUILDIN_WAIT_EVENT_EXTENSION       \
>> +       (WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED - 
>> WAIT_EVENT_EXTENSION)
> 
> Should that be NUM_BUILTIN_WAIT_EVENT_EXTENSION?

Thanks for your comments.
Yes, I'll fix it.

>> +               NamedExtensionWaitEventTrancheRequestArray = 
>> (NamedExtensionWaitEventTrancheRequest *)
>> +                       MemoryContextAlloc(TopMemoryContext,
>> +                                                          
>> NamedExtensionWaitEventTrancheRequestsAllocated
>> +                                                          * 
>> sizeof(NamedExtensionWaitEventTrancheRequest));
> 
> I can't tell from reading other Postgres code when one should cast the
> return value of MemoryContextAlloc(). Seems unnecessary to me.

I referenced RequestNamedLWLockTranche() and it looks ok to me.

```
void
RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks)
        NamedLWLockTrancheRequestArray = (NamedLWLockTrancheRequest *)
            MemoryContextAlloc(TopMemoryContext,
                               NamedLWLockTrancheRequestsAllocated
                               * sizeof(NamedLWLockTrancheRequest));
```

>> +       if (NamedExtensionWaitEventTrancheRequestArray == NULL)
>> +       {
>> +               NamedExtensionWaitEventTrancheRequestsAllocated = 16;
>> +               NamedExtensionWaitEventTrancheRequestArray = 
>> (NamedExtensionWaitEventTrancheRequest *)
>> +                       MemoryContextAlloc(TopMemoryContext,
>> +                                                          
>> NamedExtensionWaitEventTrancheRequestsAllocated
>> +                                                          * 
>> sizeof(NamedExtensionWaitEventTrancheRequest));
>> +       }
>> +
>> +       if (NamedExtensionWaitEventTrancheRequests >= 
>> NamedExtensionWaitEventTrancheRequestsAllocated)
>> +       {
>> +               int                     i = 
>> pg_nextpower2_32(NamedExtensionWaitEventTrancheRequests + 1);
>> +
>> +               NamedExtensionWaitEventTrancheRequestArray = 
>> (NamedExtensionWaitEventTrancheRequest *)
>> +                       
>> repalloc(NamedExtensionWaitEventTrancheRequestArray,
>> +                                        i * 
>> sizeof(NamedExtensionWaitEventTrancheRequest));
>> +               NamedExtensionWaitEventTrancheRequestsAllocated = i;
>> +       }
> 
> Do you think this code would look better in an if/else if?

Same as above. I referenced RequestNamedLWLockTranche().
I don't know if it's a good idea, but it's better to refactor the
existing code separately from this patch.

But I plan to remove the code to focus implementing dynamic allocation
similar to LWLockNewTrancheId() and LWLockRegisterTranche() as
Michael's suggestion. I think it's good idea as a first step. Is it ok 
for you?

>> +               int                     i = 
>> pg_nextpower2_32(NamedExtensionWaitEventTrancheRequests + 1);
> 
> In the Postgres codebase, is an int always guaranteed to be at least 32
> bits? I feel like a fixed-width type would be better for tracking the
> length of the array, unless Postgres prefers the Size type.

Same as above.

>> +       Assert(strlen(tranche_name) + 1 <= NAMEDATALEN);
>> +       strlcpy(request->tranche_name, tranche_name, NAMEDATALEN);
> 
> A sizeof(request->tranche_name) would keep this code more in-sync if
> size of tranche_name were to ever change, though I see sizeof
> expressions in the codebase are not too common. Maybe just remove the 
> +1
> and make it less than rather than a less than or equal? Seems like it
> might be worth noting in the docs of the function that the event name
> has to be less than NAMEDATALEN, but maybe this is something extension
> authors are inherently aware of?

Same as above.

> ---
> 
> What's the Postgres policy on the following?
> 
> for (int i = 0; ...)
> for (i = 0; ...)
> You are using 2 different patterns in WaitEventShmemInit() and
> InitializeExtensionWaitEventTranches().

I didn't care it. I'll unify it.
Michael's replay is interesting.

>> +       /*
>> +        * Copy the info about any named tranches into shared memory 
>> (so that
>> +        * other processes can see it), and initialize the requested 
>> wait events.
>> +        */
>> +       if (NamedExtensionWaitEventTrancheRequests > 0)
> 
> Removing this if would allow one less indentation level. Nothing would
> have to change about the containing code either since the for loop will
> then not run

Thanks, but I plan to remove to focus implementing dynamic allocation.

>> +       ExtensionWaitEventCounter = (int *) ((char *) 
>> NamedExtensionWaitEventTrancheArray - sizeof(int));
> 
> From 65e25d4b27bbb6d0934872310c24ee19f89e9631 Mon Sep 17 00:00:00 2001
> From: Masahiro Ikeda <masahiro.ikeda.us@hco.ntt.co.jp>
> Date: Thu, 15 Jun 2023 13:16:00 +0900
> Subject: [PATCH 3/3] Add docs to define custom wait events
> 
>> +    <para>
>> +     wait events are reserved by calling:
>> +<programlisting>
>> +void RequestNamedExtensionWaitEventTranche(const char *tranche_name)
>> +</programlisting>
>> +     from your <literal>shmem_request_hook</literal>.  This will 
>> ensure that
>> +     wait event is available under the name 
>> <literal>tranche_name</literal>,
>> +     which the wait event type is <literal>Extension</literal>.
>> +     Use <function>GetNamedExtensionWaitEventTranche</function>
>> +     to get a wait event information.
>> +    </para>
>> +    <para>
>> +     To avoid possible race-conditions, each backend should use the 
>> LWLock
>> +     <function>AddinShmemInitLock</function> when connecting to and 
>> initializing
>> +     its allocation of shared memory, same as LWLocks reservations 
>> above.
>> +    </para>
> 
> Should "wait" be capitalized in the first sentence?

Yes, I'll fix it

> "This will ensure that wait event is available" should have an "a"
> before "wait".

Yes, I'll fix it

> Nice patch.

Thanks for your comments too.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION