Thread: Support to define custom wait events for extensions
Hi,
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 which
extension is the bottleneck.
So, I'd like to support new APIs to define custom wait events
for extensions. It's discussed in [1].
I made patches to realize it. Although I have some TODOs,
I want to know your feedbacks. Please feel free to comment.
# Implementation of new APIs
I implemented 2 new APIs for extensions.
* RequestNamedExtensionWaitEventTranche()
* GetNamedExtensionWaitEventTranche()
Extensions can request custom wait events by calling
RequestNamedExtensionWaitEventTranche(). After that, use
GetNamedExtensionWaitEventTranche() to get the wait event information.
The APIs usage example by extensions are following.
```
shmem_request_hook = shmem_request;
shmem_startup_hook = shmem_startup;
static void
shmem_request(void)
{
/* request a custom wait event */
RequestNamedExtensionWaitEventTranche("custom_wait_event");
}
static void
shmem_startup(void)
{
/* get the wait event information */
custom_wait_event =
GetNamedExtensionWaitEventTranche("custom_wait_event");
}
void
extension_funtion()
{
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
10L * 1000,
custom_wait_event); /* notify core with custom wait event */
ResetLatch(MyLatch);
}
```
# Implementation overview
I referenced the implementation of
RequestNamedLWLockTranche()/GetNamedLWLockTranche().
(0001-Support-to-define-custom-wait-events-for-extensions.patch)
Extensions calls RequestNamedExtensionWaitEventTranche() in
shmem_request_hook() to request wait events to be used by each
extension.
In the core, the requested wait events are dynamically registered in
shared
memory. The extension then obtains the wait event information with
GetNamedExtensionWaitEventTranche() and uses the value to notify the
core
that it is waiting.
When a string representing of the wait event is requested,
it returns the name defined by calling
RequestNamedExtensionWaitEventTranche().
# PoC extension
I created the PoC extension and you can use it, as shown here:
(0002-Add-a-extension-to-test-custom-wait-event.patch)
1. start PostgreSQL with the following configuration
shared_preload_libraries = 'inject_wait_event'
2. check wait events periodically
psql-1=# SELECT query, wait_event_type, wait_event FROM pg_stat_activity
WHERE backend_type = 'client backend' AND pid != pg_backend_pid() ;
psql-1=# \watch
3. execute a function to inject a wait event
psql-2=# CREATE EXTENSION inject_wait_event;
psql-2=# SELECT inject_wait_event();
4. check the custom wait event
You can see the following results of psql-1.
(..snip..)
query | wait_event_type | wait_event
-----------------------------+-----------------+------------
SELECT inject_wait_event(); | Extension | Extension
(1 row)
(..snip..)
(...about 10 seconds later ..)
query | wait_event_type | wait_event
-----------------------------+-----------------+-------------------
SELECT inject_wait_event(); | Extension | custom_wait_event
# requested wait event by the extension!
(1 row)
(..snip..)
# TODOs
* tests on windows (since I tested on Ubuntu 20.04 only)
* add custom wait events for existing contrib modules (ex. postgres_fdw)
* add regression code (but, it seems to be difficult)
* others? (Please let me know)
[1]
https://www.postgresql.org/message-id/81290a48-b25c-22a5-31a6-3feff5864fe3%40gmail.com
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Attachment
On Thu, Jun 15, 2023 at 03:06:01PM +0900, Masahiro Ikeda wrote: > 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 which > extension is the bottleneck. Thanks for taking the time to implement a patch to do that. > I want to know your feedbacks. Please feel free to comment. I think that's been cruelly missed. > In the core, the requested wait events are dynamically registered in shared > memory. The extension then obtains the wait event information with > GetNamedExtensionWaitEventTranche() and uses the value to notify the core > that it is waiting. > > When a string representing of the wait event is requested, > it returns the name defined by calling > RequestNamedExtensionWaitEventTranche(). So this implements the equivalent of RequestNamedLWLockTranche() followed by GetNamedLWLockTranche() to get the wait event number, which can be used only during postmaster startup. Do you think that we could focus on implementing something more flexible instead, that can be used dynamically as well as statically? That would be similar to LWLockNewTrancheId() and LWLockRegisterTranche(), actually, where we would get one or more tranche IDs, then do initialization actions in shmem based on the tranche ID(s). > 4. check the custom wait event > You can see the following results of psql-1. > > query | wait_event_type | wait_event > -----------------------------+-----------------+------------------- > SELECT inject_wait_event(); | Extension | custom_wait_event # > requested wait event by the extension! > (1 row) > > (..snip..) A problem with this approach is that it is expensive as a test. Do we really need one? There are three places that set PG_WAIT_EXTENSION in src/test/modules/, more in /contrib, and there are modules like pg_stat_statements that could gain from events for I/O operations, for example. > # TODOs > > * tests on windows (since I tested on Ubuntu 20.04 only) > * add custom wait events for existing contrib modules (ex. postgres_fdw) > * add regression code (but, it seems to be difficult) > * others? (Please let me know) Hmm. You would need to maintain a state in a rather stable manner, and SQL queries can make that difficult in the TAP tests as the wait event information is reset each time a query finishes. One area where I think this gets easier is with a background worker loaded with shared_preload_libraries that has a configurable naptime. Looking at what's available in the tree, the TAP tests of pg_prewarm could use one test on pg_stat_activity with a custom wait event name (pg_prewarm.autoprewarm_interval is 0 hence the bgworker waits infinitely). Note that in this case, you would need to be careful of the case where the wait event is loaded dynamically, but like LWLocks this should be able to work as well? -- Michael
Attachment
Hi, On 6/15/23 10:00 AM, Michael Paquier wrote: > On Thu, Jun 15, 2023 at 03:06:01PM +0900, Masahiro Ikeda wrote: >> 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 which >> extension is the bottleneck. > > Thanks for taking the time to implement a patch to do that. +1 thanks for it! > >> I want to know your feedbacks. Please feel free to comment. > > I think that's been cruelly missed. Yeah, that would clearly help to diagnose which extension(s) is/are causing the waits (if any). I did not look at the code yet (will do) but just wanted to chime in to support the idea. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
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.
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?
> + 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.
> + 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?
> + 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.
> + 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?
---
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().
> + /*
> + * 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
> + 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?
"This will ensure that wait event is available" should have an "a"
before "wait".
Nice patch.
--
Tristan Partin
Neon (https://neon.tech)
On Thu, Jun 15, 2023 at 11:13:57AM -0500, Tristan Partin wrote: > 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(). C99 style is OK since v12, so the style of the patch is fine. The older style has no urgent need to change, either. One argument to let the code as-is is that it could generate backpatching conflicts, while it does not hurt as it stands. This also means that bug fixes that need to be applied down to 12 would be able to use C99 declarations freely without some of the buildfarm animals running REL_11_STABLE complaining. I have fallen into this trap recently, actually. See dbd25dd. -- Michael
Attachment
Thanks for replying and your kind advice!
On 2023-06-15 17:00, Michael Paquier wrote:
> On Thu, Jun 15, 2023 at 03:06:01PM +0900, Masahiro Ikeda wrote:
>> In the core, the requested wait events are dynamically registered in
>> shared
>> memory. The extension then obtains the wait event information with
>> GetNamedExtensionWaitEventTranche() and uses the value to notify the
>> core
>> that it is waiting.
>>
>> When a string representing of the wait event is requested,
>> it returns the name defined by calling
>> RequestNamedExtensionWaitEventTranche().
>
> So this implements the equivalent of RequestNamedLWLockTranche()
> followed by GetNamedLWLockTranche() to get the wait event number,
> which can be used only during postmaster startup. Do you think that
> we could focus on implementing something more flexible instead, that
> can be used dynamically as well as statically? That would be similar
> to LWLockNewTrancheId() and LWLockRegisterTranche(), actually, where
> we would get one or more tranche IDs, then do initialization actions
> in shmem based on the tranche ID(s).
OK, I agree. I'll make a patch to only support
ExtensionWaitEventNewTrancheId() and ExtensionWaitEventRegisterTranche()
similar to LWLockNewTrancheId() and LWLockRegisterTranche().
>> 4. check the custom wait event
>> You can see the following results of psql-1.
>>
>> query | wait_event_type | wait_event
>> -----------------------------+-----------------+-------------------
>> SELECT inject_wait_event(); | Extension | custom_wait_event
>> #
>> requested wait event by the extension!
>> (1 row)
>>
>> (..snip..)
>
> A problem with this approach is that it is expensive as a test. Do we
> really need one? There are three places that set PG_WAIT_EXTENSION in
> src/test/modules/, more in /contrib, and there are modules like
> pg_stat_statements that could gain from events for I/O operations, for
> example.
Yes. Since it's hard to test, I thought the PoC extension
should not be committed. But, I couldn't figure out the best
way to test yet.
>> # TODOs
>>
>> * tests on windows (since I tested on Ubuntu 20.04 only)
>> * add custom wait events for existing contrib modules (ex.
>> postgres_fdw)
>> * add regression code (but, it seems to be difficult)
>> * others? (Please let me know)
>
> Hmm. You would need to maintain a state in a rather stable manner,
> and SQL queries can make that difficult in the TAP tests as the wait
> event information is reset each time a query finishes. One area where
> I think this gets easier is with a background worker loaded with
> shared_preload_libraries that has a configurable naptime. Looking at
> what's available in the tree, the TAP tests of pg_prewarm could use
> one test on pg_stat_activity with a custom wait event name
> (pg_prewarm.autoprewarm_interval is 0 hence the bgworker waits
> infinitely). Note that in this case, you would need to be careful of
> the case where the wait event is loaded dynamically, but like LWLocks
> this should be able to work as well?
Thanks for your advice!
I tried to query on pg_stat_activity to check the background worker
invoked by pg_prewarm. But, I found that pg_stat_activity doesn't show
it although I may be missing something...
So, I tried to implement TAP tests. But I have a problem with it.
I couldn't find the way to check the status of another backend
while the another backend wait with custom wait events.
```
# TAP test I've implemented.
# wait forever with custom wait events in session1
$session1->query_safe("SELECT test_custom_wait_events_wait()");
# I want to check the wait event from another backend process
# But, the following code is never reached because the above
# query is waiting forever.
$session2->poll_query_until('postgres',
qq[SELECT
(SELECT count(*) FROM pg_stat_activity
WHERE query ~ '^SELECT test_custom_wait_events_wait'
AND wait_event_type = 'Extension'
AND wait_event = 'custom_wait_event'
) > 0;]);
```
If I'm missing something or you have any idea,
please let me know.
Now, I plan to
* find out more the existing tests to check wait events and locks
(though I have already checked a little, but I couldn't find it)
* find another way to check wait event of the background worker invoked
by extension
* look up the reason why pg_stat_activity doesn't show the background
worker
* find a way to implement async queries in TAP tests
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
On 2023-06-15 22:21, Drouvot, Bertrand wrote: > Hi, > > On 6/15/23 10:00 AM, Michael Paquier wrote: >> On Thu, Jun 15, 2023 at 03:06:01PM +0900, Masahiro Ikeda wrote: >>> 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 which >>> extension is the bottleneck. >> >> Thanks for taking the time to implement a patch to do that. > > +1 thanks for it! > >> >>> I want to know your feedbacks. Please feel free to comment. >> >> I think that's been cruelly missed. > > Yeah, that would clearly help to diagnose which extension(s) is/are > causing the waits (if any). > > I did not look at the code yet (will do) but just wanted to chime in > to support the idea. Great! Thanks. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
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
On Fri, Jun 16, 2023 at 11:14:05AM +0900, Masahiro Ikeda wrote: > I tried to query on pg_stat_activity to check the background worker > invoked by pg_prewarm. But, I found that pg_stat_activity doesn't show > it although I may be missing something... > > So, I tried to implement TAP tests. But I have a problem with it. > I couldn't find the way to check the status of another backend > while the another backend wait with custom wait events. Hmm. Right. It seems to me that BGWORKER_BACKEND_DATABASE_CONNECTION is required in this case, with BackgroundWorkerInitializeConnection() to connect to a database (or not, like the logical replication launcher if only access to shared catalogs is wanted). I have missed that the leader process of pg_prewarm does not use that, because it has no need to connect to a database, but its workers do. So it is not going to show up in pg_stat_activity. -- Michael
Attachment
On 2023-06-16 16:46, Michael Paquier wrote: > On Fri, Jun 16, 2023 at 11:14:05AM +0900, Masahiro Ikeda wrote: >> I tried to query on pg_stat_activity to check the background worker >> invoked by pg_prewarm. But, I found that pg_stat_activity doesn't show >> it although I may be missing something... >> >> So, I tried to implement TAP tests. But I have a problem with it. >> I couldn't find the way to check the status of another backend >> while the another backend wait with custom wait events. > > Hmm. Right. It seems to me that BGWORKER_BACKEND_DATABASE_CONNECTION > is required in this case, with BackgroundWorkerInitializeConnection() > to connect to a database (or not, like the logical replication > launcher if only access to shared catalogs is wanted). > > I have missed that the leader process of pg_prewarm does not use that, > because it has no need to connect to a database, but its workers do. > So it is not going to show up in pg_stat_activity. Yes. Thanks to your advice, I understood that BGWORKER_BACKEND_DATABASE_CONNECTION is the reason. I could make the TAP test that invokes a background worker waiting forever and checks its custom wait event in pg_stat_activity. So, I'll make patches including test codes next week. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
I will take a look at your V2 when it is ready! I will also pass along that this is wanted by Neon customers :). -- Tristan Partin Neon (https://neon.tech)
On 2023/06/17 1:16, Tristan Partin wrote: > I will take a look at your V2 when it is ready! I will also pass along > that this is wanted by Neon customers :). Thanks! Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Hi, I updated the patches. The main changes are * to support only dynamic wait event allocation * to add a regression test I appreciate any feedback. The followings are TODO items. * to check that meson.build works since I tested with old command `make` now * to make documents * to add custom wait events for existing contrib modules (ex. postgres_fdw) Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
On 2023-06-20 18:26, Masahiro Ikeda wrote: > The followings are TODO items. > * to check that meson.build works since I tested with old command > `make` now I test with meson and I updated the patches to work with it. My test procedure is the following. ``` export builddir=/mnt/tmp/build export prefix=/mnt/tmp/master # setup meson setup $builddir --prefix=$prefix -Ddebug=true -Dcassert=true -Dtap_tests=enabled # build and install with src/test/modules ninja -C $builddir install install-test-files # test meson test -v -C $builddir meson test -v -C $builddir --suite test_custom_wait_events # run the test only ``` Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
Hi, I updated the patches to handle the warning mentioned by PostgreSQL Patch Tester, and removed unnecessary spaces. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
On Fri, Jun 23, 2023 at 05:56:26PM +0900, Masahiro Ikeda wrote: > I updated the patches to handle the warning mentioned > by PostgreSQL Patch Tester, and removed unnecessary spaces. I have begun hacking on that, and the API layer inspired from the LWLocks is sound. I have been playing with it in my own extensions and it is nice to be able to plug in custom wait events into pg_stat_activity, particularly for bgworkers. Finally. The patch needed a rebase after the recent commit that introduced the automatic generation of docs and code for wait events. It requires two tweaks in generate-wait_event_types.pl, feel free to double-check them. Some of the new structures and routine names don't quite reflect the fact that we have wait events for extensions, so I have taken a stab at that. Note that the test module test_custom_wait_events would crash if attempting to launch a worker when not loaded in shared_preload_libraries, so we'd better have some protection in wait_worker_launch() (this function should be renamed as well). Attached is a rebased patch that I have begun tweaking here and there. For now, the patch is moved as waiting on author. I have merged the test module with the main patch for the moment, for simplicity. A split is straight-forward as the code paths touched are different. Another and *very* important thing is that we are going to require some documentation in xfunc.sgml to explain how to use these routines and what to expect from them. Ikeda-san, could you write some? You could look at the part about shmem and LWLock to get some inspiration. -- Michael
Attachment
> From bf06b8100cb747031959fe81a2d19baabc4838cf Mon Sep 17 00:00:00 2001
> From: Masahiro Ikeda <masahiro.ikeda.us@hco.ntt.co.jp>
> Date: Fri, 16 Jun 2023 11:53:29 +0900
> Subject: [PATCH 1/2] Support custom wait events for extensions.
> + * This is indexed by event ID minus NUM_BUILTIN_WAIT_EVENT_EXTENSION, and
> + * stores the names of all dynamically-created event ID known to the current
> + * process. Any unused entries in the array will contain NULL.
The second ID should be plural.
> + /* If necessary, create or enlarge array. */
> + if (eventId >= ExtensionWaitEventTrancheNamesAllocated)
> + {
> + int newalloc;
> +
> + newalloc = pg_nextpower2_32(Max(8, eventId + 1));
Given the context of our last conversation, I assume this code was
copied from somewhere else. Since this is new code, I think it would
make more sense if newalloc was a uint16 or size_t.
From what I undersatnd, Neon differs from upstream in some way related
to this patch. I am trying to ascertain how that is. I hope to provide
more feedback when I learn more about it.
--
Tristan Partin
Neon (https://neon.tech)
On Tue, Jul 11, 2023 at 12:39:52PM -0500, Tristan Partin wrote: > Given the context of our last conversation, I assume this code was > copied from somewhere else. Since this is new code, I think it would > make more sense if newalloc was a uint16 or size_t. This style comes from LWLockRegisterTranche() in lwlock.c. Do you think that it would be more adapted to change that to pg_nextpower2_size_t() with a Size? We could do that for the existing code on HEAD as an improvement. > From what I understand, Neon differs from upstream in some way related > to this patch. I am trying to ascertain how that is. I hope to provide > more feedback when I learn more about it. Hmm, okay, that would nice to hear about to make sure that the approach taken on this thread is able to cover what you are looking for. So you mean that Neon has been using something similar to register wait events in the backend? Last time I looked at the Neon repo, I did not get the impression that there was a custom patch for Postgres in this area. All the in-core code paths using WAIT_EVENT_EXTENSION would gain from the APIs added here, FWIW. -- Michael
Attachment
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?
> + * 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...
> +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?
> +/*
> + * 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.
> +/*
> + * 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?
> +/*
> + * Return the name of an Extension wait event ID.
> + */
> +static const char *
> +GetWaitEventExtensionIdentifier(uint16 eventId)
> +{
> + /* Build-in tranche? */
*built
> + 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
> +++ 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.
> @@ -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.
Greetings,
Andres Freund
On Tue, Jul 11, 2023 at 05:36:47PM -0700, Andres Freund wrote: > On 2023-07-11 16:45:26 +0900, Michael Paquier wrote: >> +$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. Yup, agreed that the coverage is not sufficient. > > @@ -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. Hmm. You mean in the shape of a TAP test where a backend registers a wait event by itself in a SQL function that waits for a certain amount of time with a WaitLatch(), then we use a second poll_query_until() that checks if the a wait event is stored in pg_stat_activity? With something like what's done at the end of 001_stream_rep.pl, that should be stable, I guess.. -- Michael
Attachment
On 2023-07-11 16:45, Michael Paquier wrote:
> On Fri, Jun 23, 2023 at 05:56:26PM +0900, Masahiro Ikeda wrote:
>> I updated the patches to handle the warning mentioned
>> by PostgreSQL Patch Tester, and removed unnecessary spaces.
>
> I have begun hacking on that, and the API layer inspired from the
> LWLocks is sound. I have been playing with it in my own extensions
> and it is nice to be able to plug in custom wait events into
> pg_stat_activity, particularly for bgworkers. Finally.
Great!
> The patch needed a rebase after the recent commit that introduced the
> automatic generation of docs and code for wait events. It requires
> two tweaks in generate-wait_event_types.pl, feel free to double-check
> them.
Thanks for rebasing. I confirmed it works with the current master.
I know this is a little off-topic from what we're talking about here,
but I'm curious about generate-wait_event_types.pl.
# generate-wait_event_types.pl
- # An exception is required for LWLock and Lock as these don't require
- # any C and header files generated.
+ # An exception is required for Extension, LWLock and Lock as these
don't
+ # require any C and header files generated.
die "wait event names must start with 'WAIT_EVENT_'"
if ( $trimmedwaiteventname eq $waiteventenumname
+ && $waiteventenumname !~ /^Extension/
&& $waiteventenumname !~ /^LWLock/
&& $waiteventenumname !~ /^Lock/);
In my understanding, the first column of the row for WaitEventExtension
in
wait_event_names.txt can be any value and the above code should not die.
But if I use the following input, it falls on the last line.
# wait_event_names.txt
Section: ClassName - WaitEventExtension
WAIT_EVENT_EXTENSION "Extension" "Waiting in an extension."
Extension "Extension" "Waiting in an extension."
EXTENSION "Extension" "Waiting in an extension."
If the behavior is unexpected, we need to change the current code.
I have created a patch for the areas that I felt needed to be changed.
- 0001-change-the-die-condition-in-generate-wait_event_type.patch
(In addition to the above, "$continue = ",\n";" doesn't appear to be
necessary.)
> Some of the new structures and routine names don't quite reflect the
> fact that we have wait events for extensions, so I have taken a stab
> at that.
Sorry. I confirmed the change.
> Note that the test module test_custom_wait_events would crash if
> attempting to launch a worker when not loaded in
> shared_preload_libraries, so we'd better have some protection in
> wait_worker_launch() (this function should be renamed as well).
OK, I will handle it. Since Andres gave me other comments for the
test module, I'll think about what is best.
> Attached is a rebased patch that I have begun tweaking here and
> there. For now, the patch is moved as waiting on author. I have
> merged the test module with the main patch for the moment, for
> simplicity. A split is straight-forward as the code paths touched are
> different.
>
> Another and *very* important thing is that we are going to require
> some documentation in xfunc.sgml to explain how to use these routines
> and what to expect from them. Ikeda-san, could you write some? You
> could look at the part about shmem and LWLock to get some
> inspiration.
OK. Yes, I planned to write documentation.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Attachment
On 2023-07-12 02:39, Tristan Partin wrote:
>> From bf06b8100cb747031959fe81a2d19baabc4838cf Mon Sep 17 00:00:00 2001
>> From: Masahiro Ikeda <masahiro.ikeda.us@hco.ntt.co.jp>
>> Date: Fri, 16 Jun 2023 11:53:29 +0900
>> Subject: [PATCH 1/2] Support custom wait events for extensions.
>
>> + * This is indexed by event ID minus
>> NUM_BUILTIN_WAIT_EVENT_EXTENSION, and
>> + * stores the names of all dynamically-created event ID known to the
>> current
>> + * process. Any unused entries in the array will contain NULL.
>
> The second ID should be plural.
Thanks for reviewing. Yes, I'll fix it.
>> + /* If necessary, create or enlarge array. */
>> + if (eventId >= ExtensionWaitEventTrancheNamesAllocated)
>> + {
>> + int newalloc;
>> +
>> + newalloc = pg_nextpower2_32(Max(8, eventId + 1));
>
> Given the context of our last conversation, I assume this code was
> copied from somewhere else. Since this is new code, I think it would
> make more sense if newalloc was a uint16 or size_t.
As Michael-san said, I used LWLockRegisterTranche() as a reference.
I think it is a good idea to fix the current master. I'll modify the
above code accordingly.
> From what I undersatnd, Neon differs from upstream in some way related
> to this patch. I am trying to ascertain how that is. I hope to provide
> more feedback when I learn more about it.
Oh, it was unexpected for me. Thanks for researching the reason.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
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
On Wed, Jul 12, 2023 at 04:52:38PM +0900, Masahiro Ikeda wrote:
> In my understanding, the first column of the row for WaitEventExtension in
> wait_event_names.txt can be any value and the above code should not die.
> But if I use the following input, it falls on the last line.
>
> # wait_event_names.txt
> Section: ClassName - WaitEventExtension
>
> WAIT_EVENT_EXTENSION "Extension" "Waiting in an extension."
> Extension "Extension" "Waiting in an extension."
> EXTENSION "Extension" "Waiting in an extension."
>
> If the behavior is unexpected, we need to change the current code.
> I have created a patch for the areas that I felt needed to be changed.
> - 0001-change-the-die-condition-in-generate-wait_event_type.patch
> (In addition to the above, "$continue = ",\n";" doesn't appear to be
> necessary.)
die "wait event names must start with 'WAIT_EVENT_'"
if ( $trimmedwaiteventname eq $waiteventenumname
- && $waiteventenumname !~ /^LWLock/
- && $waiteventenumname !~ /^Lock/);
- $continue = ",\n";
+ && $waitclassname !~ /^WaitEventLWLock$/
+ && $waitclassname !~ /^WaitEventLock$/);
Indeed, this looks wrong as-is. $waiteventenumname refers to the
names of the enum elements, so we could just apply a filter based on
the class names in full. The second check in for the generation of
the c/h files uses the class names.
--
Michael
Attachment
On Wed, Jul 12, 2023 at 05:46:31PM +0900, Michael Paquier wrote: > On Wed, Jul 12, 2023 at 04:52:38PM +0900, Masahiro Ikeda wrote: >> If the behavior is unexpected, we need to change the current code. >> I have created a patch for the areas that I felt needed to be changed. >> - 0001-change-the-die-condition-in-generate-wait_event_type.patch >> (In addition to the above, "$continue = ",\n";" doesn't appear to be >> necessary.) > > die "wait event names must start with 'WAIT_EVENT_'" > if ( $trimmedwaiteventname eq $waiteventenumname > - && $waiteventenumname !~ /^LWLock/ > - && $waiteventenumname !~ /^Lock/); > - $continue = ",\n"; > + && $waitclassname !~ /^WaitEventLWLock$/ > + && $waitclassname !~ /^WaitEventLock$/); > > Indeed, this looks wrong as-is. $waiteventenumname refers to the > names of the enum elements, so we could just apply a filter based on > the class names in full. The second check in for the generation of > the c/h files uses the class names. At the end, I have gone with an event simpler way and removed the checks for LWLock and Lock as their hardcoded values marked as DOCONLY satisfy this check. The second check when generating the C and header code has also been simplified a bit to use an exact match with the class name. -- Michael
Attachment
On 2023-07-13 09:12, Michael Paquier wrote: > On Wed, Jul 12, 2023 at 05:46:31PM +0900, Michael Paquier wrote: >> On Wed, Jul 12, 2023 at 04:52:38PM +0900, Masahiro Ikeda wrote: >>> If the behavior is unexpected, we need to change the current code. >>> I have created a patch for the areas that I felt needed to be >>> changed. >>> - 0001-change-the-die-condition-in-generate-wait_event_type.patch >>> (In addition to the above, "$continue = ",\n";" doesn't appear to be >>> necessary.) >> >> die "wait event names must start with 'WAIT_EVENT_'" >> if ( $trimmedwaiteventname eq $waiteventenumname >> - && $waiteventenumname !~ /^LWLock/ >> - && $waiteventenumname !~ /^Lock/); >> - $continue = ",\n"; >> + && $waitclassname !~ /^WaitEventLWLock$/ >> + && $waitclassname !~ /^WaitEventLock$/); >> >> Indeed, this looks wrong as-is. $waiteventenumname refers to the >> names of the enum elements, so we could just apply a filter based on >> the class names in full. The second check in for the generation of >> the c/h files uses the class names. > > At the end, I have gone with an event simpler way and removed the > checks for LWLock and Lock as their hardcoded values marked as DOCONLY > satisfy this check. The second check when generating the C and header > code has also been simplified a bit to use an exact match with the > class name. Thanks for your quick response. I'll rebase for the commit. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Thu, Jul 13, 2023 at 10:26:35AM +0900, Masahiro Ikeda wrote: > Thanks for your quick response. I'll rebase for the commit. Okay, thanks. I'll wait for the rebased version before moving on with the next review, then. -- Michael
Attachment
Hi, I updated the patches. * v6-0001-Support-custom-wait-events-for-extensions.patch The main diffs are * rebase it atop current HEAD * update docs to show users how to use the APIs * rename of functions and variables * fix typos * define a new spinlock in shared memory for this purpose * output an error if the number of wait event for extensions exceeds uint16 * show the wait event as "extension" if the custom wait event name is not registered, which is same as LWLock one. * add test cases which confirm it works if new wait events for extensions are defined in initialize phase and after phase. And add a boundary condition test. Please let me know if I forgot to handle something that you commented, and there are better idea. Note: I would like to change the wait event name of contrib modules for example postgres_fdw. But, I think it's better to do so after the APIs are committed. The example mentioned in docs should be updated to the contrib modules codes, not the test module. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
On 2023-07-19 12:52, Masahiro Ikeda wrote: > Hi, > > I updated the patches. > * v6-0001-Support-custom-wait-events-for-extensions.patch I updated the patch since the cfbot found a bug. * v7-0001-Support-custom-wait-events-for-extensions.patch Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
On Wed, Jul 19, 2023 at 12:52:10PM +0900, Masahiro Ikeda wrote: > I would like to change the wait event name of contrib modules for example > postgres_fdw. But, I think it's better to do so after the APIs are > committed. Agreed to do things one step at a time here. Let's focus on the core APIs and facilities first. -- Michael
Attachment
On Wed, Jul 19, 2023 at 11:49 AM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote: > > I updated the patch since the cfbot found a bug. > * v7-0001-Support-custom-wait-events-for-extensions.patch Thanks for working on this feature. +1. I've wanted this capability for a while because extensions have many different wait loops for different reasons, a single wait event type isn't enough. I think we don't need a separate test extension for demonstrating use of custom wait events, you can leverage the sample extension worker_spi because that's where extension authors look for while writing a new extension. Also, it simplifies your patch a lot. I don't mind adding a few things to worker_spi for the sake of demonstrating the use and testing for custom wait events. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Thanks for continuing to work on this patchset. I only have prose-related comments. > To support custom wait events, it add 2 APIs to define new wait events > for extensions dynamically. Remove the "it" here. > The APIs are > * WaitEventExtensionNew() > * WaitEventExtensionRegisterName() > These are similar to the existing LWLockNewTrancheId() and > LWLockRegisterTranche(). This sentence seems like it could be removed given the API names have changed during the development of this patch. > First, extensions should call WaitEventExtensionNew() to get one > or more new wait event, which IDs are allocated from a shared > counter. Next, each individual process can use the wait event with > WaitEventExtensionRegisterName() to associate that a wait event > string to the associated name. This portion of the commit message is a copy-paste of the function comment. Whatever you do in the function comment (which I commented on below), just do here as well. > + so an wait event might be reported as just <quote><literal>extension</literal></quote> > + rather than the extension-assigned name. s/an/a > + <sect2 id="xfunc-addin-wait-events"> > + <title>Custom Wait Events for Add-ins</title> This would be the second use of "Add-ins" ever, according to my search. Should this be "Extensions" instead? > + <para> > + Add-ins can define custom wait events that the wait event type is s/that/where > + <literal>Extension</literal>. > + </para> > + <para> > + First, add-ins should get new one or more wait events by calling: "one or more" doesn't seem to make sense grammatically here. > +<programlisting> > + uint32 WaitEventExtensionNew(void) > +</programlisting> > + Next, each individual process can use them to associate that Remove "that". > + a wait event string to the associated name by calling: > +<programlisting> > + void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name); > +</programlisting> > + An example can be found in > + <filename>src/test/modules/test_custom_wait_events/test_custom_wait_events.c</filename> > + in the PostgreSQL source tree. > + </para> > + </sect2> > + * Register a dynamic wait event name for extension in the lookup table > + * of the current process. Inserting an "an" before "extension" would make this read better. > +/* > + * Return the name of an wait event ID for extension. > + */ s/an/a > + /* > + * It's an user-defined wait event, so look in WaitEventExtensionNames[]. > + * However, it's possible that the name has never been registered by > + * calling WaitEventExtensionRegisterName() in the current process, in > + * which case give up and return "extension". > + */ s/an/a "extension" seems very similar to "Extension". Instead of returning a string here, could we just error? This seems like a programmer error on the part of the extension author. > + * Extensions can define their wait events. First, extensions should call > + * WaitEventExtensionNew() to get one or more wait events, which IDs are > + * allocated from a shared counter. Next, each individual process can use > + * them with WaitEventExtensionRegisterName() to associate that a wait > + * event string to the associated name. An "own" before "wait events" in the first sentence would increase clarity. "where" instead of "which" in the next sentence. Remove "that" after "associate" in the third sentence. -- Tristan Partin Neon (https://neon.tech)
On Tue Jul 11, 2023 at 6:29 PM CDT, Michael Paquier wrote: > On Tue, Jul 11, 2023 at 12:39:52PM -0500, Tristan Partin wrote: > > Given the context of our last conversation, I assume this code was > > copied from somewhere else. Since this is new code, I think it would > > make more sense if newalloc was a uint16 or size_t. > > This style comes from LWLockRegisterTranche() in lwlock.c. Do you > think that it would be more adapted to change that to > pg_nextpower2_size_t() with a Size? We could do that for the existing > code on HEAD as an improvement. Yes, I think that would be the most correct code. At the very least, using a uint32 instead of an int, would be preferrable. > > From what I understand, Neon differs from upstream in some way related > > to this patch. I am trying to ascertain how that is. I hope to provide > > more feedback when I learn more about it. > > Hmm, okay, that would nice to hear about to make sure that the > approach taken on this thread is able to cover what you are looking > for. So you mean that Neon has been using something similar to > register wait events in the backend? Last time I looked at the Neon > repo, I did not get the impression that there was a custom patch for > Postgres in this area. All the in-core code paths using > WAIT_EVENT_EXTENSION would gain from the APIs added here, FWIW. I did some investigation into the Neon fork[0], and couldn't find any commits that seemed related. Perhaps this is on our wishlist instead of something we already have implemented. I have CCed Heikki to talk some more about how this would fit in at Neon. [0]: https://github.com/neondatabase/postgres -- Tristan Partin Neon (https://neon.tech)
On Wed, Jul 19, 2023 at 11:16:34AM -0500, Tristan Partin wrote: > On Tue Jul 11, 2023 at 6:29 PM CDT, Michael Paquier wrote: >> This style comes from LWLockRegisterTranche() in lwlock.c. Do you >> think that it would be more adapted to change that to >> pg_nextpower2_size_t() with a Size? We could do that for the existing >> code on HEAD as an improvement. > > Yes, I think that would be the most correct code. At the very least, > using a uint32 instead of an int, would be preferrable. Would you like to send a patch on a new thread about that? >> Hmm, okay, that would nice to hear about to make sure that the >> approach taken on this thread is able to cover what you are looking >> for. So you mean that Neon has been using something similar to >> register wait events in the backend? Last time I looked at the Neon >> repo, I did not get the impression that there was a custom patch for >> Postgres in this area. All the in-core code paths using >> WAIT_EVENT_EXTENSION would gain from the APIs added here, FWIW. > > I did some investigation into the Neon fork[0], and couldn't find any > commits that seemed related. Perhaps this is on our wishlist instead of > something we already have implemented. I have CCed Heikki to talk some > more about how this would fit in at Neon. > > [0]: https://github.com/neondatabase/postgres Anybody with complex out-of-core extensions have wanted more monitoring capabilities for wait events without relying on the core backend. To be honest, I would not be surprised to see this stuff on more than one wishlist. -- Michael
Attachment
On Wed, Jul 19, 2023 at 10:57:39AM -0500, Tristan Partin wrote:
> > + <sect2 id="xfunc-addin-wait-events">
> > + <title>Custom Wait Events for Add-ins</title>
>
> This would be the second use of "Add-ins" ever, according to my search.
> Should this be "Extensions" instead?
Yes, I would think that just "Custom Wait Events" is enough here.
And I'd recommend to also use Shared Memory here. The case of
dynamically loaded things is possible, more advanced and can work, but
I am not sure we really need to do down to that as long as we mention
to use shared_preload_libraries.
I've rewritten the docs in their entirety, but honestly I still need
to spend more time polishing that.
Another part of the patch that has been itching me a lot are the
regression tests. I have spent some time today migrating the tests of
worker_spi to TAP for the sake of this thread, resulting in commit
320c311, and concluded that we need to care about three new cases:
- For custom wait events where the shmem state is not loaded, check
that we report the default of 'extension'.
- Check that it is possible to allocate and load a custom wait event
dynamically. Here, I have used a new SQL function in worker_spi,
called worker_spi_init(). That feels a bit hack-ish but for a test in
a template module that works great.
- Check that wait events loaded through shared_preload_libraries work
correctly.
The tests of worker_spi can take care easily of all these cases, once
a few things for the shmem handling are put in place for the dynamic
and preloading cases.
+Datum
+get_new_wait_event_info(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_UINT32(WaitEventExtensionNew());
+}
While looking at the previous patch and the test, I've noticed this
pattern. WaitEventExtensionNew() should not be called without holding
AddinShmemInitLock, or that opens the door to race conditions.
I am still mid-way through the review of the core APIs, but attached
is my current version in progress, labelled v8. I'll continue
tomorrow. I'm aware of some typos in the commit message of this
patch, and the dynamic bgworker launch is failing in the CI for
VS2019 (just too tired to finish debugging that today).
Thoughts are welcome.
--
Michael
Attachment
Hi, all.
Sorry for late reply.
> I am still mid-way through the review of the core APIs, but attached
> is my current version in progress, labelled v8. I'll continue
> tomorrow. I'm aware of some typos in the commit message of this
> patch, and the dynamic bgworker launch is failing in the CI for
> VS2019 (just too tired to finish debugging that today).
I suspect that I forgot to specify "volatile" to the variable
for the spinlock.
+/* dynamic allocation counter for custom wait events for extensions */
+typedef struct WaitEventExtensionCounter
+{
+ int nextId; /* next ID to assign */
+ slock_t mutex; /* protects the counter only */
+} WaitEventExtensionCounter;
+
+/* pointer to the shared memory */
+static WaitEventExtensionCounter * waitEventExtensionCounter;
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
On Thu, Jul 27, 2023 at 06:29:22PM +0900, Masahiro Ikeda wrote:
> I suspect that I forgot to specify "volatile" to the variable
> for the spinlock.
+ if (!IsUnderPostmaster)
+ {
+ /* Allocate space in shared memory. */
+ waitEventExtensionCounter = (WaitEventExtensionCounter *)
+ ShmemInitStruct("waitEventExtensionCounter", WaitEventExtensionShmemSize(), &found);
+ if (found)
+ return;
I think that your error is here. WaitEventExtensionShmemInit() is
forgetting to set the pointer to waitEventExtensionCounter for
processes where IsUnderPostmaster is true, which impacts things not
forked like in -DEXEC_BACKEND (the crash is reproducible on Linux with
-DEXEC_BACKEND in CFLAGS, as well). The correct thing to do is to
always call ShmemInitStruct, but only initialize the contents of the
shared memory area if ShmemInitStruct() has *not* found the shmem
contents.
WaitEventExtensionNew() could be easily incorrectly used, so I'd
rather add a LWLockHeldByMeInMode() on AddinShmemInitLock as safety
measure. Perhaps we should do the same for the LWLocks, subject for a
different thread..
+ int newalloc;
+
+ newalloc = pg_nextpower2_32(Max(8, eventId + 1));
This should be a uint32.
+ if (eventId >= WaitEventExtensionNamesAllocated ||
+ WaitEventExtensionNames[eventId] == NULL)
+ return "extension";
That's too close to the default of "Extension". It would be cleaner
to use "unknown", but we've been using "???" as well in many default
paths where an ID cannot be mapped to a string, so I would recommend
to just use that.
I have spent more time polishing the docs and the comments. This v9
looks in a rather committable shape now with docs, tests and core
routines in place.
--
Michael
Attachment
On Fri, Jul 28, 2023 at 6:36 AM Michael Paquier <michael@paquier.xyz> wrote: > > I have spent more time polishing the docs and the comments. This v9 > looks in a rather committable shape now with docs, tests and core > routines in place. Thanks. Here are some comments on v9 patch: 1. - so an <literal>LWLock</literal> wait event might be reported as - just <quote><literal>extension</literal></quote> rather than the - extension-assigned name. + if the extension's library is not loaded; so a custom wait event might + be reported as just <quote><literal>???</literal></quote> + rather than the custom name assigned. Trying to understand why '???' is any better than 'extension' for a registered custom wait event of an unloaded extension? PS: Looked at other instances where '???' is being used for representing an unknown "thing". 2. Have an example of how a custom wait event is displayed in the example in the docs "Here is an example of how wait events can be viewed:". We can use the worker_spi wait event type there. 3. - so an <literal>LWLock</literal> wait event might be reported as - just <quote><literal>extension</literal></quote> rather than the - extension-assigned name. + <xref linkend="wait-event-lwlock-table"/>. In some cases, the name + assigned by an extension will not be available in all server processes + if the extension's library is not loaded; so a custom wait event might + be reported as just <quote><literal>???</literal></quote> Are we missing to explicitly say what wait event will be reported for an LWLock when the extension library is not loaded? 4. + Add-ins can define custom wait events under the wait event type I see a few instances of Add-ins/add-in in xfunc.sgml. Isn't it better to use the word extension given that glossary defines what an extension is https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-EXTENSION? 5. +} WaitEventExtensionCounter; + +/* pointer to the shared memory */ +static WaitEventExtensionCounter *waitEventExtensionCounter; How about naming the structure variable as WaitEventExtensionCounterData and pointer as WaitEventExtensionCounter? This keeps all the static variable names consistent WaitEventExtensionNames, WaitEventExtensionNamesAllocated and WaitEventExtensionCounter. 6. + /* Check the wait event class. */ + Assert((wait_event_info & 0xFF000000) == PG_WAIT_EXTENSION); + + /* This should only be called for user-defined wait event. */ + Assert(eventId >= NUM_BUILTIN_WAIT_EVENT_EXTENSION); Maybe, we must turn the above asserts into ereport(ERROR) to protect against an extension sending in an unregistered wait_event_info? Especially, the first Assert((wait_event_info & 0xFF000000) == PG_WAIT_EXTENSION); checks that the passed in wait_event_info is previously returned by WaitEventExtensionNew. IMO, these assertions better fit for errors. 7. + * Extensions can define their own wait events in this categiry. First, Typo - s/categiry/category 8. + First, + * they should call WaitEventExtensionNew() to get one or more wait event + * IDs that are allocated from a shared counter. Can WaitEventExtensionNew() be WaitEventExtensionNew(int num_ids, int *result) to get the required number of wait event IDs in one call similar to RequestNamedLWLockTranche? Currently, an extension needs to call WaitEventExtensionNew() N number of times to get N wait event IDs. Maybe the existing WaitEventExtensionNew() is good, but just a thought. 9. # The expected result is a special pattern here with a newline coming from the # first query where the shared memory state is set. $result = $node->poll_query_until( 'postgres', qq[SELECT worker_spi_init(); SELECT wait_event FROM pg_stat_activity WHERE backend_type ~ 'worker_spi';], qq[ worker_spi_main]); This test doesn't have to be that complex with the result being a special pattern, SELECT worker_spi_init(); can just be within a separate safe_psql. 10. + wsstate = ShmemInitStruct("custom_wait_event", Name the shared memory just "worker_spi" to make it generic and extensible. Essentially, it is a woker_spi shared memory area part of it is for custom wait event id. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, Jul 28, 2023 at 12:43:36PM +0530, Bharath Rupireddy wrote: > 1. > - so an <literal>LWLock</literal> wait event might be reported as > - just <quote><literal>extension</literal></quote> rather than the > - extension-assigned name. > + if the extension's library is not loaded; so a custom wait event might > + be reported as just <quote><literal>???</literal></quote> > + rather than the custom name assigned. > > Trying to understand why '???' is any better than 'extension' for a > registered custom wait event of an unloaded extension? > > PS: Looked at other instances where '???' is being used for > representing an unknown "thing". You are right that I am making things inconsistent here. Having a behavior close to the existing LWLock and use "extension" when the event cannot be found makes the most sense. I have been a bit confused with the wording though of this part of the docs, though, as LWLocks don't directly use the custom wait event APIs. > 2. Have an example of how a custom wait event is displayed in the > example in the docs "Here is an example of how wait events can be > viewed:". We can use the worker_spi wait event type there. Fine by me, added one. > 3. > - so an <literal>LWLock</literal> wait event might be reported as > - just <quote><literal>extension</literal></quote> rather than the > - extension-assigned name. > > + <xref linkend="wait-event-lwlock-table"/>. In some cases, the name > + assigned by an extension will not be available in all server processes > + if the extension's library is not loaded; so a custom wait event might > + be reported as just <quote><literal>???</literal></quote> > > Are we missing to explicitly say what wait event will be reported for > an LWLock when the extension library is not loaded? Yes, see answer to point 1. > 4. > + Add-ins can define custom wait events under the wait event type > > I see a few instances of Add-ins/add-in in xfunc.sgml. Isn't it better > to use the word extension given that glossary defines what an > extension is https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-EXTENSION? An extension is an Add-in, and may be loaded, but it is possible to have modules that just need to be LOAD'ed (with command line or just shared_preload_libraries) so an add-in may not always be an extension. I am not completely sure if add-ins is the best term, but it covers both, and that's consistent with the existing docs. Perhaps the same area of the docs should be refreshed, but that looks like a separate patch for me. For now, I'd rather use a consistent term, and this one sounds OK to me. [1]: https://www.postgresql.org/docs/devel/extend-extensions.html. > 5. > +} WaitEventExtensionCounter; > + > +/* pointer to the shared memory */ > +static WaitEventExtensionCounter *waitEventExtensionCounter; > > How about naming the structure variable as > WaitEventExtensionCounterData and pointer as > WaitEventExtensionCounter? This keeps all the static variable names > consistent WaitEventExtensionNames, WaitEventExtensionNamesAllocated > and WaitEventExtensionCounter. Hmm, good point on consistency here, especially to use an upper-case character for the first characters of waitEventExtensionCounter.. Err.. WaitEventExtensionCounter. > 6. > + /* Check the wait event class. */ > + Assert((wait_event_info & 0xFF000000) == PG_WAIT_EXTENSION); > + > + /* This should only be called for user-defined wait event. */ > + Assert(eventId >= NUM_BUILTIN_WAIT_EVENT_EXTENSION); > > Maybe, we must turn the above asserts into ereport(ERROR) to protect > against an extension sending in an unregistered wait_event_info? > Especially, the first Assert((wait_event_info & 0xFF000000) == > PG_WAIT_EXTENSION); checks that the passed in wait_event_info is > previously returned by WaitEventExtensionNew. IMO, these assertions > better fit for errors. Okay by me that it may be a better idea to use ereport(ERROR) in the long run, so changed this way. I have introduced a WAIT_EVENT_CLASS_MASK and a WAIT_EVENT_ID_MASK as we now use 0xFF000000 and 0x0000FFFF in three places of this file. This should just be a patch on its own. > 7. > + * Extensions can define their own wait events in this categiry. First, > Typo - s/categiry/category Thanks, missed that. > 8. > + First, > + * they should call WaitEventExtensionNew() to get one or more wait event > + * IDs that are allocated from a shared counter. > > Can WaitEventExtensionNew() be WaitEventExtensionNew(int num_ids, int > *result) to get the required number of wait event IDs in one call > similar to RequestNamedLWLockTranche? Currently, an extension needs to > call WaitEventExtensionNew() N number of times to get N wait event > IDs. Maybe the existing WaitEventExtensionNew() is good, but just a > thought. Yes, this was mentioned upthread. I am not completely sure yet how much we need to do for this interface, but surely it would be faster to have a Multiple() interface that returns an array made of N numbers requested (rather than a rank of them). For now, I'd rather just aim for simplicity for the basics. > 9. > # The expected result is a special pattern here with a newline coming from the > # first query where the shared memory state is set. > $result = $node->poll_query_until( > 'postgres', > qq[SELECT worker_spi_init(); SELECT wait_event FROM > pg_stat_activity WHERE backend_type ~ 'worker_spi';], > qq[ > worker_spi_main]); > > This test doesn't have to be that complex with the result being a > special pattern, SELECT worker_spi_init(); can just be within a > separate safe_psql. No, it cannot because we need the custom wait event string to be loaded in the same connection as the one querying pg_stat_activity. A different thing that can be done here is to use background_psql() with a query_until(), though I am not sure that this is worth doing here. > 10. > + wsstate = ShmemInitStruct("custom_wait_event", > > Name the shared memory just "worker_spi" to make it generic and > extensible. Essentially, it is a woker_spi shared memory area part of > it is for custom wait event id. Right, this is misleading. This could be something like a "worker_spi State", for instance. I have switched to this term. Attached is a new version. -- Michael
Attachment
On Mon, Jul 31, 2023 at 6:40 AM Michael Paquier <michael@paquier.xyz> wrote: > > You are right that I am making things inconsistent here. Having a > behavior close to the existing LWLock and use "extension" when the > event cannot be found makes the most sense. I have been a bit > confused with the wording though of this part of the docs, though, as > LWLocks don't directly use the custom wait event APIs. + * calling WaitEventExtensionRegisterName() in the current process, in + * which case give up and return an unknown state. We're not giving up and returning an unknown state in the v10 patch unlike v9, no? This comment needs to change. > > 4. > > + Add-ins can define custom wait events under the wait event type > > > > I see a few instances of Add-ins/add-in in xfunc.sgml. Isn't it better > > to use the word extension given that glossary defines what an > > extension is https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-EXTENSION? > > An extension is an Add-in, and may be loaded, but it is possible to > have modules that just need to be LOAD'ed (with command line or just > shared_preload_libraries) so an add-in may not always be an extension. > I am not completely sure if add-ins is the best term, but it covers > both, and that's consistent with the existing docs. Perhaps the same > area of the docs should be refreshed, but that looks like a separate > patch for me. For now, I'd rather use a consistent term, and this one > sounds OK to me. > > [1]: https://www.postgresql.org/docs/devel/extend-extensions.html. The "external module" seems the right wording here. Use of "add-ins" is fine by me for this patch. > Okay by me that it may be a better idea to use ereport(ERROR) in the > long run, so changed this way. I have introduced a > WAIT_EVENT_CLASS_MASK and a WAIT_EVENT_ID_MASK as we now use > 0xFF000000 and 0x0000FFFF in three places of this file. This should > just be a patch on its own. Yeah, I don't mind these macros going along or before or after the custom wait events feature. > Yes, this was mentioned upthread. I am not completely sure yet how > much we need to do for this interface, but surely it would be faster > to have a Multiple() interface that returns an array made of N numbers > requested (rather than a rank of them). For now, I'd rather just aim > for simplicity for the basics. +1 to be simple for now. If any such requests come in future, I'm sure we can always get back to it. > > 9. > > # The expected result is a special pattern here with a newline coming from the > > # first query where the shared memory state is set. > > $result = $node->poll_query_until( > > 'postgres', > > qq[SELECT worker_spi_init(); SELECT wait_event FROM > > pg_stat_activity WHERE backend_type ~ 'worker_spi';], > > qq[ > > worker_spi_main]); > > > > This test doesn't have to be that complex with the result being a > > special pattern, SELECT worker_spi_init(); can just be within a > > separate safe_psql. > > No, it cannot because we need the custom wait event string to be > loaded in the same connection as the one querying pg_stat_activity. > A different thing that can be done here is to use background_psql() > with a query_until(), though I am not sure that this is worth doing > here. -1 to go to the background_psql and query_until route. However, enhancing the comment might help "we need the custom wait event string to be loaded in the same connection as .....". Having said this, I don't quite understand the point of having worker_spi_init() when we say clearly how to use shmem hooks and custom wait events. If its intention is to show loading of shared memory to a backend via a function, do we really need it in worker_spi? I don't mind removing it if it's not adding any significant value. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 2023-07-31 10:10, Michael Paquier wrote:
> Attached is a new version.
Thanks for all the improvements.
I have some comments for v10.
(1)
<note>
<para>
- Extensions can add <literal>LWLock</literal> types to the list
shown in
- <xref linkend="wait-event-lwlock-table"/>. In some cases, the
name
+ Extensions can add <literal>Extension</literal> and
+ <literal>LWLock</literal> types
+ to the list shown in <xref linkend="wait-event-extension-table"/>
and
+ <xref linkend="wait-event-lwlock-table"/>. In some cases, the name
assigned by an extension will not be available in all server
processes;
- so an <literal>LWLock</literal> wait event might be reported as
- just <quote><literal>extension</literal></quote> rather than the
+ so an <literal>LWLock</literal> or <literal>Extension</literal>
wait
+ event might be reported as just
+ <quote><literal>extension</literal></quote> rather than the
extension-assigned name.
</para>
</note>
I think the order in which they are mentioned should be matched. I mean
that
- so an <literal>LWLock</literal> or <literal>Extension</literal>
wait
+ so an <literal>Extension</literal> or <literal>LWLock</literal>
wait
(2)
/* This should only be called for user-defined wait event. */
if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid wait event ID %u", eventId));
I was just wondering if it should also check the eventId
that has been allocated though it needs to take the spinlock
and GetWaitEventExtensionIdentifier() doesn't take it into account.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
On Mon, Jul 31, 2023 at 12:07:40PM +0530, Bharath Rupireddy wrote: > We're not giving up and returning an unknown state in the v10 patch > unlike v9, no? This comment needs to change. Right. Better to be consistent with lwlock.c here. >> No, it cannot because we need the custom wait event string to be >> loaded in the same connection as the one querying pg_stat_activity. >> A different thing that can be done here is to use background_psql() >> with a query_until(), though I am not sure that this is worth doing >> here. > > -1 to go to the background_psql and query_until route. However, > enhancing the comment might help "we need the custom wait event string > to be loaded in the same connection as .....". Having said this, I > don't quite understand the point of having worker_spi_init() when we > say clearly how to use shmem hooks and custom wait events. If its > intention is to show loading of shared memory to a backend via a > function, do we really need it in worker_spi? I don't mind removing it > if it's not adding any significant value. It seems to initialize the state of the worker_spi, so attaching a function to this stuff makes sense to me, just for the sake of testing all that. -- Michael
Attachment
On Mon, Jul 31, 2023 at 03:53:27PM +0900, Masahiro Ikeda wrote:
> I think the order in which they are mentioned should be matched. I mean that
> - so an <literal>LWLock</literal> or <literal>Extension</literal> wait
> + so an <literal>Extension</literal> or <literal>LWLock</literal> wait
Makes sense.
> /* This should only be called for user-defined wait event. */
> if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
> ereport(ERROR,
> errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("invalid wait event ID %u", eventId));
>
> I was just wondering if it should also check the eventId
> that has been allocated though it needs to take the spinlock
> and GetWaitEventExtensionIdentifier() doesn't take it into account.
What kind of extra check do you have in mind? Once WAIT_EVENT_ID_MASK
is applied, we already know that we don't have something larger than
PG_UNIT16_MAX, or perhaps you want to cross-check this number with
what nextId holds in shared memory and that we don't have a number
between nextId and PG_UNIT16_MAX? I am not sure that we need to care
much about that this much in this code path, and I'd rather avoid
taking an extra time the spinlock just for a cross-check.
Attaching a v11 based on Bharath's feedback and yours, for now. I
have also applied the addition of the two masking variables in
wait_event.c separately with 7395a90.
--
Michael
Attachment
On 2023-07-31 16:28, Michael Paquier wrote:
> On Mon, Jul 31, 2023 at 03:53:27PM +0900, Masahiro Ikeda wrote:
>> /* This should only be called for user-defined wait event. */
>> if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
>> ereport(ERROR,
>> errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> errmsg("invalid wait event ID %u", eventId));
>>
>> I was just wondering if it should also check the eventId
>> that has been allocated though it needs to take the spinlock
>> and GetWaitEventExtensionIdentifier() doesn't take it into account.
>
> What kind of extra check do you have in mind? Once WAIT_EVENT_ID_MASK
> is applied, we already know that we don't have something larger than
> PG_UNIT16_MAX, or perhaps you want to cross-check this number with
> what nextId holds in shared memory and that we don't have a number
> between nextId and PG_UNIT16_MAX? I am not sure that we need to care
> much about that this much in this code path, and I'd rather avoid
> taking an extra time the spinlock just for a cross-check.
OK. I assumed to check that we don't have a number between nextId and
PG_UNIT16_MAX.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
On Mon, Jul 31, 2023 at 12:58 PM Michael Paquier <michael@paquier.xyz> wrote: > > > Attaching a v11 based on Bharath's feedback and yours, for now. I > have also applied the addition of the two masking variables in > wait_event.c separately with 7395a90. +uint32 WaitEventExtensionNew(void) +</programlisting> + Next, each process needs to associate the wait event allocated previously + to a user-facing custom string, which is something done by calling: +<programlisting> +void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name) +</programlisting> + An example can be found in <filename>src/test/modules/worker_spi</filename> + in the PostgreSQL source tree. + </para> Do you think it's worth adding a note here in the docs about an external module defining more than one custom wait event? A pseudo code if possible or just a note? Also, how about a XXX comment atop WaitEventExtensionNew and/or WaitEventExtensionRegisterName on the possibility of extending the functions to support allocation of more than one custom wait events? -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
At Mon, 31 Jul 2023 16:28:16 +0900, Michael Paquier <michael@paquier.xyz> wrote in > Attaching a v11 based on Bharath's feedback and yours, for now. I > have also applied the addition of the two masking variables in > wait_event.c separately with 7395a90. +/* + * Return the name of an wait event ID for extension. + */ +static const char * +GetWaitEventExtensionIdentifier(uint16 eventId) This looks inconsistent. Shouldn't it be GetWaitEventExtentionName()? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, Jul 31, 2023 at 01:37:49PM +0530, Bharath Rupireddy wrote: > Do you think it's worth adding a note here in the docs about an > external module defining more than one custom wait event? A pseudo > code if possible or just a note? Also, how about a XXX comment atop > WaitEventExtensionNew and/or WaitEventExtensionRegisterName on the > possibility of extending the functions to support allocation of more > than one custom wait events? I am not sure that any of that is necessary. Anyway, I have applied v11 to get the basics done. Now, I agree that a WaitEventExtensionMultiple() may come in handy, particularly for postgres_fdw that uses WAIT_EVENT_EXTENSION three times. -- Michael
Attachment
On Mon, Jul 31, 2023 at 05:10:21PM +0900, Kyotaro Horiguchi wrote: > +/* > + * Return the name of an wait event ID for extension. > + */ > +static const char * > +GetWaitEventExtensionIdentifier(uint16 eventId) > > This looks inconsistent. Shouldn't it be GetWaitEventExtentionName()? This is an inspiration from GetLWLockIdentifier(), which is kind of OK by me. If there is a consensus in changing that, fine by me, of course. -- Michael
Attachment
On Mon, Jul 31, 2023 at 3:54 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Jul 31, 2023 at 05:10:21PM +0900, Kyotaro Horiguchi wrote: > > +/* > > + * Return the name of an wait event ID for extension. > > + */ > > +static const char * > > +GetWaitEventExtensionIdentifier(uint16 eventId) > > > > This looks inconsistent. Shouldn't it be GetWaitEventExtentionName()? > > This is an inspiration from GetLWLockIdentifier(), which is kind of OK > by me. If there is a consensus in changing that, fine by me, of > course. +1 to GetWaitEventExtensionIdentifier for consistency with LWLock's counterpart. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 2023-07-31 19:22, Michael Paquier wrote: > I am not sure that any of that is necessary. Anyway, I have applied > v11 to get the basics done. Thanks for committing the main patch. In my understanding, the rest works are * to support WaitEventExtensionMultiple() * to replace WAIT_EVENT_EXTENSION to custom wait events Do someone already works for them? If not, I'll consider how to realize them. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Tue, Aug 01, 2023 at 11:51:35AM +0900, Masahiro Ikeda wrote: > Thanks for committing the main patch. > > In my understanding, the rest works are > * to support WaitEventExtensionMultiple() > * to replace WAIT_EVENT_EXTENSION to custom wait events > > Do someone already works for them? If not, I'll consider > how to realize them. Note that postgres_fdw and dblink use WAIT_EVENT_EXTENSION, but have no dependency to shared_preload_libraries. Perhaps these could just use a dynamic handling but that deserves a separate discussion because of the fact that they'd need shared memory without being able to request it. autoprewarm.c is much simpler. -- Michael
Attachment
Hi, On 2023-08-01 12:14:49 +0900, Michael Paquier wrote: > On Tue, Aug 01, 2023 at 11:51:35AM +0900, Masahiro Ikeda wrote: > > Thanks for committing the main patch. > > > > In my understanding, the rest works are > > * to support WaitEventExtensionMultiple() > > * to replace WAIT_EVENT_EXTENSION to custom wait events > > > > Do someone already works for them? If not, I'll consider > > how to realize them. > > Note that postgres_fdw and dblink use WAIT_EVENT_EXTENSION, but have > no dependency to shared_preload_libraries. Perhaps these could just > use a dynamic handling but that deserves a separate discussion because > of the fact that they'd need shared memory without being able to > request it. autoprewarm.c is much simpler. This is why the scheme as implemented doesn't really make sense to me. It'd be much easier to use if we had a shared hashtable with the identifiers than what's been merged now. In plenty of cases it's not realistic for an extension library to run in each backend, while still needing to wait for things. Greetings, Andres Freund
Hi,
On Tue, Aug 01, 2023 at 11:51:35AM +0900, Masahiro Ikeda wrote:
> Thanks for committing the main patch.
> Thanks for committing the main patch.
Latest head
Ubuntu 64 bits
gcc 13 64 bits
./configure --without-icu
make clean
make
In file included from ../../src/include/pgstat.h:20,
from controldata_utils.c:38:
../../src/include/utils/wait_event.h:56:9: error: redeclaration of enumerator ‘WAIT_EVENT_EXTENSION’
56 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
| ^~~~~~~~~~~~~~~~~~~~
In file included from ../../src/include/utils/wait_event.h:29,
from ../../src/include/pgstat.h:20,
from controldata_utils.c:38:
../../src/include/utils/wait_event_types.h:60:9: note: previous definition of ‘WAIT_EVENT_EXTENSION’ with type ‘enum <anonymous>’
60 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION
| ^~~~~~~~~~~~~~~~~~~~
In file included from ../../src/include/pgstat.h:20,
from controldata_utils.c:38:
../../src/include/utils/wait_event.h:58:3: error: conflicting types for ‘WaitEventExtension’; have ‘enum <anonymous>’
58 | } WaitEventExtension;
| ^~~~~~~~~~~~~~~~~~
In file included from ../../src/include/utils/wait_event.h:29,
from ../../src/include/pgstat.h:20,
from controldata_utils.c:38:
../../src/include/utils/wait_event_types.h:61:3: note: previous declaration of ‘WaitEventExtension’ with type ‘WaitEventExtension’
61 | } WaitEventExtension;
| ^~~~~~~~~~~~~~~~~~
make[2]: *** [Makefile:174: controldata_utils_srv.o] Erro 1
make[2]: Saindo do diretório '/usr/src/postgres_commits/src/common'
make[1]: *** [Makefile:42: all-common-recurse] Erro 2
make[1]: Saindo do diretório '/usr/src/postgres_commits/src'
make: *** [GNUmakefile:11: all-src-recurse] Erro 2
ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c' "WAIT_EVENT_EXTESION" .
ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c' "WAIT_EVENT_EXTENSION" .
from controldata_utils.c:38:
../../src/include/utils/wait_event.h:56:9: error: redeclaration of enumerator ‘WAIT_EVENT_EXTENSION’
56 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
| ^~~~~~~~~~~~~~~~~~~~
In file included from ../../src/include/utils/wait_event.h:29,
from ../../src/include/pgstat.h:20,
from controldata_utils.c:38:
../../src/include/utils/wait_event_types.h:60:9: note: previous definition of ‘WAIT_EVENT_EXTENSION’ with type ‘enum <anonymous>’
60 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION
| ^~~~~~~~~~~~~~~~~~~~
In file included from ../../src/include/pgstat.h:20,
from controldata_utils.c:38:
../../src/include/utils/wait_event.h:58:3: error: conflicting types for ‘WaitEventExtension’; have ‘enum <anonymous>’
58 | } WaitEventExtension;
| ^~~~~~~~~~~~~~~~~~
In file included from ../../src/include/utils/wait_event.h:29,
from ../../src/include/pgstat.h:20,
from controldata_utils.c:38:
../../src/include/utils/wait_event_types.h:61:3: note: previous declaration of ‘WaitEventExtension’ with type ‘WaitEventExtension’
61 | } WaitEventExtension;
| ^~~~~~~~~~~~~~~~~~
make[2]: *** [Makefile:174: controldata_utils_srv.o] Erro 1
make[2]: Saindo do diretório '/usr/src/postgres_commits/src/common'
make[1]: *** [Makefile:42: all-common-recurse] Erro 2
make[1]: Saindo do diretório '/usr/src/postgres_commits/src'
make: *** [GNUmakefile:11: all-src-recurse] Erro 2
ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c' "WAIT_EVENT_EXTESION" .
ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c' "WAIT_EVENT_EXTENSION" .
grep -r --include '*.h' "WAIT_EVENT_EXTENSION" .
./src/backend/utils/activity/wait_event_types.h: WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION
./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED
./src/backend/utils/activity/wait_event_types.h: WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION
./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED
Not sure if this is really a mistake in my environment.
regards,
Ranier Vilela
On 2023-08-02 08:38, Ranier Vilela wrote: > Hi, > > On Tue, Aug 01, 2023 at 11:51:35AM +0900, Masahiro Ikeda wrote: >> Thanks for committing the main patch. > > Latest head > Ubuntu 64 bits > gcc 13 64 bits > > ./configure --without-icu > make clean > > make > > In file included from ../../src/include/pgstat.h:20, > from controldata_utils.c:38: > ../../src/include/utils/wait_event.h:56:9: error: redeclaration of > enumerator ‘WAIT_EVENT_EXTENSION’ > 56 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION, > | ^~~~~~~~~~~~~~~~~~~~ > In file included from ../../src/include/utils/wait_event.h:29, > from ../../src/include/pgstat.h:20, > from controldata_utils.c:38: > ../../src/include/utils/wait_event_types.h:60:9: note: previous > definition of ‘WAIT_EVENT_EXTENSION’ with type ‘enum > <anonymous>’ > 60 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION > | ^~~~~~~~~~~~~~~~~~~~ > In file included from ../../src/include/pgstat.h:20, > from controldata_utils.c:38: > ../../src/include/utils/wait_event.h:58:3: error: conflicting types > for ‘WaitEventExtension’; have ‘enum <anonymous>’ > 58 | } WaitEventExtension; > | ^~~~~~~~~~~~~~~~~~ > In file included from ../../src/include/utils/wait_event.h:29, > from ../../src/include/pgstat.h:20, > from controldata_utils.c:38: > ../../src/include/utils/wait_event_types.h:61:3: note: previous > declaration of ‘WaitEventExtension’ with type > ‘WaitEventExtension’ > 61 | } WaitEventExtension; > | ^~~~~~~~~~~~~~~~~~ > make[2]: *** [Makefile:174: controldata_utils_srv.o] Erro 1 > make[2]: Saindo do diretório '/usr/src/postgres_commits/src/common' > make[1]: *** [Makefile:42: all-common-recurse] Erro 2 > make[1]: Saindo do diretório '/usr/src/postgres_commits/src' > make: *** [GNUmakefile:11: all-src-recurse] Erro 2 > ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c' > "WAIT_EVENT_EXTESION" . > ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c' > "WAIT_EVENT_EXTENSION" . > > grep -r --include '*.h' "WAIT_EVENT_EXTENSION" . > ./src/backend/utils/activity/wait_event_types.h: WAIT_EVENT_EXTENSION > = PG_WAIT_EXTENSION > ./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION = > PG_WAIT_EXTENSION, > ./src/include/utils/wait_event.h: > WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED > > Not sure if this is really a mistake in my environment. I can build without error. * latest commit(3845577cb55eab3e06b3724e307ebbcac31f4841) * gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.1) The result in my environment is $ grep -r --include '*.h' "WAIT_EVENT_EXTENSION" . ./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION, ./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED I think the error occurred because the old wait_event_types.h still exists. Can you remove it before building and try again? $ make maintainer-clean # remove wait_event_types.h $ ./configure --without-icu $ make Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Em ter., 1 de ago. de 2023 às 21:34, Masahiro Ikeda <ikedamsh@oss.nttdata.com> escreveu:
On 2023-08-02 08:38, Ranier Vilela wrote:
> Hi,
>
> On Tue, Aug 01, 2023 at 11:51:35AM +0900, Masahiro Ikeda wrote:
>> Thanks for committing the main patch.
>
> Latest head
> Ubuntu 64 bits
> gcc 13 64 bits
>
> ./configure --without-icu
> make clean
>
> make
>
> In file included from ../../src/include/pgstat.h:20,
> from controldata_utils.c:38:
> ../../src/include/utils/wait_event.h:56:9: error: redeclaration of
> enumerator ‘WAIT_EVENT_EXTENSION’
> 56 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
> | ^~~~~~~~~~~~~~~~~~~~
> In file included from ../../src/include/utils/wait_event.h:29,
> from ../../src/include/pgstat.h:20,
> from controldata_utils.c:38:
> ../../src/include/utils/wait_event_types.h:60:9: note: previous
> definition of ‘WAIT_EVENT_EXTENSION’ with type ‘enum
> <anonymous>’
> 60 | WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION
> | ^~~~~~~~~~~~~~~~~~~~
> In file included from ../../src/include/pgstat.h:20,
> from controldata_utils.c:38:
> ../../src/include/utils/wait_event.h:58:3: error: conflicting types
> for ‘WaitEventExtension’; have ‘enum <anonymous>’
> 58 | } WaitEventExtension;
> | ^~~~~~~~~~~~~~~~~~
> In file included from ../../src/include/utils/wait_event.h:29,
> from ../../src/include/pgstat.h:20,
> from controldata_utils.c:38:
> ../../src/include/utils/wait_event_types.h:61:3: note: previous
> declaration of ‘WaitEventExtension’ with type
> ‘WaitEventExtension’
> 61 | } WaitEventExtension;
> | ^~~~~~~~~~~~~~~~~~
> make[2]: *** [Makefile:174: controldata_utils_srv.o] Erro 1
> make[2]: Saindo do diretório '/usr/src/postgres_commits/src/common'
> make[1]: *** [Makefile:42: all-common-recurse] Erro 2
> make[1]: Saindo do diretório '/usr/src/postgres_commits/src'
> make: *** [GNUmakefile:11: all-src-recurse] Erro 2
> ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c'
> "WAIT_EVENT_EXTESION" .
> ranier@notebook2:/usr/src/postgres_commits$ grep -r --include '*.c'
> "WAIT_EVENT_EXTENSION" .
>
> grep -r --include '*.h' "WAIT_EVENT_EXTENSION" .
> ./src/backend/utils/activity/wait_event_types.h: WAIT_EVENT_EXTENSION
> = PG_WAIT_EXTENSION
> ./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION =
> PG_WAIT_EXTENSION,
> ./src/include/utils/wait_event.h:
> WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED
>
> Not sure if this is really a mistake in my environment.
I can build without error.
* latest commit(3845577cb55eab3e06b3724e307ebbcac31f4841)
* gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.1)
The result in my environment is
$ grep -r --include '*.h' "WAIT_EVENT_EXTENSION" .
./src/include/utils/wait_event.h: WAIT_EVENT_EXTENSION =
PG_WAIT_EXTENSION,
./src/include/utils/wait_event.h:
WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED
I think the error occurred because the old wait_event_types.h still
exists.
Can you remove it before building and try again?
$ make maintainer-clean # remove wait_event_types.h
$ ./configure --without-icu
$ make
Yeah, removing wait_event_types.h works.
Thanks.
regards,
Ranier Vilela
On 2023-08-01 12:23, Andres Freund wrote: > Hi, > > On 2023-08-01 12:14:49 +0900, Michael Paquier wrote: >> On Tue, Aug 01, 2023 at 11:51:35AM +0900, Masahiro Ikeda wrote: >> > Thanks for committing the main patch. >> > >> > In my understanding, the rest works are >> > * to support WaitEventExtensionMultiple() >> > * to replace WAIT_EVENT_EXTENSION to custom wait events >> > >> > Do someone already works for them? If not, I'll consider >> > how to realize them. >> >> Note that postgres_fdw and dblink use WAIT_EVENT_EXTENSION, but have >> no dependency to shared_preload_libraries. Perhaps these could just >> use a dynamic handling but that deserves a separate discussion because >> of the fact that they'd need shared memory without being able to >> request it. autoprewarm.c is much simpler. > > This is why the scheme as implemented doesn't really make sense to me. > It'd be > much easier to use if we had a shared hashtable with the identifiers > than > what's been merged now. > > In plenty of cases it's not realistic for an extension library to run > in each > backend, while still needing to wait for things. OK, I'll try to make a PoC patch. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Wed, Aug 02, 2023 at 06:34:15PM +0900, Masahiro Ikeda wrote: > On 2023-08-01 12:23, Andres Freund wrote: >> This is why the scheme as implemented doesn't really make sense to me. >> It'd be >> much easier to use if we had a shared hashtable with the identifiers >> than >> what's been merged now. >> >> In plenty of cases it's not realistic for an extension library to run in >> each >> backend, while still needing to wait for things. > > OK, I'll try to make a PoC patch. Hmm. There are a few things to take into account here: - WaitEventExtensionShmemInit() should gain a dshash_create(), to make sure that the shared table is around, and we are going to have a reference to it in WaitEventExtensionCounterData, saved from dshash_get_hash_table_handle(). - The hash table entries could just use nextId as key to look at the entries, with entries added during WaitEventExtensionNew(), and use as contents the name of the wait event. We are going to need a fixed size for these custom strings, but perhaps a hard limit of 256 characters for each entry of the hash table is more than enough for most users? - WaitEventExtensionRegisterName() could be removed, I guess, replaced by a single WaitEventExtensionNew(), as of: uint32 WaitEventExtensionNew(const char *event_name); - GetWaitEventExtensionIdentifier() needs to switch to a lookup of the shared hash table, based on the eventId. All that would save from the extra WaitEventExtensionRegisterName() needed in the backends to keep a track of the names, indeed. -- Michael
Attachment
On 2023-08-08 08:54, Michael Paquier wrote: > On Wed, Aug 02, 2023 at 06:34:15PM +0900, Masahiro Ikeda wrote: >> On 2023-08-01 12:23, Andres Freund wrote: >>> This is why the scheme as implemented doesn't really make sense to >>> me. >>> It'd be >>> much easier to use if we had a shared hashtable with the identifiers >>> than >>> what's been merged now. >>> >>> In plenty of cases it's not realistic for an extension library to run >>> in >>> each >>> backend, while still needing to wait for things. >> >> OK, I'll try to make a PoC patch. > > Hmm. There are a few things to take into account here: > - WaitEventExtensionShmemInit() should gain a dshash_create(), to make > sure that the shared table is around, and we are going to have a > reference to it in WaitEventExtensionCounterData, saved from > dshash_get_hash_table_handle(). > - The hash table entries could just use nextId as key to look at the > entries, with entries added during WaitEventExtensionNew(), and use as > contents the name of the wait event. We are going to need a fixed > size for these custom strings, but perhaps a hard limit of 256 > characters for each entry of the hash table is more than enough for > most users? > - WaitEventExtensionRegisterName() could be removed, I guess, replaced > by a single WaitEventExtensionNew(), as of: > uint32 WaitEventExtensionNew(const char *event_name); > - GetWaitEventExtensionIdentifier() needs to switch to a lookup of the > shared hash table, based on the eventId. > > All that would save from the extra WaitEventExtensionRegisterName() > needed in the backends to keep a track of the names, indeed. Thank you for pointing the direction of implementation. I am thinking a bit that we also need another hash where the key is a custom string. For extensions that have no dependencies with shared_preload_libraries, I think we need to avoid that WaitEventExtensionNew() is called repeatedly and a new eventId is issued each time. So, is it better to have another hash where the key is a custom string and uniqueness is identified by it to determine if a new eventId should be issued? Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Tue, Aug 08, 2023 at 09:39:02AM +0900, Masahiro Ikeda wrote: > I am thinking a bit that we also need another hash where the key > is a custom string. For extensions that have no dependencies > with shared_preload_libraries, I think we need to avoid that > WaitEventExtensionNew() is called repeatedly and a new eventId > is issued each time. > > So, is it better to have another hash where the key is > a custom string and uniqueness is identified by it to determine > if a new eventId should be issued? Yeah, I was also considering if something like that is really necessary, but I cannot stop worrying about adding more contention to the hash table lookup each time an extention needs to retrieve an event ID to use for WaitLatch() or such. The results of the hash table lookups could be cached in each backend, still it creates an extra cost when combined with queries running in parallel on pg_stat_activity that do the opposite lookup event_id -> event_name. My suggestion adds more load to AddinShmemInitLock instead. Hence, I was just thinking about relying on AddinShmemInitLock to insert new entries in the hash table, only if its shmem state is not found when calling ShmemInitStruct(). Or perhaps it is just OK to not care about the impact event_name -> event_id lookup for fresh connections, and just bite the bullet with two lookup keys instead of relying on AddinShmemInitLock for the addition of new entries in the hash table? Hmm, perhaps you're right with your approach here, at the end. -- Michael
Attachment
On 2023-08-08 10:05, Michael Paquier wrote: > On Tue, Aug 08, 2023 at 09:39:02AM +0900, Masahiro Ikeda wrote: >> I am thinking a bit that we also need another hash where the key >> is a custom string. For extensions that have no dependencies >> with shared_preload_libraries, I think we need to avoid that >> WaitEventExtensionNew() is called repeatedly and a new eventId >> is issued each time. >> >> So, is it better to have another hash where the key is >> a custom string and uniqueness is identified by it to determine >> if a new eventId should be issued? > > Yeah, I was also considering if something like that is really > necessary, but I cannot stop worrying about adding more contention to > the hash table lookup each time an extention needs to retrieve an > event ID to use for WaitLatch() or such. The results of the hash > table lookups could be cached in each backend, still it creates an > extra cost when combined with queries running in parallel on > pg_stat_activity that do the opposite lookup event_id -> event_name. > > My suggestion adds more load to AddinShmemInitLock instead. > Hence, I was just thinking about relying on AddinShmemInitLock to > insert new entries in the hash table, only if its shmem state is not > found when calling ShmemInitStruct(). Or perhaps it is just OK to not > care about the impact event_name -> event_id lookup for fresh > connections, and just bite the bullet with two lookup keys instead of > relying on AddinShmemInitLock for the addition of new entries in the > hash table? Hmm, perhaps you're right with your approach here, at the > end. For the first idea, I agree that if a lot of new connections come in, it is easy to leads many conflicts. The only solution I can think of is to use connection pooling now. IIUC, the second idea is based on the premise of allocating their shared memory for each extension. In that case, the cons of the first idea can be solved because the wait event infos are saved in their shared memory and they don't need call WaitEventExtensionNew() anymore. Is that right? Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
I accidentally attached a patch in one previous email. But, you don't need to check it, sorry. (v1-0001-Change-to-manage-custom-wait-events-in-shared-hash.patch) Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Tue, Aug 08, 2023 at 08:40:53PM +0900, Masahiro Ikeda wrote: > I accidentally attached a patch in one previous email. > But, you don't need to check it, sorry. > (v1-0001-Change-to-manage-custom-wait-events-in-shared-hash.patch) Sure, no worries. With that in place, the init function in worker_spi can be removed. -- Michael
Attachment
Hi, On 2023-08-08 08:54:10 +0900, Michael Paquier wrote: > - WaitEventExtensionShmemInit() should gain a dshash_create(), to make > sure that the shared table is around, and we are going to have a > reference to it in WaitEventExtensionCounterData, saved from > dshash_get_hash_table_handle(). I'm not even sure it's worth using dshash here. Why don't we just create a decently sized dynahash (say 128 enties) in shared memory? We overallocate shared memory by enough that there's a lot of headroom for further entries, in the rare cases they're needed. > We are going to need a fixed size for these custom strings, but perhaps a > hard limit of 256 characters for each entry of the hash table is more than > enough for most users? I'd just use NAMEDATALEN. - Andres
On Tue, Aug 08, 2023 at 03:59:54PM -0700, Andres Freund wrote: > On 2023-08-08 08:54:10 +0900, Michael Paquier wrote: >> - WaitEventExtensionShmemInit() should gain a dshash_create(), to make >> sure that the shared table is around, and we are going to have a >> reference to it in WaitEventExtensionCounterData, saved from >> dshash_get_hash_table_handle(). > > I'm not even sure it's worth using dshash here. Why don't we just create a > decently sized dynahash (say 128 enties) in shared memory? We overallocate > shared memory by enough that there's a lot of headroom for further entries, in > the rare cases they're needed. The question here would be how many slots the most popular extensions actually need, but that could always be sized up based on the feedback. >> We are going to need a fixed size for these custom strings, but perhaps a >> hard limit of 256 characters for each entry of the hash table is more than >> enough for most users? > > I'd just use NAMEDATALEN. Both suggestions WFM. -- Michael
Attachment
Hi, On 2023-08-09 08:03:29 +0900, Michael Paquier wrote: > On Tue, Aug 08, 2023 at 03:59:54PM -0700, Andres Freund wrote: > > On 2023-08-08 08:54:10 +0900, Michael Paquier wrote: > >> - WaitEventExtensionShmemInit() should gain a dshash_create(), to make > >> sure that the shared table is around, and we are going to have a > >> reference to it in WaitEventExtensionCounterData, saved from > >> dshash_get_hash_table_handle(). > > > > I'm not even sure it's worth using dshash here. Why don't we just create a > > decently sized dynahash (say 128 enties) in shared memory? We overallocate > > shared memory by enough that there's a lot of headroom for further entries, in > > the rare cases they're needed. > > The question here would be how many slots the most popular extensions > actually need, but that could always be sized up based on the > feedback. On a default initdb (i.e. 128MB s_b), after explicitly disabling huge pages, we over-allocate shared memory by by 1922304 bytes, according to pg_shmem_allocations. We allow that memory to be used for things like shared hashtables that grow beyond their initial size. So even if the hash table's static size is too small, there's lots of room to grow, even on small systems. Just because it's somewhat interesting: With huge pages available and not disabled, we over-allocate by 3364096 bytes. Greetings, Andres Freund
Hi, Thanks for your comments to v1 patch. I made v2 patch. Main changes are * change to NAMEDATALEN * change to use dynahash from dshash * remove worker_spi_init() * create second hash table to find a event id from a name to identify uniquness. It enable extensions which don't use share memory for their use to define custom wait events because WaitEventExtensionNew() will not allocate duplicate wait events. * create PoC patch to show that extensions, which don't use shared memory for their use, can define custom wait events. (v2-0002-poc-custom-wait-event-for-dblink.patch) I'm worrying about * Is 512(wee_hash_max_size) the maximum number of the custom wait events sufficient? * Is there any way to not force extensions that don't use shared memory for their use like dblink to acquire AddinShmemInitLock?; Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
Hi, On 2023-08-09 20:10:42 +0900, Masahiro Ikeda wrote: > * Is there any way to not force extensions that don't use shared > memory for their use like dblink to acquire AddinShmemInitLock?; I think the caller shouldn't need to do deal with AddinShmemInitLock at all. Greetings, Andres Freund
On Wed, Aug 09, 2023 at 08:10:42PM +0900, Masahiro Ikeda wrote:
> * create second hash table to find a event id from a name to
> identify uniquness. It enable extensions which don't use share
> memory for their use to define custom wait events because
> WaitEventExtensionNew() will not allocate duplicate wait events.
Okay, a second hash table to check if events are registered works for
me.
> * create PoC patch to show that extensions, which don't use shared
> memory for their use, can define custom wait events.
> (v2-0002-poc-custom-wait-event-for-dblink.patch)
>
> I'm worrying about
> * Is 512(wee_hash_max_size) the maximum number of the custom wait
> events sufficient?
Thanks for sending a patch!
I'm OK to start with that. This could always be revisited later, but
even for a server loaded with a bunch of extensions that looks more
than enough to me.
> * Is there any way to not force extensions that don't use shared
> memory for their use like dblink to acquire AddinShmemInitLock?;
Yes, they don't need it at all as the dynahashes are protected with
their own LWLocks.
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -53,3 +53,4 @@ XactTruncationLock 44
# 45 was XactTruncationLock until removal of BackendRandomLock
WrapLimitsVacuumLock 46
NotifyQueueTailLock 47
+WaitEventExtensionLock 48
This new LWLock needs to be added to wait_event_names.txt, or it won't
be reported to pg_stat_activity and it would not be documented when
the sgml docs are generated from the txt data.
-extern uint32 WaitEventExtensionNew(void);
-extern void WaitEventExtensionRegisterName(uint32 wait_event_info,
- const char *wait_event_name);
+extern uint32 WaitEventExtensionNew(const char *wait_event_name);
Looks about right, and the docs are refreshed.
+static const int wee_hash_init_size = 128;
+static const int wee_hash_max_size = 512;
I would use a few #defines with upper-case characters here instead as
these are constants for us.
Now that it is possible to rely on LWLocks for the hash tables, more
cleanup is possible in worker_spi, with the removal of
worker_spi_state, the shmem hooks and their routines. The only thing
that should be needed is something like that at the start of
worker_spi_main() (same position as worker_spi_shmem_init now):
+static uint32 wait_event = 0;
[...]
+ if (wait_event == 0)
+ wait_event = WaitEventExtensionNew("worker_spi_main");
The updates in 001_worker_spi.pl look OK.
+ * The entry must be stored because it's registered in
+ * WaitEventExtensionNew().
*/
- eventId -= NUM_BUILTIN_WAIT_EVENT_EXTENSION;
+ if (!entry)
+ ereport(ERROR,
+ errmsg("could not find the name for custom wait event ID %u", eventId));
Yeah, I think that's better than just falling back to "extension". An
ID reported in pg_stat_activity should always have an entry, or we
have race conditions. This should be an elog(ERROR), as in
this-error-shall-never-happen. No need to translate the error string,
as well (the docs have been updated with this change. thanks!).
Additionally, LWLockHeldByMeInMode(AddinShmemInitLock) in
WaitEventExtensionNew() should not be needed, thanks to
WaitEventExtensionLock.
+ * WaitEventExtensionNameHash is used to find the name from a event id.
+ * It enables all backends look up them without additional processing
+ * per backend like LWLockRegisterTranche().
It does not seem necessary to mention LWLockRegisterTranche().
+ * WaitEventExtensionIdHash is used to find the event id from a name.
+ * Since it can identify uniquness by the names, extensions that do not
+ * use shared memory also be able to define custom wait events without
+ * defining duplicate wait events.
Perhaps this could just say that this table is necessary to ensure
that we don't have duplicated entries when registering new strings
with their IDs? s/uniquness/uniqueness/. The second part of the
sentence about shared memory does not seem necessary now.
+ sz = add_size(sz, hash_estimate_size(wee_hash_init_size,
+ sizeof(WaitEventExtensionNameEntry)));
+ sz = add_size(sz, hash_estimate_size(wee_hash_init_size,
+ sizeof(WaitEventExtensionIdEntry)));
Err, this should use the max size, and not the init size for the size
estimation, no?
+ if (strlen(wait_event_name) >= NAMEDATALEN)
+ ereport(ERROR,
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("wait event name is too long"));
This could just be an elog(ERROR), I assume, as that could only be
reached by developers. The string needs to be rewritten, like "cannot
use custom wait event string longer than %u characters", or something
like that.
+ if (wait_event_info == NULL)
+ {
+ wait_event_info = (uint32 *) MemoryContextAlloc(TopMemoryContext, sizeof(uint32));
+ LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+ *wait_event_info = WaitEventExtensionNew("dblink_get_con");
+ LWLockRelease(AddinShmemInitLock);
+ }
+ conn = libpqsrv_connect(connstr, *wait_event_info)
In 0002. Caching the value statically in the backend is what you
should do, but a pointer, an allocation to the TopMemoryContext and a
dependency to AddinShmemInitLock should not be necessary when dealing
with a uint32. You could use an initial value of 0, for example, or
just PG_WAIT_EXTENSION but the latter is not really necessary and
would bypass the sanity checks.
+ /* Register the new custom wait event in the shared hash table */
+ LWLockAcquire(WaitEventExtensionLock, LW_EXCLUSIVE);
+
+ name_entry = (WaitEventExtensionNameEntry *)
+ hash_search(WaitEventExtensionNameHash, &eventId, HASH_ENTER, &found);
+ Assert(!found);
+ strlcpy(name_entry->wait_event_name, wait_event_name, sizeof(name_entry->wait_event_name));
+
+ id_entry = (WaitEventExtensionIdEntry *)
+ hash_search(WaitEventExtensionIdHash, &wait_event_name, HASH_ENTER, &found);
+ Assert(!found);
+ id_entry->event_id = eventId;
+
+ LWLockRelease(WaitEventExtensionLock);
The logic added to WaitEventExtensionNew() is a bit racy, where it
would be possible with the same entry to be added multiple times.
Imagine for example the following:
- Process 1 does WaitEventExtensionNew("foo1"), does not find the
entry by name in hash_search, gets an eventId of 1, releases the
spinlock.
- Process 2 calls as well WaitEventExtensionNew("foo1"), does not find
the entry by name because it has not been added by process 1 yet,
allocates an eventId of 2
- Process 2 takes first WaitEventExtensionLock in LW_EXCLUSIVE to add
entry "foo1", there is no entry by name, so one is added for the ID.
WaitEventExtensionLock is released
- Process 1, that was waiting on WaitEventExtensionLock, can now take
it in exclusive mode. It finds an entry by name for "foo1", fails the
assertion because an entry is found.
I think that the ordering of WaitEventExtensionNew() should be
reworked a bit. This order should be safer.
- Take WaitEventExtensionLock in shared mode, look if there's an entry
by name, release the lock. The patch does that.
- If an entry is found, return, we're OK. The patch does that.
- Take again WaitEventExtensionLock in exclusive mode.
- Look again at the hash table with the name given, in case somebody
has inserted an equivalent entry in the short window where the lock
was not held.
-- If an entry is found, release the lock and leave, we're OK.
-- If an entry is not found, keep the lock.
- Acquire the spinlock, and get a new event ID. Release spinlock.
- Add the new entries to both tables, both assertions on found are OK
to have.
- Release LWLock and leave.
--
Michael
Attachment
Hi, Thanks for your comments about the v2 patches. I updated to v3 patches. The main changes are: * remove the AddinShmemInitLock assertion * add the new lock (WaitEventExtensionLock) to wait_event_names.txt * change "static const int wee_hash_XXX_size" to "#define XXX" * simplify worker_spi. I removed codes related to share memory and try to allocate the new wait event before waiting per background worker. * change to elog from ereport because the errors are for developers. * revise comments as advised. * fix the request size for shared memory correctly * simplify dblink.c * fix process ordering of WaitEventExtensionNew() as advised to avoid leading illegal state. In addition, I change the followings: * update about custom wait events in sgml. we don't need to use shmem_startup_hook. * change the hash names for readability. (ex. WaitEventExtensionNameHash -> WaitEventExtensionHashById) * fix a bug to fail to get already defined events by names because HASH_BLOBS was specified. HASH_STRINGS is right since the key is C strings. I create a new entry in commitfest for CI testing. (https://commitfest.postgresql.org/44/4494/) Thanks for closing the former entry. (https://commitfest.postgresql.org/43/4368/) Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Attachment
On Thu, Aug 10, 2023 at 01:08:39PM +0900, Masahiro Ikeda wrote:
> In addition, I change the followings:
> * update about custom wait events in sgml. we don't need to use
> shmem_startup_hook.
> * change the hash names for readability.
> (ex. WaitEventExtensionNameHash -> WaitEventExtensionHashById)
> * fix a bug to fail to get already defined events by names
> because HASH_BLOBS was specified. HASH_STRINGS is right since
> the key is C strings.
That's what I get based on what ShmemInitHash() relies on.
I got a few more comments about v3. Overall this looks much better.
This time the ordering of the operations in WaitEventExtensionNew()
looks much better.
+ * The entry must be stored because it's registered in
+ * WaitEventExtensionNew().
Not sure of the value added by this comment, I would remove it.
+ if (!entry)
+ elog(ERROR, "could not find the name for custom wait event ID %u",
+ eventId);
Or a simpler "could not find custom wait event name for ID %u"?
+static HTAB *WaitEventExtensionHashById; /* find names from ids */
+static HTAB *WaitEventExtensionHashByName; /* find ids from names */
These names are OK here.
+/* Local variables */
+static uint32 worker_spi_wait_event = 0;
That's a cached value, used as a placeholder for the custom wait event
ID found from the table.
+ HASH_ELEM | HASH_STRINGS); /* key is Null-terminated C strings */
Looks obvious to me based on the code, I would remove this note.
+/* hash table entres */
s/entres/entries/
+ /*
+ * Allocate and register a new wait event. But, we need to recheck because
+ * other processes could already do so while releasing the lock.
+ */
Could be reworked for the second sentence, like "Recheck if the event
exists, as it could be possible that a concurrent process has inserted
one with the same name while the lock was previously released."
+ /* Recheck */
Duplicate.
/* OK to make connection */
- conn = libpqsrv_connect(connstr, WAIT_EVENT_EXTENSION);
+ if (wait_event_info == 0)
+ wait_event_info = WaitEventExtensionNew("dblink_get_con");
+ conn = libpqsrv_connect(connstr, wait_event_info);
It is going to be difficult to make that simpler ;)
This looks correct, but perhaps we need to think harder about the
custom event names and define a convention when more of this stuff is
added to the core modules.
--
Michael
Attachment
On Thu, Aug 10, 2023 at 05:37:55PM +0900, Michael Paquier wrote: > This looks correct, but perhaps we need to think harder about the > custom event names and define a convention when more of this stuff is > added to the core modules. Okay, I have put my hands on that, fixing a couple of typos, polishing a couple of comments, clarifying the docs and applying an indentation. And here is a v4. Any thoughts or comments? I'd like to apply that soon, so as we are able to move on with the wait event catalog and assigning custom wait events to the other in-core modules. -- Michael
Attachment
On 2023-08-14 08:06, Michael Paquier wrote: > On Thu, Aug 10, 2023 at 05:37:55PM +0900, Michael Paquier wrote: >> This looks correct, but perhaps we need to think harder about the >> custom event names and define a convention when more of this stuff is >> added to the core modules. > > Okay, I have put my hands on that, fixing a couple of typos, polishing > a couple of comments, clarifying the docs and applying an indentation. > And here is a v4. > > Any thoughts or comments? I'd like to apply that soon, so as we are > able to move on with the wait event catalog and assigning custom wait > events to the other in-core modules. Thanks! I confirmed the changes, and all tests passed. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Mon, Aug 14, 2023 at 12:31:05PM +0900, Masahiro Ikeda wrote: > Thanks! I confirmed the changes, and all tests passed. Okay, cool. I got some extra time today and applied that, with a few more tweaks. -- Michael
Attachment
On 2023-08-14 15:28, Michael Paquier wrote: > On Mon, Aug 14, 2023 at 12:31:05PM +0900, Masahiro Ikeda wrote: >> Thanks! I confirmed the changes, and all tests passed. > > Okay, cool. I got some extra time today and applied that, with a few > more tweaks. Thanks for applying master branch! > This looks correct, but perhaps we need to think harder about the > custom event names and define a convention when more of this stuff is > added to the core modules. I checked the source code how many functions use WAIT_EVENT_EXTENSION. There are 3 contrib modules and a test module use WAIT_EVENT_EXTENSION and there are 8 places where it is called as an argument. * dblink - dblink_get_conn(): the wait event is set until the connection establishment succeeded - dblink_connect(): same as above * autoprewarm - autoprewarm_main(): the wait event is set until shutdown request is received - autoprewarm_main(): the wait event is set until the next dump time * postgres_fdw - connect_pg_server(): the wait event is set until connection establishment succeeded - pgfdw_get_result(): the wait event is set until the results are received - pgfdw_get_cleanup_result(): same as above except for abort cleanup * test_sh_mq - wait_for_workers_to_become_ready(): the wait event is set until the workers become ready I'm thinking a name like "contrib name + description summary" would be nice. The "contrib name" is namespace-like and the "description summary" is the same as the name of the waiting event name in core. For example, "DblinkConnect" for dblink. In the same as the core one, I thought the name should be the camel case. BTW, is it better to discuss this in a new thread because other developers might be interested in user-facing wait event names? I also would like to add documentation on the wait events for each modules, as they are not mentioned now. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Mon, Aug 14, 2023 at 05:55:42PM +0900, Masahiro Ikeda wrote: > I'm thinking a name like "contrib name + description summary" would > be nice. The "contrib name" is namespace-like and the "description summary" > is the same as the name of the waiting event name in core. For example, > "DblinkConnect" for dblink. In the same as the core one, I thought the name > should be the camel case. Or you could use something more in line with the other in-core wait events formatted as camel-case, like DblinkConnect, etc. > BTW, is it better to discuss this in a new thread because other developers > might be interested in user-facing wait event names? I also would like to > add documentation on the wait events for each modules, as they are not mentioned > now. Saying that, having some documentation on the page of each extension is mandatory once these can be customized, in my opinion. All that should be discussed on a new, separate thread, to attract the correct audience. -- Michael
Attachment
On 2023-08-14 18:26, Michael Paquier wrote: > On Mon, Aug 14, 2023 at 05:55:42PM +0900, Masahiro Ikeda wrote: >> I'm thinking a name like "contrib name + description summary" would >> be nice. The "contrib name" is namespace-like and the "description >> summary" >> is the same as the name of the waiting event name in core. For >> example, >> "DblinkConnect" for dblink. In the same as the core one, I thought the >> name >> should be the camel case. > > Or you could use something more in line with the other in-core wait > events formatted as camel-case, like DblinkConnect, etc. > >> BTW, is it better to discuss this in a new thread because other >> developers >> might be interested in user-facing wait event names? I also would like >> to >> add documentation on the wait events for each modules, as they are not >> mentioned >> now. > > Saying that, having some documentation on the page of each extension > is mandatory once these can be customized, in my opinion. All that > should be discussed on a new, separate thread, to attract the correct > audience. OK. I'll make a new patch and start a new thread. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
On Tue, Aug 15, 2023 at 09:14:15AM +0900, Masahiro Ikeda wrote: > OK. I'll make a new patch and start a new thread. Cool, thanks! -- Michael