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