Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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
From 83e40dace75d0df91e8b7f617bd03d02bd8450d4 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 14 Aug 2023 07:48:15 +0900
Subject: [PATCH v4] Change custom wait events to use shared hash tables
Currently, names of the custom wait event must be registered for each
backends, requiring all these to link to the shared memory area of an
extention, even if not loaded with shared_preload_libraries.
This patch relaxes the constraints by storing the wait events and their
names in hash tables in shared memory. First, this has the advantage to
simplify the registration of custom wait events to a single routine
call that returns an event ID, either existing or incremented:
uint32 WaitEventExtensionNew(const char *wait_event_name);
Any other backend looking at the wait event information, for instance
via pg_stat_activity, is then able to automatically know about the new
event name.
The code changes done in worker_spi show how things are simplified:
- worker_spi_init() is gone.
- No more shmem hooks.
- The custom wait event ID is cached in the process that needs to set
it, with one single call to WaitEventExtensionNew() to retrieve it.
Per suggestion from Andres Freund.
Author: Masahiro Ikeda
Discussion: https://postgr.es/m/20230801032349.aaiuvhtrcvvcw...@awork3.anarazel.de
---
src/include/utils/wait_event.h| 18 +-
src/backend/storage/lmgr/lwlocknames.txt | 1 +
src/backend/utils/activity/wait_event.c | 217 +++---
.../utils/activity/wait_event_names.txt | 1 +
.../modules/worker_spi/t/001_worker_spi.pl| 18 +-
.../modules/worker_spi/worker_spi--1.0.sql| 5 -
src/test/modules/worker_spi/worker_spi.c | 109 +
doc/src/sgml/monitoring.sgml | 5 +-
doc/src/sgml/xfunc.sgml | 26 +--
src/tools/pgindent/typedefs.list | 2 +
10 files changed, 164 insertions(+), 238 deletions(-)
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index aad8bc08fa..3cffae23e1 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -44,12 +44,14 @@ extern PGDLLIMPORT uint32 *my_wait_event_info;
* Use this category when the server process is waiting for some condition
* defined by an extension module.
*
- * Extensions can define their own wait events in this category. First,
- * they should call WaitEventExtensionNew() to get one or more wait event
- * IDs that are allocated from a shared counter. These can be used directly
- * with pgstat_report_wait_start() or equivalent. Next, each individual
- * process should call WaitEventExtensionRegisterName() to associate a wait
- * event string to the number allocated previously.
+ * Extensions can define their own wait events in this category. They should
+ * call WaitEventExtensionNew() with a wait event string. If the wait event
+ * associated the string is already allocated, it returns the wait event info.
+ * If not, it will get one wait event ID that is allocated from a shared
+ + counter, associate the string to the number in the shared dynamic hash and
+ * return the wait event info.
+ *
+ * The ID retrieved can be used with pgstat_report_wait_start() or equivalent.
*/
typedef enum
{
@@ -60,9 +62,7 @@ typedef enum
extern void WaitEventExtensionShmemInit(void);
extern Size WaitEventExtensionShmemSize(void);
-extern uint32 WaitEventExtensionNew(void);
-extern void WaitEventExtensionRegisterName(uint32 wait_event_info,
- const char *wait_event_name);
+extern uint32 WaitEventExtensionNew(const char *wait_event_name);
/* --
* pgstat_report_wait_start() -
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index b34b6afecd..7af3e0ba1a 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -53,3 +53,4 @@ XactTruncationLock 44
# 45 was XactTruncationLock until removal of BackendRandomLock
WrapLimitsVacuumLock46
NotifyQueueTailLock 47
+WaitEventExtensionLock 48
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index b3596ece80..14750233d4 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -45,6 +45,41 @@ uint32 *my_wait_event_info = _my_wait_event_info;
#define
Re: Support to define custom wait events for extensions
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
signature.asc
Description: PGP signature
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom 234cc7d852aebf78285ccde383f27ea35a27dad2 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Thu, 10 Aug 2023 10:44:41 +0900
Subject: [PATCH 2/2] poc: custom wait event for dblink
---
contrib/dblink/dblink.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 41e1f6c91d..b7158ecd4f 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -129,6 +129,7 @@ static void restoreLocalGucs(int nestlevel);
/* Global */
static remoteConn *pconn = NULL;
static HTAB *remoteConnHash = NULL;
+static uint32 wait_event_info = 0;
/*
* Following is list that holds multiple remote connections.
@@ -203,7 +204,9 @@ dblink_get_conn(char *conname_or_str,
dblink_connstr_check(connstr);
/* 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);
if (PQstatus(conn) == CONNECTION_BAD)
{
--
2.25.1
From 07bbff06a0f58f08d313c02cbb4104ad1db2a0e9 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Thu, 10 Aug 2023 10:43:34 +0900
Subject: [PATCH 1/2] Change to manage custom wait events in shared hash
Currently, names of the custom wait event must be registered
per backends. This patch relaxes the constraints by store the
wait events and their names in hash tables in shared memory.
So, all backends can look up the wait event names on
pg_stat_activity view without additional processing by extensions.
In addition, extensions which do not use shared memory for their
use can define custom wait events because WaitEventExtensionNew()
will never allocate new one if the wait event associated to the
name is already allocated. It use hash table to identify uniqueness.
---
doc/src/sgml/monitoring.sgml | 7 +-
doc/src/sgml/xfunc.sgml | 26 +--
src/backend/storage/lmgr/lwlocknames.txt | 1 +
src/backend/utils/activity/wait_event.c | 219 +++---
.../utils/activity/wait_event_names.txt | 1 +
src/include/utils/wait_event.h| 17 +-
.../modules/worker_spi/t/001_worker_spi.pl| 18 +-
.../modules/worker_spi/worker_spi--1.0.sql| 5 -
src/test/modules/worker_spi/worker_spi.c | 109 +
9 files changed, 165 insertions(+), 238 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index f4fc5d814f..19181832d7 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1121,10 +1121,9 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
LWLock types
to the list shown in and
. In some cases, the name
- assigned by an extension will not be available in all server processes;
- so an Extension or LWLock wait
- event might be reported as just
- extension rather than the
+ of LWLock assigned by an extension will not be
+ available in all server processes; so an wait event might be reported
+ as just extension rather than the
extension-assigned name.
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index d6345a775b..281c178b0e 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3454,33 +3454,15 @@ if (!ptr)
-Shared Memory and Custom Wait Events
+Custom Wait Events
Add-ins can define custom wait events under the wait event type
- Extension. The add-in's shared library must be
- preloaded by specifying it in shared_preload_libraries,
- and register a shmem_request_hook and a
-
Re: Support to define custom wait events for extensions
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 @@ XactTruncationLock44
# 45 was XactTruncationLock until removal of BackendRandomLock
WrapLimitsVacuumLock 46
NotifyQueueTailLock47
+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));
+
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom ec7d2b0ae47be9bedabb4a4127c914bfb8c361b5 Mon Sep 17 00:00:00 2001 From: Masahiro Ikeda Date: Wed, 9 Aug 2023 19:19:58 +0900 Subject: [PATCH 1/2] Change to manage custom wait events in shared hash Currently, names of the custom wait event must be registered per backends. This patch relaxes the constraints by store the wait events and their names in hash tables in shared memory. So, all backends can look up the wait event names on pg_stat_activity view without additional processing by extensions. In addition, extensions which do not use shared memory for their use can define custom wait events because WaitEventExtensionNew() will never allocate new one if the wait event associated to the name is already allocated. It use hash table to identify uniquness. --- doc/src/sgml/monitoring.sgml | 7 +- doc/src/sgml/xfunc.sgml | 10 +- src/backend/storage/lmgr/lwlocknames.txt | 1 + src/backend/utils/activity/wait_event.c | 198 +++--- src/include/utils/wait_event.h| 17 +- .../modules/worker_spi/t/001_worker_spi.pl| 18 +- .../modules/worker_spi/worker_spi--1.0.sql| 5 - src/test/modules/worker_spi/worker_spi.c | 22 +- 8 files changed, 137 insertions(+), 141 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index f4fc5d814f..19181832d7 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -1121,10 +1121,9 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i LWLock types to the list shown in and . In some cases, the name - assigned by an extension will not be available in all server processes; - so an Extension or LWLock wait - event might be reported as just - extension rather than the + of LWLock assigned by an extension will not be + available in all server processes; so an wait event might be reported + as just extension rather than the extension-assigned name. diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index d6345a775b..7fec034db4 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3470,17 +3470,13 @@ void RequestAddinShmemSpace(int size) - shmem_startup_hook can allocate in shared memory + shmem_startup_hook can allocate in dynamic shared memory custom wait events by calling while holding the LWLock AddinShmemInitLock to avoid any race conditions: -uint32 WaitEventExtensionNew(void) - - Next, each process needs to associate the wait event allocated previously - to a user-facing custom string, which is something done by calling: - -void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name) +uint32 WaitEventExtensionNew(const char *wait_event_name) + The wait event is associated to a user-facing custom string. An example can be found in src/test/modules/worker_spi in the PostgreSQL source tree. diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt index b34b6afecd..7af3e0ba1a 100644 --- a/src/backend/storage/lmgr/lwlocknames.txt +++ b/src/backend/storage/lmgr/lwlocknames.txt @@ -53,3 +53,4 @@ XactTruncationLock 44 # 45 was XactTruncationLock until removal of BackendRandomLock WrapLimitsVacuumLock46 NotifyQueueTailLock 47 +WaitEventExtensionLock 48 diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c index b3596ece80..15b2dd06cc 100644 --- a/src/backend/utils/activity/wait_event.c +++ b/src/backend/utils/activity/wait_event.c @@ -45,6 +45,42 @@ uint32 *my_wait_event_info = _my_wait_event_info; #define WAIT_EVENT_CLASS_MASK 0xFF00 #define WAIT_EVENT_ID_MASK 0x +/* + * Hash tables for storing custom wait event ids and their names in + * shared memory. + * + * WaitEventExtensionNameHash is used to find the name from a event id. + * It enables all backends look up them without additional processing + * per backend like
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom 0e3ccc6474bfcc51114d9363b7819b68f37fcad3 Mon Sep 17 00:00:00 2001 From: Masahiro Ikeda Date: Tue, 8 Aug 2023 19:24:32 +0900 Subject: [PATCH] Change to manage custom wait events in shared hash Currently, names of the custom wait event must be registered per backends. This patch relaxes the constraints to store the wait events and their names in the dynamic shared hashtable. So, all backends can look up the wait event names on pg_stat_activity view without additional processing by extensions. --- doc/src/sgml/monitoring.sgml | 7 +- doc/src/sgml/xfunc.sgml | 10 +- src/backend/storage/ipc/ipci.c| 6 + src/backend/storage/lmgr/lwlock.c | 2 + src/backend/utils/activity/wait_event.c | 275 -- src/backend/utils/init/postinit.c | 3 + src/include/storage/lwlock.h | 2 + src/include/utils/wait_event.h| 17 +- .../modules/worker_spi/t/001_worker_spi.pl| 18 +- src/test/modules/worker_spi/worker_spi.c | 6 +- src/tools/pgindent/typedefs.list | 2 +- 11 files changed, 216 insertions(+), 132 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index f4fc5d814f..19181832d7 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -1121,10 +1121,9 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i LWLock types to the list shown in and . In some cases, the name - assigned by an extension will not be available in all server processes; - so an Extension or LWLock wait - event might be reported as just - extension rather than the + of LWLock assigned by an extension will not be + available in all server processes; so an wait event might be reported + as just extension rather than the extension-assigned name. diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index d6345a775b..7fec034db4 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3470,17 +3470,13 @@ void RequestAddinShmemSpace(int size) - shmem_startup_hook can allocate in shared memory + shmem_startup_hook can allocate in dynamic shared memory custom wait events by calling while holding the LWLock AddinShmemInitLock to avoid any race conditions: -uint32 WaitEventExtensionNew(void) - - Next, each process needs to associate the wait event allocated previously - to a user-facing custom string, which is something done by calling: - -void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name) +uint32 WaitEventExtensionNew(const char *wait_event_name) + The wait event is associated to a user-facing custom string. An example can be found in src/test/modules/worker_spi in the PostgreSQL source tree.
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
Em ter., 1 de ago. de 2023 às 21:34, Masahiro Ikeda < ikeda...@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 > > ’ > >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 ’ > >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
Re: Support to define custom wait events for extensions
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 ’ 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 ’ 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
Re: Support to define custom wait events for extensions
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 ’ 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 ’ 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. regards, Ranier Vilela
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
On Mon, Jul 31, 2023 at 3:54 PM Michael Paquier 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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
At Mon, 31 Jul 2023 16:28:16 +0900, Michael Paquier 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
Re: Support to define custom wait events for extensions
On Mon, Jul 31, 2023 at 12:58 PM Michael Paquier 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) + + Next, each process needs to associate the wait event allocated previously + to a user-facing custom string, which is something done by calling: + +void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name) + + An example can be found in src/test/modules/worker_spi + in the PostgreSQL source tree. + 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
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 LWLock or Extension wait
> + so an Extension or LWLock 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
From 23b6c4c36fbf23ca9c6fb4f42a3f943be44689a6 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 31 Jul 2023 16:04:20 +0900
Subject: [PATCH v11] Support custom wait events for wait event type
"Extension"
Two backend routines are added to allow extension to allocate and define
custom wait events, all of these being allocated as of type "Extension":
* WaitEventExtensionNew(), that allocates a wait event ID computed from
a counter in shared memory.
* WaitEventExtensionRegisterName(), to associate a custom string to the
wait event registered.
Note that this includes an example of how to use this new facility in
worker_spi, with tests for various scenarios.
The use of these routines should be extended across more in-core
modules, but this is left as work for future patches.
---
src/include/utils/wait_event.h| 26 +++
src/backend/storage/ipc/ipci.c| 3 +
.../activity/generate-wait_event_types.pl | 7 +-
src/backend/utils/activity/wait_event.c | 178 +-
.../utils/activity/wait_event_names.txt | 2 +-
.../modules/worker_spi/t/001_worker_spi.pl| 34 +++-
.../modules/worker_spi/worker_spi--1.0.sql| 5 +
src/test/modules/worker_spi/worker_spi.c | 104 +-
doc/src/sgml/monitoring.sgml | 11 +-
doc/src/sgml/xfunc.sgml | 45 +
src/tools/pgindent/typedefs.list | 2 +
11 files changed, 393 insertions(+), 24 deletions(-)
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 4517425f84..aad8bc08fa 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -38,6 +38,32 @@ extern void pgstat_reset_wait_event_storage(void);
extern PGDLLIMPORT uint32 *my_wait_event_info;
+/* --
+ * Wait Events - Extension
+ *
+ * Use this category when the server process is waiting for some condition
+ * defined by an extension module.
+ *
+ * Extensions can define their own wait events in this category. First,
+ * they should call WaitEventExtensionNew() to get one or more wait event
+ * IDs that are allocated from a shared counter. These can be used directly
+ * with pgstat_report_wait_start() or equivalent. Next, each individual
+ * process should call WaitEventExtensionRegisterName() to associate a wait
+ * event string to the number allocated previously.
+ */
+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 WaitEventExtensionNew(void);
+extern void WaitEventExtensionRegisterName(uint32 wait_event_info,
+ const char *wait_event_name);
+
/* --
* pgstat_report_wait_start() -
*
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index cc387c00a1..5551afffc0 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -49,6 +49,7 @@
#include "storage/spin.h"
#include "utils/guc.h"
#include "utils/snapmgr.h"
+#include "utils/wait_event.h"
/* GUCs */
int shared_memory_type = DEFAULT_SHARED_MEMORY_TYPE;
@@ -142,6 +143,7 @@ CalculateShmemSize(int *num_semaphores)
size = add_size(size, SyncScanShmemSize());
size = add_size(size, AsyncShmemSize());
size = add_size(size, StatsShmemSize());
+ size = add_size(size, WaitEventExtensionShmemSize());
#ifdef EXEC_BACKEND
size = add_size(size, ShmemBackendArraySize());
#endif
@@ -301,6 +303,7 @@ CreateSharedMemoryAndSemaphores(void)
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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)
- Extensions can add LWLock types to the list
shown in
- . In some cases, the
name
+ Extensions can add Extension and
+ LWLock types
+ to the list shown in
and
+ . In some cases, the name
assigned by an extension will not be available in all server
processes;
- so an LWLock wait event might be reported as
- just extension rather than the
+ so an LWLock or Extension
wait
+ event might be reported as just
+ extension rather than the
extension-assigned name.
I think the order in which they are mentioned should be matched. I mean
that
- so an LWLock or Extension
wait
+ so an Extension or LWLock
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
Re: Support to define custom wait events for extensions
On Mon, Jul 31, 2023 at 6:40 AM Michael Paquier 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 > 0xFF00 and 0x 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
Re: Support to define custom wait events for extensions
On Fri, Jul 28, 2023 at 12:43:36PM +0530, Bharath Rupireddy wrote: > 1. > - so an LWLock wait event might be reported as > - just extension rather than the > - extension-assigned name. > + if the extension's library is not loaded; so a custom wait event might > + be reported as just ??? > + 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 LWLock wait event might be reported as > - just extension rather than the > - extension-assigned name. > > + . 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 ??? > > 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 & 0xFF00) == 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 & 0xFF00) == > 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 0xFF00 and 0x 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
Re: Support to define custom wait events for extensions
On Fri, Jul 28, 2023 at 6:36 AM Michael Paquier 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 LWLock wait event might be reported as
- just extension rather than the
- extension-assigned name.
+ if the extension's library is not loaded; so a custom wait event might
+ be reported as just ???
+ 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 LWLock wait event might be reported as
- just extension rather than the
- extension-assigned name.
+ . 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 ???
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 & 0xFF00) == 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 & 0xFF00) ==
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
Re: Support to define custom wait events for extensions
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(), );
+ 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
From 766d1db13035eefd87a02adb88853f575abe75cb Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 28 Jul 2023 10:01:43 +0900
Subject: [PATCH v9] Support custom wait events for wait event type "Extension"
Two backend routines are added to allow extension to allocate and define
custom wait events, all of these being allocated as of type "Extension":
* WaitEventExtensionNew(), that allocates a wait event ID computed from
a counter in shared memory.
* WaitEventExtensionRegisterName(), to associate a custom string to the
wait event registered.
Note that this includes an example of how to use this new facility in
worker_spi, with tests for various scenarios.
The use of these routines should be extended across more in-core
modules, but this is left as work for future patches.
---
src/include/utils/wait_event.h| 26 +++
src/backend/storage/ipc/ipci.c| 3 +
.../activity/generate-wait_event_types.pl | 7 +-
src/backend/utils/activity/wait_event.c | 171 +-
.../utils/activity/wait_event_names.txt | 2 +-
.../modules/worker_spi/t/001_worker_spi.pl| 31 +++-
.../modules/worker_spi/worker_spi--1.0.sql| 5 +
src/test/modules/worker_spi/worker_spi.c | 104 ++-
doc/src/sgml/monitoring.sgml | 13 +-
doc/src/sgml/xfunc.sgml | 33
10 files changed, 369 insertions(+), 26 deletions(-)
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 4517425f84..3e38215c2d 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -38,6 +38,32 @@ extern void pgstat_reset_wait_event_storage(void);
extern PGDLLIMPORT uint32 *my_wait_event_info;
+/* --
+ * Wait Events - Extension
+ *
+ * Use this category when the server process is waiting for some condition
+ * defined by an extension module.
+ *
+ * Extensions can define their own wait events in this categiry. First,
+ * they should call WaitEventExtensionNew() to get one or more wait event
+ * IDs that are allocated from a shared counter. These can be used directly
+ * with pgstat_report_wait_start() or equivalent. Next, each individual
+ * process should call WaitEventExtensionRegisterName() to associate a wait
+ * event string to the number allocated previously.
+ */
+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 WaitEventExtensionNew(void);
+extern void WaitEventExtensionRegisterName(uint32 wait_event_info,
+ const char *wait_event_name);
+
/* --
* pgstat_report_wait_start() -
*
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index cc387c00a1..5551afffc0 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -49,6 +49,7 @@
#include "storage/spin.h"
#include "utils/guc.h"
#include "utils/snapmgr.h"
+#include "utils/wait_event.h"
/* GUCs */
int shared_memory_type = DEFAULT_SHARED_MEMORY_TYPE;
@@ -142,6 +143,7 @@ CalculateShmemSize(int
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
On Wed, Jul 19, 2023 at 10:57:39AM -0500, Tristan Partin wrote:
> > +
> > +Custom Wait Events for Add-ins
>
> 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
From deb7cab66671e0277cc34236c7515f4ea18fac65 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Wed, 19 Jul 2023 12:28:12 +0900
Subject: [PATCH v8] Support custom wait events for extensions.
To support custom wait events, it add 2 APIs to define new wait events
for extensions dynamically.
The APIs are
* WaitEventExtensionNew()
* WaitEventExtensionRegisterName()
These are similar to the existing LWLockNewTrancheId() and
LWLockRegisterTranche().
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.
Note that this includes an example of how to use this new facility in
worker_spi.
---
src/include/utils/wait_event.h| 25 +++
src/backend/storage/ipc/ipci.c| 3 +
.../activity/generate-wait_event_types.pl | 7 +-
src/backend/utils/activity/wait_event.c | 171 +-
.../utils/activity/wait_event_names.txt | 2 +-
.../modules/worker_spi/t/001_worker_spi.pl| 31 +++-
.../modules/worker_spi/worker_spi--1.0.sql| 5 +
src/test/modules/worker_spi/worker_spi.c | 105 ++-
doc/src/sgml/monitoring.sgml | 13 +-
doc/src/sgml/xfunc.sgml | 34
10 files changed, 370 insertions(+), 26 deletions(-)
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 4517425f84..d405398b75 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -67,6 +67,31 @@ pgstat_report_wait_start(uint32 wait_event_info)
*(volatile uint32 *) my_wait_event_info = wait_event_info;
}
+/* --
+ * Wait Events - Extension
+ *
+ * Use this category when the server process is waiting for some condition
+ * defined by an extension module.
+ *
+ * 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.
+ */
+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 WaitEventExtensionNew(void);
+extern void WaitEventExtensionRegisterName(uint32 wait_event_info,
+ const char *wait_event_name);
+
/* --
* pgstat_report_wait_end() -
*
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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)
Re: Support to define custom wait events for extensions
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 extension + rather than the extension-assigned name. s/an/a + +Custom Wait Events for Add-ins This would be the second use of "Add-ins" ever, according to my search. Should this be "Extensions" instead? + +Add-ins can define custom wait events that the wait event type is s/that/where +Extension. + + +First, add-ins should get new one or more wait events by calling: "one or more" doesn't seem to make sense grammatically here. + +uint32 WaitEventExtensionNew(void) + +Next, each individual process can use them to associate that Remove "that". +a wait event string to the associated name by calling: + +void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name); + +An example can be found in + src/test/modules/test_custom_wait_events/test_custom_wait_events.c +in the PostgreSQL source tree. + + + * 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)
Re: Support to define custom wait events for extensions
On Wed, Jul 19, 2023 at 11:49 AM Masahiro Ikeda 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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom e5d63913cbba9f20098252cc1e415eda4d32f5ad Mon Sep 17 00:00:00 2001 From: Masahiro Ikeda Date: Wed, 19 Jul 2023 12:28:12 +0900 Subject: [PATCH] Support custom wait events for extensions. To support custom wait events, it add 2 APIs to define new wait events for extensions dynamically. The APIs are * WaitEventExtensionNew() * WaitEventExtensionRegisterName() These are similar to the existing LWLockNewTrancheId() and LWLockRegisterTranche(). 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. Note that this includes a test module, which perhaps should be split later into its own commit. --- doc/src/sgml/monitoring.sgml | 10 +- doc/src/sgml/xfunc.sgml | 23 ++ src/backend/storage/ipc/ipci.c| 3 + .../activity/generate-wait_event_types.pl | 7 +- src/backend/utils/activity/wait_event.c | 171 +++- .../utils/activity/wait_event_names.txt | 2 +- src/include/utils/wait_event.h| 25 ++ src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + .../test_custom_wait_events/.gitignore| 4 + .../modules/test_custom_wait_events/Makefile | 23 ++ .../test_custom_wait_events/meson.build | 33 +++ .../test_custom_wait_events/t/001_basic.pl| 137 ++ .../test_custom_wait_events--1.0.sql | 39 +++ .../test_custom_wait_events.c | 253 ++ .../test_custom_wait_events.control | 3 + 16 files changed, 718 insertions(+), 17 deletions(-) create mode 100644 src/test/modules/test_custom_wait_events/.gitignore create mode 100644 src/test/modules/test_custom_wait_events/Makefile create mode 100644 src/test/modules/test_custom_wait_events/meson.build create mode 100644 src/test/modules/test_custom_wait_events/t/001_basic.pl create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events--1.0.sql create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.c create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.control diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 588b720f57..3aee5c243f 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -1117,12 +1117,12 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i - Extensions can add LWLock types to the list shown in - . In some cases, the name + Extensions can add Extension and LWLock types + to the list shown in and + . In some cases, the name assigned by an extension will not be available in all server processes; - so an LWLock wait event might be reported as - just extension rather than the - extension-assigned name. + so an wait event might be reported as just extension + rather than the extension-assigned name. diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 9620ea9ae3..bc210cd03b 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3453,6 +3453,29 @@ if (!ptr) + +Custom Wait Events for Add-ins + + +Add-ins can define custom wait events that the wait event type is +Extension. + + +First, add-ins should get new one or more wait events by calling: + +uint32 WaitEventExtensionNew(void) + +Next, each individual process can use them to associate that +a wait event string to the associated name by calling: + +void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name); + +An example can be found in +src/test/modules/test_custom_wait_events/test_custom_wait_events.c +in the PostgreSQL source tree. + + + Using C++ for Extensibility diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index cc387c00a1..5551afffc0 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -49,6 +49,7 @@ #include "storage/spin.h" #include "utils/guc.h" #include "utils/snapmgr.h" +#include "utils/wait_event.h" /* GUCs */ int shared_memory_type = DEFAULT_SHARED_MEMORY_TYPE; @@ -142,6 +143,7 @@ CalculateShmemSize(int *num_semaphores) size = add_size(size, SyncScanShmemSize()); size = add_size(size, AsyncShmemSize()); size = add_size(size, StatsShmemSize()); + size = add_size(size,
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom 9c5c1f1f4f0be87cc06c7127396e7d228b665b8a Mon Sep 17 00:00:00 2001 From: Masahiro Ikeda Date: Wed, 19 Jul 2023 12:28:12 +0900 Subject: [PATCH] Support custom wait events for extensions. To support custom wait events, it add 2 APIs to define new wait events for extensions dynamically. The APIs are * WaitEventExtensionNew() * WaitEventExtensionRegisterName() These are similar to the existing LWLockNewTrancheId() and LWLockRegisterTranche(). 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. Note that this includes a test module, which perhaps should be split later into its own commit. --- doc/src/sgml/monitoring.sgml | 10 +- doc/src/sgml/xfunc.sgml | 23 ++ src/backend/storage/ipc/ipci.c| 3 + .../activity/generate-wait_event_types.pl | 7 +- src/backend/utils/activity/wait_event.c | 171 +++- .../utils/activity/wait_event_names.txt | 2 +- src/include/utils/wait_event.h| 25 ++ src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + .../test_custom_wait_events/.gitignore| 4 + .../modules/test_custom_wait_events/Makefile | 23 ++ .../test_custom_wait_events/meson.build | 33 +++ .../test_custom_wait_events/t/001_basic.pl| 137 ++ .../test_custom_wait_events--1.0.sql | 39 +++ .../test_custom_wait_events.c | 253 ++ .../test_custom_wait_events.control | 3 + 16 files changed, 718 insertions(+), 17 deletions(-) create mode 100644 src/test/modules/test_custom_wait_events/.gitignore create mode 100644 src/test/modules/test_custom_wait_events/Makefile create mode 100644 src/test/modules/test_custom_wait_events/meson.build create mode 100644 src/test/modules/test_custom_wait_events/t/001_basic.pl create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events--1.0.sql create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.c create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.control diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 588b720f57..3aee5c243f 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -1117,12 +1117,12 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i - Extensions can add LWLock types to the list shown in - . In some cases, the name + Extensions can add Extension and LWLock types + to the list shown in and + . In some cases, the name assigned by an extension will not be available in all server processes; - so an LWLock wait event might be reported as - just extension rather than the - extension-assigned name. + so an wait event might be reported as just extension + rather than the extension-assigned name. diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 9620ea9ae3..bc210cd03b 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3453,6 +3453,29 @@ if (!ptr) + +Custom Wait Events for Add-ins + + +Add-ins can define custom wait events that the wait event type is +Extension. + + +First, add-ins should get new one or more wait events by calling: + +uint32 WaitEventExtensionNew(void) + +Next, each individual process can use them to associate that +a wait event string to the associated name by calling: + +void WaitEventExtensionRegisterName(uint32 wait_event_info, const char *wait_event_name); + +An example can be found in
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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 & 0xFF00) == PG_WAIT_EXTENSION);
+
+ eventId = wait_event_info & 0x;
+
+ /* 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;
+
Re: Support to define custom wait events for extensions
On 2023-07-12 02:39, Tristan Partin wrote:
From bf06b8100cb747031959fe81a2d19baabc4838cf Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
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
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom 5198fc6302a3bf4232252b23b45d39d987e736bc Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Wed, 12 Jul 2023 16:28:27 +0900
Subject: [PATCH] change the die condition in generate-wait_event_types.pl
---
src/backend/utils/activity/generate-wait_event_types.pl | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl
index 6d1a2af42a..fbe011039a 100644
--- a/src/backend/utils/activity/generate-wait_event_types.pl
+++ b/src/backend/utils/activity/generate-wait_event_types.pl
@@ -89,9 +89,8 @@ foreach my $line (@lines_sorted)
# any C and header files generated.
die "wait event names must start with 'WAIT_EVENT_'"
if ( $trimmedwaiteventname eq $waiteventenumname
- && $waiteventenumname !~ /^LWLock/
- && $waiteventenumname !~ /^Lock/);
- $continue = ",\n";
+ && $waitclassname !~ /^WaitEventLWLock$/
+ && $waitclassname !~ /^WaitEventLock$/);
push(@{ $hashwe{$waitclassname} }, @waiteventlist);
}
--
2.25.1
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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 & 0xFF00) == PG_WAIT_EXTENSION);
> +
> + eventId = wait_event_info & 0x;
> +
> + /* 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'"
> +);
>
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
> From bf06b8100cb747031959fe81a2d19baabc4838cf Mon Sep 17 00:00:00 2001
> From: Masahiro Ikeda
> 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)
Re: Support to define custom wait events for extensions
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 From 630f78822534171bb8839c79d0fdca3bb8268a95 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 11 Jul 2023 16:41:35 +0900 Subject: [PATCH v5] Support custom wait events for extensions. To support custom wait events, it add 2 APIs to allocate new wait events dynamically. The APIs are * ExtensionWaitEventNewTranche() * ExtensionWaitEventRegisterTranche() These are similar to the existing LWLockNewTrancheId() and LWLockRegisterTranche(). First, extension should call ExtensionWaitEventNewTranche() to get one or more wait event "tranches", which are ID is allocated from a shared counter. Next, each individual process can use the tranche numbers with ExtensionWaitEventRegisterTranche() to associate that a wait event string to the associated name. Note that this includes a test module, which perhaps should be split later into its own commit. --- src/include/utils/wait_event.h| 31 +++ src/backend/storage/ipc/ipci.c| 3 + src/backend/storage/ipc/shmem.c | 3 +- .../activity/generate-wait_event_types.pl | 12 +- src/backend/utils/activity/wait_event.c | 149 +- .../utils/activity/wait_event_names.txt | 2 +- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + .../test_custom_wait_events/.gitignore| 2 + .../modules/test_custom_wait_events/Makefile | 23 +++ .../test_custom_wait_events/meson.build | 33 +++ .../test_custom_wait_events/t/001_basic.pl| 34 .../test_custom_wait_events--1.0.sql | 14 ++ .../test_custom_wait_events.c | 188 ++ .../test_custom_wait_events.control | 3 + 15 files changed, 484 insertions(+), 15 deletions(-) create mode 100644 src/test/modules/test_custom_wait_events/.gitignore create mode 100644 src/test/modules/test_custom_wait_events/Makefile create mode 100644 src/test/modules/test_custom_wait_events/meson.build create mode 100644 src/test/modules/test_custom_wait_events/t/001_basic.pl create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events--1.0.sql create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.c create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.control diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h index 4517425f84..dfcaf8c506 100644 --- a/src/include/utils/wait_event.h +++ b/src/include/utils/wait_event.h @@ -67,6 +67,37 @@ pgstat_report_wait_start(uint32 wait_event_info) *(volatile uint32 *) my_wait_event_info = wait_event_info; } +/* -- + * 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. + * + * 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
Re: Support to define custom wait events for extensions
Hi,
I updated the patches to handle the warning mentioned
by PostgreSQL Patch Tester, and removed unnecessary spaces.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATIONFrom 1bb78fa2cbe6b030cea7a570bec88bd4d68f314a Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Fri, 23 Jun 2023 17:38:38 +0900
Subject: [PATCH 2/2] Add test codes for custom wait events
---
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
.../test_custom_wait_events/.gitignore| 2 +
.../modules/test_custom_wait_events/Makefile | 23 +++
.../test_custom_wait_events/meson.build | 33
.../test_custom_wait_events/t/001_basic.pl| 34
.../test_custom_wait_events--1.0.sql | 14 ++
.../test_custom_wait_events.c | 182 ++
.../test_custom_wait_events.control | 3 +
9 files changed, 293 insertions(+)
create mode 100644 src/test/modules/test_custom_wait_events/.gitignore
create mode 100644 src/test/modules/test_custom_wait_events/Makefile
create mode 100644 src/test/modules/test_custom_wait_events/meson.build
create mode 100644 src/test/modules/test_custom_wait_events/t/001_basic.pl
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events--1.0.sql
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.c
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.control
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 6331c976dc..84312b7e57 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -17,6 +17,7 @@ SUBDIRS = \
test_bloomfilter \
test_copy_callbacks \
test_custom_rmgrs \
+ test_custom_wait_events \
test_ddl_deparse \
test_extensions \
test_ginpostinglist \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 17d369e378..83a1d8fd19 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -14,6 +14,7 @@ subdir('ssl_passphrase_callback')
subdir('test_bloomfilter')
subdir('test_copy_callbacks')
subdir('test_custom_rmgrs')
+subdir('test_custom_wait_events')
subdir('test_ddl_deparse')
subdir('test_extensions')
subdir('test_ginpostinglist')
diff --git a/src/test/modules/test_custom_wait_events/.gitignore b/src/test/modules/test_custom_wait_events/.gitignore
new file mode 100644
index 00..716e17f5a2
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/.gitignore
@@ -0,0 +1,2 @@
+# Generated subdirectories
+/tmp_check/
diff --git a/src/test/modules/test_custom_wait_events/Makefile b/src/test/modules/test_custom_wait_events/Makefile
new file mode 100644
index 00..dda365d523
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_custom_wait_events/Makefile
+
+MODULE_big = test_custom_wait_events
+OBJS = \
+ $(WIN32RES) \
+ test_custom_wait_events.o
+PGFILEDESC = "test_custom_wait_events - test custom wait events"
+
+EXTENSION = test_custom_wait_events
+DATA = test_custom_wait_events--1.0.sql
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_custom_wait_events
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_custom_wait_events/meson.build b/src/test/modules/test_custom_wait_events/meson.build
new file mode 100644
index 00..8ad073e577
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) PostgreSQL Global Development Group
+
+test_custom_wait_events = files(
+ 'test_custom_wait_events.c',
+)
+
+if host_system == 'windows'
+ test_custom_wait_events += rc_lib_gen.process(win32ver_rc, extra_args: [
+'--NAME', 'test_custom_wait_events',
+'--FILEDESC', 'test_custom_wait_events - test custom wait events',])
+endif
+
+test_custom_wait_events = shared_module('test_custom_wait_events',
+ test_custom_wait_events,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += test_custom_wait_events
+
+test_install_data += files(
+ 'test_custom_wait_events.control',
+ 'test_custom_wait_events--1.0.sql',
+)
+
+tests += {
+ 'name': 'test_custom_wait_events',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+'tests': [
+ 't/001_basic.pl',
+],
+ },
+}
diff --git a/src/test/modules/test_custom_wait_events/t/001_basic.pl b/src/test/modules/test_custom_wait_events/t/001_basic.pl
new file mode 100644
index 00..b43c82a713
--- /dev/null
+++ 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 =
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom 901b75d31070ef0029557db6981c98e06f5c16c3 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Tue, 20 Jun 2023 17:59:34 +0900
Subject: [PATCH 2/2] Add test codes for custom wait events
---
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
.../test_custom_wait_events/.gitignore| 2 +
.../modules/test_custom_wait_events/Makefile | 23 +++
.../test_custom_wait_events/meson.build | 33
.../test_custom_wait_events/t/001_basic.pl| 34
.../test_custom_wait_events--1.0.sql | 14 ++
.../test_custom_wait_events.c | 182 ++
.../test_custom_wait_events.control | 3 +
9 files changed, 293 insertions(+)
create mode 100644 src/test/modules/test_custom_wait_events/.gitignore
create mode 100644 src/test/modules/test_custom_wait_events/Makefile
create mode 100644 src/test/modules/test_custom_wait_events/meson.build
create mode 100644 src/test/modules/test_custom_wait_events/t/001_basic.pl
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events--1.0.sql
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.c
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.control
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 6331c976dc..84312b7e57 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -17,6 +17,7 @@ SUBDIRS = \
test_bloomfilter \
test_copy_callbacks \
test_custom_rmgrs \
+ test_custom_wait_events \
test_ddl_deparse \
test_extensions \
test_ginpostinglist \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 17d369e378..83a1d8fd19 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -14,6 +14,7 @@ subdir('ssl_passphrase_callback')
subdir('test_bloomfilter')
subdir('test_copy_callbacks')
subdir('test_custom_rmgrs')
+subdir('test_custom_wait_events')
subdir('test_ddl_deparse')
subdir('test_extensions')
subdir('test_ginpostinglist')
diff --git a/src/test/modules/test_custom_wait_events/.gitignore b/src/test/modules/test_custom_wait_events/.gitignore
new file mode 100644
index 00..716e17f5a2
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/.gitignore
@@ -0,0 +1,2 @@
+# Generated subdirectories
+/tmp_check/
diff --git a/src/test/modules/test_custom_wait_events/Makefile b/src/test/modules/test_custom_wait_events/Makefile
new file mode 100644
index 00..dda365d523
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_custom_wait_events/Makefile
+
+MODULE_big = test_custom_wait_events
+OBJS = \
+ $(WIN32RES) \
+ test_custom_wait_events.o
+PGFILEDESC = "test_custom_wait_events - test custom wait events"
+
+EXTENSION = test_custom_wait_events
+DATA = test_custom_wait_events--1.0.sql
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_custom_wait_events
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_custom_wait_events/meson.build b/src/test/modules/test_custom_wait_events/meson.build
new file mode 100644
index 00..8ad073e577
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) PostgreSQL Global Development Group
+
+test_custom_wait_events = files(
+ 'test_custom_wait_events.c',
+)
+
+if host_system == 'windows'
+ test_custom_wait_events += rc_lib_gen.process(win32ver_rc, extra_args: [
+'--NAME', 'test_custom_wait_events',
+'--FILEDESC', 'test_custom_wait_events - test custom wait events',])
+endif
+
+test_custom_wait_events = shared_module('test_custom_wait_events',
+ test_custom_wait_events,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += test_custom_wait_events
+
+test_install_data += files(
+ 'test_custom_wait_events.control',
+ 'test_custom_wait_events--1.0.sql',
+)
+
+tests += {
+ 'name': 'test_custom_wait_events',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+
Re: Support to define custom wait events for extensions
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 CORPORATIONFrom 206ce9e1d74d1449b50cfc765936172de98687e8 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
Date: Tue, 20 Jun 2023 17:59:34 +0900
Subject: [PATCH 2/2] Add test codes for custom wait events
---
src/test/modules/Makefile | 1 +
.../test_custom_wait_events/.gitignore| 2 +
.../modules/test_custom_wait_events/Makefile | 23 +++
.../test_custom_wait_events/meson.build | 33
.../test_custom_wait_events/t/001_basic.pl| 34
.../test_custom_wait_events--1.0.sql | 14 ++
.../test_custom_wait_events.c | 182 ++
.../test_custom_wait_events.control | 3 +
8 files changed, 292 insertions(+)
create mode 100644 src/test/modules/test_custom_wait_events/.gitignore
create mode 100644 src/test/modules/test_custom_wait_events/Makefile
create mode 100644 src/test/modules/test_custom_wait_events/meson.build
create mode 100644 src/test/modules/test_custom_wait_events/t/001_basic.pl
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events--1.0.sql
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.c
create mode 100644 src/test/modules/test_custom_wait_events/test_custom_wait_events.control
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 6331c976dc..84312b7e57 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -17,6 +17,7 @@ SUBDIRS = \
test_bloomfilter \
test_copy_callbacks \
test_custom_rmgrs \
+ test_custom_wait_events \
test_ddl_deparse \
test_extensions \
test_ginpostinglist \
diff --git a/src/test/modules/test_custom_wait_events/.gitignore b/src/test/modules/test_custom_wait_events/.gitignore
new file mode 100644
index 00..716e17f5a2
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/.gitignore
@@ -0,0 +1,2 @@
+# Generated subdirectories
+/tmp_check/
diff --git a/src/test/modules/test_custom_wait_events/Makefile b/src/test/modules/test_custom_wait_events/Makefile
new file mode 100644
index 00..dda365d523
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_custom_wait_events/Makefile
+
+MODULE_big = test_custom_wait_events
+OBJS = \
+ $(WIN32RES) \
+ test_custom_wait_events.o
+PGFILEDESC = "test_custom_wait_events - test custom wait events"
+
+EXTENSION = test_custom_wait_events
+DATA = test_custom_wait_events--1.0.sql
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_custom_wait_events
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_custom_wait_events/meson.build b/src/test/modules/test_custom_wait_events/meson.build
new file mode 100644
index 00..da2b720f8f
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2022-2023, PostgreSQL Global Development Group
+
+test_custom_wait_events = files(
+ 'test_custom_wait_events.c',
+)
+
+if host_system == 'windows'
+ test_custom_wait_events += rc_lib_gen.process(win32ver_rc, extra_args: [
+'--NAME', 'test_custom_wait_events',
+'--FILEDESC', 'test_custom_wait_events - test custom wait events',])
+endif
+
+test_custom_wait_events = shared_module('test_custom_wait_events',
+ test_custom_wait_events,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += test_custom_wait_events
+
+test_install_data += files(
+ 'test_custom_wait_events.control',
+ 'test_custom_wait_events--1.0.sql',
+)
+
+tests += {
+ 'name': 'test_custom_wait_events',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+'tests': [
+ 't/001_basic.pl',
+],
+ },
+}
diff --git a/src/test/modules/test_custom_wait_events/t/001_basic.pl b/src/test/modules/test_custom_wait_events/t/001_basic.pl
new file mode 100644
index 00..59ba14919b
--- /dev/null
+++ b/src/test/modules/test_custom_wait_events/t/001_basic.pl
@@ -0,0 +1,34 @@
+# Copyright (c) 2021-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;
+
+# setup
+$node->safe_psql('postgres', 'CREATE EXTENSION
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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)
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
On 2023-06-16 01:13, Tristan Partin wrote:
We had this on our list of things to do at Neon, so it is a nice
surprise that you brought up an initial patchset :). It was also my
first time looking up the word tranche.
What a coincidence! I came up with the idea when I used Neon with
postgres_fdw. As a Neon user, I also feel the feature is important.
Same as you. Thanks to Michael and Drouvot, I got to know the word
tranche
and the related existing code.
From 59a118402e5e59685fb9e0fb086872e25a405736 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
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 abou
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
Re: Support to define custom wait events for extensions
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
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
Date: Thu, 15 Jun 2023 13:16:00 +0900
Subject: [PATCH 3/3] Add docs to define custom wait events
> +
> + wait events are reserved by calling:
> +
> +void RequestNamedExtensionWaitEventTranche(const char *tranche_name)
> +
> + from your shmem_request_hook. This will ensure that
> + wait event is available under the name tranche_name,
> + which the wait event type is Extension.
> + Use GetNamedExtensionWaitEventTranche
> + to get a wait event information
Re: Support to define custom wait events for extensions
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
Re: Support to define custom wait events for extensions
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 signature.asc Description: PGP signature
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 CORPORATIONFrom 59a118402e5e59685fb9e0fb086872e25a405736 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda
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.
This commit allows defining custom wait events for extensions and
introduces a new API called RequestNamedExtensionWaitEventTranche()/
GetNamedExtensionWaitEventTranche().
These refer to RequestNamedLWLockTranche()/GetNamedLWLockTranche(),
but do not require as much flexibility as LWLock and can be implemented
more simply.
The extension 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.
---
src/backend/postmaster/postmaster.c | 6 +
src/backen
