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(dot)ikeda(dot)us(at)hco(dot)ntt(dot)co(dot)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(dot)ikeda(dot)us(at)hco(dot)ntt(dot)co(dot)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