MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Fri, 16 Jun 2023 11:49:45 +0900
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Tristan Partin <tristan@neon.tech>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Support to define custom wait events for extensions
In-Reply-To: <CTDCV6CIJAKR.TQOTL86K9628@gonk>
References: <b9f5411acda0cf15c8fbb767702ff43e@oss.nttdata.com>
 <CTDCV6CIJAKR.TQOTL86K9628@gonk>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <e4cef41ee00f791c2264f871dc6b5b19@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/e4cef41ee00f791c2264f871dc6b5b19%40oss.nttdata.com>
Precedence: bulk

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