pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedIntroduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
25+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2023-07-03 2aeaf80e57 Refactor some code related to wait events "BufferPin" and "Extension"
* Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-15 08:07 Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
0 siblings, 2 replies; 25+ messages in thread
From: Drouvot, Bertrand @ 2023-05-15 08:07 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi hackers,
Please find attached a patch to $SUBJECT.
This is preliminary work to autogenerate some wait events
code and documentation done in [1].
The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
and WAIT_EVENT_BUFFER_PIN) and their associated functions
(pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
Please note that that would not break extensions outside contrib
that make use of the existing PG_WAIT_EXTENSION.
[1]: https://www.postgresql.org/message-id/flat/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98@gmail.com
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Introducing-WAIT_EVENT_EXTENSION-and-WAIT_EVENT_B.patch (text/plain, 10.0K) download
From b48b0189067b6ce799636e469a10b0e265ff4473 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Sat, 13 May 2023 07:59:08 +0000
Subject: [PATCH v1] Introducing WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
---
contrib/dblink/dblink.c | 4 +-
contrib/pg_prewarm/autoprewarm.c | 4 +-
contrib/postgres_fdw/connection.c | 6 +--
src/backend/storage/buffer/bufmgr.c | 2 +-
src/backend/storage/ipc/standby.c | 2 +-
src/backend/utils/activity/wait_event.c | 66 +++++++++++++++++++++---
src/include/utils/wait_event.h | 20 ++++++-
src/test/modules/test_shm_mq/setup.c | 2 +-
src/test/modules/test_shm_mq/test.c | 2 +-
src/test/modules/worker_spi/worker_spi.c | 2 +-
10 files changed, 91 insertions(+), 19 deletions(-)
8.9% contrib/dblink/
4.7% contrib/pg_prewarm/
9.4% contrib/postgres_fdw/
3.4% src/backend/storage/buffer/
3.3% src/backend/storage/ipc/
48.6% src/backend/utils/activity/
14.4% src/include/utils/
4.6% src/test/modules/test_shm_mq/
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 55f75eff36..f167cb71d4 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -203,7 +203,7 @@ dblink_get_conn(char *conname_or_str,
dblink_connstr_check(connstr);
/* OK to make connection */
- conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
+ conn = libpqsrv_connect(connstr, WAIT_EVENT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
@@ -293,7 +293,7 @@ dblink_connect(PG_FUNCTION_ARGS)
dblink_connstr_check(connstr);
/* OK to make connection */
- conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
+ conn = libpqsrv_connect(connstr, WAIT_EVENT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 93835449c0..d0efc9e524 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -237,7 +237,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
}
else
{
@@ -264,7 +264,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
delay_in_ms,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
}
/* Reset the latch, loop. */
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index da32d503bc..25d0c43b64 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -530,7 +530,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
/* OK to make connection */
conn = libpqsrv_connect_params(keywords, values,
false, /* expand_dbname */
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
if (!conn || PQstatus(conn) != CONNECTION_OK)
ereport(ERROR,
@@ -863,7 +863,7 @@ pgfdw_get_result(PGconn *conn, const char *query)
WL_LATCH_SET | WL_SOCKET_READABLE |
WL_EXIT_ON_PM_DEATH,
PQsocket(conn),
- -1L, PG_WAIT_EXTENSION);
+ -1L, WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
@@ -1567,7 +1567,7 @@ pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime, PGresult **result,
WL_LATCH_SET | WL_SOCKET_READABLE |
WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
PQsocket(conn),
- cur_timeout, PG_WAIT_EXTENSION);
+ cur_timeout, WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 1fa689052e..c7c1782461 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4895,7 +4895,7 @@ LockBufferForCleanup(Buffer buffer)
SetStartupBufferPinWaitBufId(-1);
}
else
- ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
+ ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);
/*
* Remove flag marking us as waiter. Normally this will not be set
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index ffe5e1563f..33188f829e 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -840,7 +840,7 @@ ResolveRecoveryConflictWithBufferPin(void)
* SIGHUP signal handler, etc cannot do that because it uses the different
* latch from that ProcWaitForSignal() waits on.
*/
- ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
+ ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);
if (got_standby_delay_timeout)
SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index 7940d64639..8572cf169e 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -28,7 +28,9 @@
static const char *pgstat_get_wait_activity(WaitEventActivity w);
+static const char *pgstat_get_wait_bufferpin(WaitEventBufferPin w);
static const char *pgstat_get_wait_client(WaitEventClient w);
+static const char *pgstat_get_wait_extension(WaitEventExtension w);
static const char *pgstat_get_wait_ipc(WaitEventIPC w);
static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
static const char *pgstat_get_wait_io(WaitEventIO w);
@@ -90,7 +92,7 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
case PG_WAIT_LOCK:
event_type = "Lock";
break;
- case PG_WAIT_BUFFER_PIN:
+ case PG_WAIT_BUFFERPIN:
event_type = "BufferPin";
break;
case PG_WAIT_ACTIVITY:
@@ -147,9 +149,13 @@ pgstat_get_wait_event(uint32 wait_event_info)
case PG_WAIT_LOCK:
event_name = GetLockNameFromTagType(eventId);
break;
- case PG_WAIT_BUFFER_PIN:
- event_name = "BufferPin";
- break;
+ case PG_WAIT_BUFFERPIN:
+ {
+ WaitEventBufferPin w = (WaitEventBufferPin) wait_event_info;
+
+ event_name = pgstat_get_wait_bufferpin(w);
+ break;
+ }
case PG_WAIT_ACTIVITY:
{
WaitEventActivity w = (WaitEventActivity) wait_event_info;
@@ -165,8 +171,12 @@ pgstat_get_wait_event(uint32 wait_event_info)
break;
}
case PG_WAIT_EXTENSION:
- event_name = "Extension";
- break;
+ {
+ WaitEventExtension w = (WaitEventExtension) wait_event_info;
+
+ event_name = pgstat_get_wait_extension(w);
+ break;
+ }
case PG_WAIT_IPC:
{
WaitEventIPC w = (WaitEventIPC) wait_event_info;
@@ -254,6 +264,28 @@ pgstat_get_wait_activity(WaitEventActivity w)
return event_name;
}
+/* ----------
+ * pgstat_get_wait_bufferpin() -
+ *
+ * Convert WaitEventBufferPin to string.
+ * ----------
+ */
+static const char *
+pgstat_get_wait_bufferpin(WaitEventBufferPin w)
+{
+ const char *event_name = "unknown wait event";
+
+ switch (w)
+ {
+ case WAIT_EVENT_BUFFER_PIN:
+ event_name = "BufferPin";
+ break;
+ /* no default case, so that compiler will warn */
+ }
+
+ return event_name;
+}
+
/* ----------
* pgstat_get_wait_client() -
*
@@ -297,6 +329,28 @@ pgstat_get_wait_client(WaitEventClient w)
return event_name;
}
+/* ----------
+ * pgstat_get_wait_extension() -
+ *
+ * Convert WaitEventExtension to string.
+ * ----------
+ */
+static const char *
+pgstat_get_wait_extension(WaitEventExtension w)
+{
+ const char *event_name = "unknown wait event";
+
+ switch (w)
+ {
+ case WAIT_EVENT_EXTENSION:
+ event_name = "Extension";
+ break;
+ /* no default case, so that compiler will warn */
+ }
+
+ return event_name;
+}
+
/* ----------
* pgstat_get_wait_ipc() -
*
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 518d3b0a1f..dc01d4e84d 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -17,7 +17,7 @@
*/
#define PG_WAIT_LWLOCK 0x01000000U
#define PG_WAIT_LOCK 0x03000000U
-#define PG_WAIT_BUFFER_PIN 0x04000000U
+#define PG_WAIT_BUFFERPIN 0x04000000U
#define PG_WAIT_ACTIVITY 0x05000000U
#define PG_WAIT_CLIENT 0x06000000U
#define PG_WAIT_EXTENSION 0x07000000U
@@ -50,6 +50,15 @@ typedef enum
WAIT_EVENT_WAL_WRITER_MAIN
} WaitEventActivity;
+/* ----------
+ * Wait Events - BUFFERPIN
+ * ----------
+ */
+typedef enum
+{
+ WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFERPIN
+} WaitEventBufferPin;
+
/* ----------
* Wait Events - Client
*
@@ -70,6 +79,15 @@ typedef enum
WAIT_EVENT_WAL_SENDER_WRITE_DATA,
} WaitEventClient;
+/* ----------
+ * Wait Events - EXTENSION
+ * ----------
+ */
+typedef enum
+{
+ WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION
+} WaitEventExtension;
+
/* ----------
* Wait Events - IPC
*
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index bec5732e87..192e5cc2ab 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -280,7 +280,7 @@ wait_for_workers_to_become_ready(worker_state *wstate,
/* Wait to be signaled. */
(void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
/* Reset the latch so we don't spin. */
ResetLatch(MyLatch);
diff --git a/src/test/modules/test_shm_mq/test.c b/src/test/modules/test_shm_mq/test.c
index 906e943e2d..d9be703350 100644
--- a/src/test/modules/test_shm_mq/test.c
+++ b/src/test/modules/test_shm_mq/test.c
@@ -232,7 +232,7 @@ test_shm_mq_pipelined(PG_FUNCTION_ARGS)
* for us to do.
*/
(void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
}
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index ad491d7722..7227cfaa45 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -199,7 +199,7 @@ worker_spi_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
worker_spi_naptime * 1000L,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
--
2.34.1
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-15 18:29 Andres Freund <andres@anarazel.de>
parent: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Andres Freund @ 2023-05-15 18:29 UTC (permalink / raw)
To: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2023-05-15 10:07:04 +0200, Drouvot, Bertrand wrote:
> Please find attached a patch to $SUBJECT.
>
> This is preliminary work to autogenerate some wait events
> code and documentation done in [1].
>
> The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
> and WAIT_EVENT_BUFFER_PIN) and their associated functions
> (pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
>
> Please note that that would not break extensions outside contrib
> that make use of the existing PG_WAIT_EXTENSION.
>
> [1]: https://www.postgresql.org/message-id/flat/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98@gmail.com
>
> Looking forward to your feedback,
Without an explanation for why this change is needed for [1], it's hard to
give useful feedback...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-15 22:30 Michael Paquier <michael@paquier.xyz>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2023-05-15 22:30 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, May 15, 2023 at 11:29:56AM -0700, Andres Freund wrote:
> Without an explanation for why this change is needed for [1], it's hard to
> give useful feedback...
The point is to integrate the wait event classes for buffer pin and
extension into the txt file that automates the creation of the SGML
and C code associated to them. Doing the refactoring of this patch
has two advantages:
- Being able to easily organize the tables for each wait event type
alphabetically, the same way as HEAD, for all wait event classes.
- Minimizing the number of exception rules needed in the perl script
that transforms the txt file into SGML and the C code to list all the
events associated in a class, where one function is used for each wait
event class. Currently the wait event class extension does not use
that.
This impacts only the internal object names, not the wait event
strings or the class associated to them. So this does not change the
contents of pg_stat_activity.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-16 00:17 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 25+ messages in thread
From: Andres Freund @ 2023-05-16 00:17 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2023-05-16 07:30:54 +0900, Michael Paquier wrote:
> On Mon, May 15, 2023 at 11:29:56AM -0700, Andres Freund wrote:
> > Without an explanation for why this change is needed for [1], it's hard to
> > give useful feedback...
>
> The point is to integrate the wait event classes for buffer pin and
> extension into the txt file that automates the creation of the SGML
> and C code associated to them.
IMO the submission should include why automating requires these changes (yours
doesn't really either). I can probably figure it out if I stare a bit at the
code and read the other thread - but I shouldn't need to.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-16 00:38 Michael Paquier <michael@paquier.xyz>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2023-05-16 00:38 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:
> IMO the submission should include why automating requires these changes (yours
> doesn't really either). I can probably figure it out if I stare a bit at the
> code and read the other thread - but I shouldn't need to.
Hm? My previous message includes two reasons.. Anyway, I assume that
my previous message is also missing the explanation from the other
thread that this is to translate a .txt file shaped similarly to
errcodes.txt for the wait events (sections as wait event classes,
listing the elements) into automatically-generated SGML and C code :)
The extensions and buffer pin parts need a few internal tweaks to make
the other changes much easier to do, which is what the patch of this
thread is doing.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-16 01:01 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 25+ messages in thread
From: Andres Freund @ 2023-05-16 01:01 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2023-05-16 09:38:54 +0900, Michael Paquier wrote:
> On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:
> > IMO the submission should include why automating requires these changes (yours
> > doesn't really either). I can probably figure it out if I stare a bit at the
> > code and read the other thread - but I shouldn't need to.
>
> Hm? My previous message includes two reasons..
It explained the motivation, but not why that requires the specific
changes. At least not in a way that I could quickly undestand.
> The extensions and buffer pin parts need a few internal tweaks to make
> the other changes much easier to do, which is what the patch of this
> thread is doing.
Why those tweaks are necessary is precisely what I am asking for.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-16 05:14 Michael Paquier <michael@paquier.xyz>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 3 replies; 25+ messages in thread
From: Michael Paquier @ 2023-05-16 05:14 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, May 15, 2023 at 06:01:02PM -0700, Andres Freund wrote:
> On 2023-05-16 09:38:54 +0900, Michael Paquier wrote:
>> On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:
>>> IMO the submission should include why automating requires these changes (yours
>>> doesn't really either). I can probably figure it out if I stare a bit at the
>>> code and read the other thread - but I shouldn't need to.
>>
>> Hm? My previous message includes two reasons..
>
> It explained the motivation, but not why that requires the specific
> changes. At least not in a way that I could quickly undestand.
>
>> The extensions and buffer pin parts need a few internal tweaks to make
>> the other changes much easier to do, which is what the patch of this
>> thread is doing.
>
> Why those tweaks are necessary is precisely what I am asking for.
Not sure how to answer to that without copy-pasting some code, and
that's what is written in the patch. Anyway, I am used to the context
of what's wanted, so here you go with more details. As a whole, the
patch reshapes the code to be more consistent for all the wait event
classes, so as it is possible to generate the code refactored by the
patch of this thread in a single way for all the classes.
The first change is related to the functions associated to each class,
used to fetch a specific wait event. On HEAD, all the wait event classes
use a dedicated function (activity, IPC, etc.) like that:
case PG_WAIT_BLAH:
{
WaitEventBlah w = (WaitEventBlah) wait_event_info;
event_name = pgstat_get_wait_blah(w);
break;
}
There are two exceptions to that, the wait event classes for extension
and buffer pin, that just do that because these classes have only one
wait event currently:
case PG_WAIT_EXTENSION:
event_name = "Extension";
break
[...]
case PG_WAIT_BUFFER_PIN:
event_name = "BufferPin";
break;
The first thing changed is to introduce two new functions for these
two classes, to work the same way as the other classes. The script in
charge of generating the code from the wait event .txt file will just
build the internals of these functions.
The second change is to rework the enum structures for extension and
buffer pin, to be consistent with the other classes, so as all the
classes have structures shaped like that:
typedef enum
{
WAIT_EVENT_1 = PG_WAIT_BLAH,
[...]
WAIT_EVENT_N
} WaitEventBlah;
Then the perl script generates the same structures for all the wait
event classes, with all the events associated to that. There may be a
point in keeping extension and buffer pin out of this refactoring, but
reworking these like that moves in a single place all the wait event
definitions, making future additions easier. Buffer pin actually
needed a small rename to stick with the naming rules of the other
classes.
These are the two things refactored in the patch, explaining the what.
The reason behind the why is to make the script in charge of
generating all these structures and functions consistent for all the
wait event classes, simply. Treating all the wait event classes
together eases greatly the generation of the documentation, so that it
is possible to enforce an ordering of the tables of each class used to
list each wait event type attached to them. Does this explanation
make sense?
Lock and LWLock are funkier because of the way they grab wait events
for the inner function, still these had better have their
documentation generated so as all the SGML tables created for all the
wait event tables are ordered alphabetically, in the same way as
HEAD.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-16 05:38 Kirk Wolak <wolakk@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
2 siblings, 0 replies; 25+ messages in thread
From: Kirk Wolak @ 2023-05-16 05:38 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, May 16, 2023 at 1:14 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, May 15, 2023 at 06:01:02PM -0700, Andres Freund wrote:
> > Why those tweaks are necessary is precisely what I am asking for.
>
> Then the perl script generates the same structures for all the wait
> event classes, with all the events associated to that. There may be a
> point in keeping extension and buffer pin out of this refactoring, but
> reworking these like that moves in a single place all the wait event
> definitions, making future additions easier. Buffer pin actually
> needed a small rename to stick with the naming rules of the other
> classes.
>
+1 (Nice explanation, Improving things, Consistency. Always good)
>
> These are the two things refactored in the patch, explaining the what.
> The reason behind the why is to make the script in charge of
> generating all these structures and functions consistent for all the
> wait event classes, simply. Treating all the wait event classes
> together eases greatly the generation of the documentation, so that it
> is possible to enforce an ordering of the tables of each class used to
> list each wait event type attached to them. Does this explanation
> make sense?
>
+1 (To me, but I am not important. But having this saved in the thread!!!)
>
> Lock and LWLock are funkier because of the way they grab wait events
> for the inner function, still these had better have their
> documentation generated so as all the SGML tables created for all the
> wait event tables are ordered alphabetically, in the same way as
> HEAD.
> --
> Michael
>
+1 (Whatever helps automate/generate the docs)
Kirk...
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-16 06:10 Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
2 siblings, 0 replies; 25+ messages in thread
From: Drouvot, Bertrand @ 2023-05-16 06:10 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Andres Freund <andres@anarazel.de>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 5/16/23 7:14 AM, Michael Paquier wrote:
> On Mon, May 15, 2023 at 06:01:02PM -0700, Andres Freund wrote:
>> On 2023-05-16 09:38:54 +0900, Michael Paquier wrote:
>>> On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:
> These are the two things refactored in the patch, explaining the what.
> The reason behind the why is to make the script in charge of
> generating all these structures and functions consistent for all the
> wait event classes, simply. Treating all the wait event classes
> together eases greatly the generation of the documentation, so that it
> is possible to enforce an ordering of the tables of each class used to
> list each wait event type attached to them.
Right, it does "fix" the ordering issue (for BufferPin and Extension)
that I've described in the patch introduction in [1]:
"
so that PG_WAIT_LWLOCK, PG_WAIT_LOCK, PG_WAIT_BUFFER_PIN and PG_WAIT_EXTENSION are not autogenerated.
This result to having the wait event part of the documentation "monitoring-stats" not ordered as compared to the "Wait Event Types" Table.
.
.
.
"
Thanks Michael for having provided this detailed explanation (my patch
introduction clearly was missing some context as Andres pointed out).
[1]: https://www.postgresql.org/message-id/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98%40gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-16 06:16 Michael Paquier <michael@paquier.xyz>
parent: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2023-05-16 06:16 UTC (permalink / raw)
To: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, May 15, 2023 at 10:07:04AM +0200, Drouvot, Bertrand wrote:
> This is preliminary work to autogenerate some wait events
> code and documentation done in [1].
>
> The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
> and WAIT_EVENT_BUFFER_PIN) and their associated functions
> (pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
>
> Please note that that would not break extensions outside contrib
> that make use of the existing PG_WAIT_EXTENSION.
I have looked at this one, and I think that's OK for what you are
aiming at here (in addition to my previous message that I hope
provides enough context about the whys and the hows).
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-17 06:29 Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 25+ messages in thread
From: Drouvot, Bertrand @ 2023-05-17 06:29 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 5/16/23 8:16 AM, Michael Paquier wrote:
> On Mon, May 15, 2023 at 10:07:04AM +0200, Drouvot, Bertrand wrote:
>> This is preliminary work to autogenerate some wait events
>> code and documentation done in [1].
>>
>> The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
>> and WAIT_EVENT_BUFFER_PIN) and their associated functions
>> (pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
>>
>> Please note that that would not break extensions outside contrib
>> that make use of the existing PG_WAIT_EXTENSION.
>
> I have looked at this one, and I think that's OK for what you are
> aiming at here (in addition to my previous message that I hope
> provides enough context about the whys and the hows).
Thanks!
Please find V2 attached, it adds WaitEventBufferPin and WaitEventExtension to
src/tools/pgindent/typedefs.list (that was done in [1]...).
[1]: https://www.postgresql.org/message-id/flat/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98%40gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Introducing-WAIT_EVENT_EXTENSION-and-WAIT_EVENT_B.patch (text/plain, 10.5K) download
From 4c9ca1389b0aa9f7eae34c3b72f15dbc7da39783 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Sat, 13 May 2023 07:59:08 +0000
Subject: [PATCH v2] Introducing WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
---
contrib/dblink/dblink.c | 4 +-
contrib/pg_prewarm/autoprewarm.c | 4 +-
contrib/postgres_fdw/connection.c | 6 +--
src/backend/storage/buffer/bufmgr.c | 2 +-
src/backend/storage/ipc/standby.c | 2 +-
src/backend/utils/activity/wait_event.c | 66 +++++++++++++++++++++---
src/include/utils/wait_event.h | 20 ++++++-
src/test/modules/test_shm_mq/setup.c | 2 +-
src/test/modules/test_shm_mq/test.c | 2 +-
src/test/modules/worker_spi/worker_spi.c | 2 +-
src/tools/pgindent/typedefs.list | 2 +
11 files changed, 93 insertions(+), 19 deletions(-)
8.8% contrib/dblink/
4.6% contrib/pg_prewarm/
9.3% contrib/postgres_fdw/
3.4% src/backend/storage/buffer/
3.2% src/backend/storage/ipc/
47.9% src/backend/utils/activity/
14.2% src/include/utils/
4.5% src/test/modules/test_shm_mq/
3.7% src/
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 55f75eff36..f167cb71d4 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -203,7 +203,7 @@ dblink_get_conn(char *conname_or_str,
dblink_connstr_check(connstr);
/* OK to make connection */
- conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
+ conn = libpqsrv_connect(connstr, WAIT_EVENT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
@@ -293,7 +293,7 @@ dblink_connect(PG_FUNCTION_ARGS)
dblink_connstr_check(connstr);
/* OK to make connection */
- conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
+ conn = libpqsrv_connect(connstr, WAIT_EVENT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 93835449c0..d0efc9e524 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -237,7 +237,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
}
else
{
@@ -264,7 +264,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
delay_in_ms,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
}
/* Reset the latch, loop. */
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index da32d503bc..25d0c43b64 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -530,7 +530,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
/* OK to make connection */
conn = libpqsrv_connect_params(keywords, values,
false, /* expand_dbname */
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
if (!conn || PQstatus(conn) != CONNECTION_OK)
ereport(ERROR,
@@ -863,7 +863,7 @@ pgfdw_get_result(PGconn *conn, const char *query)
WL_LATCH_SET | WL_SOCKET_READABLE |
WL_EXIT_ON_PM_DEATH,
PQsocket(conn),
- -1L, PG_WAIT_EXTENSION);
+ -1L, WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
@@ -1567,7 +1567,7 @@ pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime, PGresult **result,
WL_LATCH_SET | WL_SOCKET_READABLE |
WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
PQsocket(conn),
- cur_timeout, PG_WAIT_EXTENSION);
+ cur_timeout, WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 1fa689052e..c7c1782461 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4895,7 +4895,7 @@ LockBufferForCleanup(Buffer buffer)
SetStartupBufferPinWaitBufId(-1);
}
else
- ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
+ ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);
/*
* Remove flag marking us as waiter. Normally this will not be set
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index ffe5e1563f..33188f829e 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -840,7 +840,7 @@ ResolveRecoveryConflictWithBufferPin(void)
* SIGHUP signal handler, etc cannot do that because it uses the different
* latch from that ProcWaitForSignal() waits on.
*/
- ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
+ ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);
if (got_standby_delay_timeout)
SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index 7940d64639..8572cf169e 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -28,7 +28,9 @@
static const char *pgstat_get_wait_activity(WaitEventActivity w);
+static const char *pgstat_get_wait_bufferpin(WaitEventBufferPin w);
static const char *pgstat_get_wait_client(WaitEventClient w);
+static const char *pgstat_get_wait_extension(WaitEventExtension w);
static const char *pgstat_get_wait_ipc(WaitEventIPC w);
static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
static const char *pgstat_get_wait_io(WaitEventIO w);
@@ -90,7 +92,7 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
case PG_WAIT_LOCK:
event_type = "Lock";
break;
- case PG_WAIT_BUFFER_PIN:
+ case PG_WAIT_BUFFERPIN:
event_type = "BufferPin";
break;
case PG_WAIT_ACTIVITY:
@@ -147,9 +149,13 @@ pgstat_get_wait_event(uint32 wait_event_info)
case PG_WAIT_LOCK:
event_name = GetLockNameFromTagType(eventId);
break;
- case PG_WAIT_BUFFER_PIN:
- event_name = "BufferPin";
- break;
+ case PG_WAIT_BUFFERPIN:
+ {
+ WaitEventBufferPin w = (WaitEventBufferPin) wait_event_info;
+
+ event_name = pgstat_get_wait_bufferpin(w);
+ break;
+ }
case PG_WAIT_ACTIVITY:
{
WaitEventActivity w = (WaitEventActivity) wait_event_info;
@@ -165,8 +171,12 @@ pgstat_get_wait_event(uint32 wait_event_info)
break;
}
case PG_WAIT_EXTENSION:
- event_name = "Extension";
- break;
+ {
+ WaitEventExtension w = (WaitEventExtension) wait_event_info;
+
+ event_name = pgstat_get_wait_extension(w);
+ break;
+ }
case PG_WAIT_IPC:
{
WaitEventIPC w = (WaitEventIPC) wait_event_info;
@@ -254,6 +264,28 @@ pgstat_get_wait_activity(WaitEventActivity w)
return event_name;
}
+/* ----------
+ * pgstat_get_wait_bufferpin() -
+ *
+ * Convert WaitEventBufferPin to string.
+ * ----------
+ */
+static const char *
+pgstat_get_wait_bufferpin(WaitEventBufferPin w)
+{
+ const char *event_name = "unknown wait event";
+
+ switch (w)
+ {
+ case WAIT_EVENT_BUFFER_PIN:
+ event_name = "BufferPin";
+ break;
+ /* no default case, so that compiler will warn */
+ }
+
+ return event_name;
+}
+
/* ----------
* pgstat_get_wait_client() -
*
@@ -297,6 +329,28 @@ pgstat_get_wait_client(WaitEventClient w)
return event_name;
}
+/* ----------
+ * pgstat_get_wait_extension() -
+ *
+ * Convert WaitEventExtension to string.
+ * ----------
+ */
+static const char *
+pgstat_get_wait_extension(WaitEventExtension w)
+{
+ const char *event_name = "unknown wait event";
+
+ switch (w)
+ {
+ case WAIT_EVENT_EXTENSION:
+ event_name = "Extension";
+ break;
+ /* no default case, so that compiler will warn */
+ }
+
+ return event_name;
+}
+
/* ----------
* pgstat_get_wait_ipc() -
*
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 518d3b0a1f..dc01d4e84d 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -17,7 +17,7 @@
*/
#define PG_WAIT_LWLOCK 0x01000000U
#define PG_WAIT_LOCK 0x03000000U
-#define PG_WAIT_BUFFER_PIN 0x04000000U
+#define PG_WAIT_BUFFERPIN 0x04000000U
#define PG_WAIT_ACTIVITY 0x05000000U
#define PG_WAIT_CLIENT 0x06000000U
#define PG_WAIT_EXTENSION 0x07000000U
@@ -50,6 +50,15 @@ typedef enum
WAIT_EVENT_WAL_WRITER_MAIN
} WaitEventActivity;
+/* ----------
+ * Wait Events - BUFFERPIN
+ * ----------
+ */
+typedef enum
+{
+ WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFERPIN
+} WaitEventBufferPin;
+
/* ----------
* Wait Events - Client
*
@@ -70,6 +79,15 @@ typedef enum
WAIT_EVENT_WAL_SENDER_WRITE_DATA,
} WaitEventClient;
+/* ----------
+ * Wait Events - EXTENSION
+ * ----------
+ */
+typedef enum
+{
+ WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION
+} WaitEventExtension;
+
/* ----------
* Wait Events - IPC
*
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index bec5732e87..192e5cc2ab 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -280,7 +280,7 @@ wait_for_workers_to_become_ready(worker_state *wstate,
/* Wait to be signaled. */
(void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
/* Reset the latch so we don't spin. */
ResetLatch(MyLatch);
diff --git a/src/test/modules/test_shm_mq/test.c b/src/test/modules/test_shm_mq/test.c
index 906e943e2d..d9be703350 100644
--- a/src/test/modules/test_shm_mq/test.c
+++ b/src/test/modules/test_shm_mq/test.c
@@ -232,7 +232,7 @@ test_shm_mq_pipelined(PG_FUNCTION_ARGS)
* for us to do.
*/
(void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
}
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index ad491d7722..7227cfaa45 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -199,7 +199,7 @@ worker_spi_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
worker_spi_naptime * 1000L,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
ResetLatch(MyLatch);
CHECK_FOR_INTERRUPTS();
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 06da7cd428..dce1bcbfdf 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2972,7 +2972,9 @@ WSANETWORKEVENTS
WSAPROTOCOL_INFO
WaitEvent
WaitEventActivity
+WaitEventBufferPin
WaitEventClient
+WaitEventExtension
WaitEventIO
WaitEventIPC
WaitEventSet
--
2.34.1
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-17 13:22 Robert Haas <robertmhaas@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
2 siblings, 2 replies; 25+ messages in thread
From: Robert Haas @ 2023-05-17 13:22 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andres Freund <andres@anarazel.de>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, May 16, 2023 at 1:14 AM Michael Paquier <michael@paquier.xyz> wrote:
> These are the two things refactored in the patch, explaining the what.
> The reason behind the why is to make the script in charge of
> generating all these structures and functions consistent for all the
> wait event classes, simply. Treating all the wait event classes
> together eases greatly the generation of the documentation, so that it
> is possible to enforce an ordering of the tables of each class used to
> list each wait event type attached to them. Does this explanation
> make sense?
Not really. At least not to me. Changing the code in dblink to use
WAIT_EVENT_EXTENSION instead of PG_WAIT_EXTENSION doesn't help you
automatically generate documentation in any way.
It seems to me that your automatic generation code might need a
special case for wait event types that contain only a single wait
event. But that doesn't seem like a bad thing to have. Adding
pgstat_get_wait_extension adds runtime cost for no corresponding
benefit. Having a special case in the code to avoid that seems
worthwhile.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-17 22:48 Michael Paquier <michael@paquier.xyz>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2023-05-17 22:48 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 17, 2023 at 09:22:19AM -0400, Robert Haas wrote:
> It seems to me that your automatic generation code might need a
> special case for wait event types that contain only a single wait
> event. But that doesn't seem like a bad thing to have. Adding
> pgstat_get_wait_extension adds runtime cost for no corresponding
> benefit. Having a special case in the code to avoid that seems
> worthwhile.
Okay. We are going to need an approach similar to what's done for
src/backend/nodes where two things are generated in order to be able
to have some of the wait event classes be treated as exceptions in the
switch calling each function (pgstat_get_wait_event). I'd assume:
- Create the code calling the functions automatically, say in a
wait_event_type.switch.c or something like that. If a class has one
single element, generate the code from it.
- Create a second file with the functions and their internals, as the
patch does now (like wait_event_type.funcs.c?), discarding classes
with single elements.
- Skip the creation of the enum structures for single-element classes,
as well.
Still it looks like the renaming of BufferPin would need to remain
around to ease a bit the work of the script. Bertrand, what do you
think?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-17 23:28 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 25+ messages in thread
From: Michael Paquier @ 2023-05-17 23:28 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, May 18, 2023 at 07:48:26AM +0900, Michael Paquier wrote:
> Okay. We are going to need an approach similar to what's done for
> src/backend/nodes where two things are generated in order to be able
> to have some of the wait event classes be treated as exceptions in the
> switch calling each function (pgstat_get_wait_event). I'd assume:
> - Create the code calling the functions automatically, say in a
> wait_event_type.switch.c or something like that. If a class has one
> single element, generate the code from it.
> - Create a second file with the functions and their internals, as the
> patch does now (like wait_event_type.funcs.c?), discarding classes
> with single elements.
> - Skip the creation of the enum structures for single-element classes,
> as well.
On top of that, why don't we just apply some inlining to all the
pgstat_get_wait_*() functions? If we do that, even the existing
functions could see a benefit on top of the ones associated to classes
with single elements. Inlining may not be always applied depending on
what the compiler wants, of course..
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-17 23:38 Andres Freund <andres@anarazel.de>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Andres Freund @ 2023-05-17 23:38 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 2023-05-17 09:22:19 -0400, Robert Haas wrote:
> Adding pgstat_get_wait_extension adds runtime cost for no corresponding
> benefit. Having a special case in the code to avoid that seems worthwhile.
I don't think that should ever be used in a path where performance is
relevant?
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-18 16:28 Robert Haas <robertmhaas@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 25+ messages in thread
From: Robert Haas @ 2023-05-18 16:28 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Michael Paquier <michael@paquier.xyz>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, May 17, 2023 at 7:38 PM Andres Freund <andres@anarazel.de> wrote:
> On 2023-05-17 09:22:19 -0400, Robert Haas wrote:
> > Adding pgstat_get_wait_extension adds runtime cost for no corresponding
> > benefit. Having a special case in the code to avoid that seems worthwhile.
>
> I don't think that should ever be used in a path where performance is
> relevant?
I mean, I agree that it would probably be hard to measure any real
performance difference. But I'm not sure that's a good reason to add
cycles to a path where we don't really need to.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-18 22:36 Michael Paquier <michael@paquier.xyz>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2023-05-18 22:36 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, May 18, 2023 at 12:28:20PM -0400, Robert Haas wrote:
> I mean, I agree that it would probably be hard to measure any real
> performance difference. But I'm not sure that's a good reason to add
> cycles to a path where we don't really need to.
Honestly, I am not sure that it's worth worrying about performance
here, or perhaps you know of some external stuff that could set the
extension class type in a code path hot enough that it would matter.
Anyway, why couldn't we make all these functions static inline
instead, then?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-05-19 07:48 Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 25+ messages in thread
From: Drouvot, Bertrand @ 2023-05-19 07:48 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 5/19/23 12:36 AM, Michael Paquier wrote:
> On Thu, May 18, 2023 at 12:28:20PM -0400, Robert Haas wrote:
>> I mean, I agree that it would probably be hard to measure any real
>> performance difference. But I'm not sure that's a good reason to add
>> cycles to a path where we don't really need to.
>
> Honestly, I am not sure that it's worth worrying about performance
> here,
Same feeling here and as those new functions will be used "only" from
pg_stat_get_activity() / pg_stat_get_backend_wait_event().
> or perhaps you know of some external stuff that could set the
> extension class type in a code path hot enough that it would matter.
And that would matter, only when making use of pg_stat_get_activity()
/ pg_stat_get_backend_wait_event() at the time the "extension is waiting"
on this wait event.
While at it, I think that making use of an enum might also be an open door
(need to think more about it) to allow extensions to set their own wait event.
Something like RequestNamedLWLockTranche()/GetNamedLWLockTranche() are doing.
Currently we have "only" the "extension" wait event which is not that useful when
there is multiples extensions installed in a database.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-06-08 01:57 Masahiro Ikeda <ikedamsh@oss.nttdata.com>
parent: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Masahiro Ikeda @ 2023-06-08 01:57 UTC (permalink / raw)
To: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2023-05-19 16:48, Drouvot, Bertrand wrote:
> While at it, I think that making use of an enum might also be an open
> door
> (need to think more about it) to allow extensions to set their own wait
> event.
> Something like RequestNamedLWLockTranche()/GetNamedLWLockTranche() are
> doing.
>
> Currently we have "only" the "extension" wait event which is not that
> useful when
> there is multiples extensions installed in a database.
(Excuse me for cutting in, and this is not directly related to the
thread.)
+1. I'm interested in the feature.
Recently, I encountered a case where it would be nice if
different wait events were output for each extension.
I tested a combination of two extensions, postgres_fdw and neon[1],
and they output the "Extension" wait event, but it wasn't immediately
clear
which one was the bottleneck.
This is just a example and it probable be useful for other users. IMO,
at least,
it's better to improve the specification that "Extension" wait event
type has
only the "Extension" wait event.
[1] https://github.com/neondatabase/neon
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-06-08 23:15 Michael Paquier <michael@paquier.xyz>
parent: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
0 siblings, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2023-06-08 23:15 UTC (permalink / raw)
To: Masahiro Ikeda <ikedamsh@oss.nttdata.com>; +Cc: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
> (Excuse me for cutting in, and this is not directly related to the thread.)
> +1. I'm interested in the feature.
>
> This is just a example and it probable be useful for other users. IMO, at
> least, it's better to improve the specification that "Extension"
> wait event type has only the "Extension" wait event.
I hope that nobody would counter-argue you here. In my opinion, we
should just introduce an API that allows extensions to retrieve wait
event numbers that are allocated by the backend under
PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
Say something like:
int GetExtensionWaitEvent(const char *wait_event_name);
I don't quite see a design where extensions could rely on their own
numbers statically assigned by the extension maintainers, as this is
most likely going to cause conflicts. And I would guess that a lot of
external code would want to get more data pushed to pg_stat_activity,
meaning a lot of conflicts, potentially.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-06-09 04:26 Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 25+ messages in thread
From: Drouvot, Bertrand @ 2023-06-09 04:26 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Masahiro Ikeda <ikedamsh@oss.nttdata.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 6/9/23 1:15 AM, Michael Paquier wrote:
> On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
>> (Excuse me for cutting in, and this is not directly related to the thread.)
>> +1. I'm interested in the feature.
>>
>> This is just a example and it probable be useful for other users. IMO, at
>> least, it's better to improve the specification that "Extension"
>> wait event type has only the "Extension" wait event.
>
> I hope that nobody would counter-argue you here. In my opinion, we
> should just introduce an API that allows extensions to retrieve wait
> event numbers that are allocated by the backend under
> PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
> Say something like:
> int GetExtensionWaitEvent(const char *wait_event_name);
+1, that's something I've in mind to work on once/if this patch is/get
committed.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-06-09 05:58 Michael Paquier <michael@paquier.xyz>
parent: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
0 siblings, 2 replies; 25+ messages in thread
From: Michael Paquier @ 2023-06-09 05:58 UTC (permalink / raw)
To: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; +Cc: Masahiro Ikeda <ikedamsh@oss.nttdata.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Jun 09, 2023 at 06:26:07AM +0200, Drouvot, Bertrand wrote:
> +1, that's something I've in mind to work on once/if this patch is/get
> committed.
FWIW, I'm OK with the patch, without the need to worry about the
performance. Even if that's the case, we could just mark all these as
inline and move on..
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-06-09 09:20 Masahiro Ikeda <ikedamsh@oss.nttdata.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 25+ messages in thread
From: Masahiro Ikeda @ 2023-06-09 09:20 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 2023-06-09 13:26, Drouvot, Bertrand wrote:
> Hi,
>
> On 6/9/23 1:15 AM, Michael Paquier wrote:
>> On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
>>> (Excuse me for cutting in, and this is not directly related to the
>>> thread.)
>>> +1. I'm interested in the feature.
>>>
>>> This is just a example and it probable be useful for other users.
>>> IMO, at
>>> least, it's better to improve the specification that "Extension"
>>> wait event type has only the "Extension" wait event.
>>
>> I hope that nobody would counter-argue you here. In my opinion, we
>> should just introduce an API that allows extensions to retrieve wait
>> event numbers that are allocated by the backend under
>> PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
>> Say something like:
>> int GetExtensionWaitEvent(const char *wait_event_name);
>
> +1, that's something I've in mind to work on once/if this patch is/get
> committed.
Thanks for replying. If you are ok, I'll try to make a patch
to allow extensions to define custom wait events.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-06-09 10:11 Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
parent: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
0 siblings, 0 replies; 25+ messages in thread
From: Drouvot, Bertrand @ 2023-06-09 10:11 UTC (permalink / raw)
To: Masahiro Ikeda <ikedamsh@oss.nttdata.com>; Michael Paquier <michael@paquier.xyz>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 6/9/23 11:20 AM, Masahiro Ikeda wrote:
> Hi,
>
> On 2023-06-09 13:26, Drouvot, Bertrand wrote:
>> Hi,
>>
>> On 6/9/23 1:15 AM, Michael Paquier wrote:
>>> On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
>>>> (Excuse me for cutting in, and this is not directly related to the thread.)
>>>> +1. I'm interested in the feature.
>>>>
>>>> This is just a example and it probable be useful for other users. IMO, at
>>>> least, it's better to improve the specification that "Extension"
>>>> wait event type has only the "Extension" wait event.
>>>
>>> I hope that nobody would counter-argue you here. In my opinion, we
>>> should just introduce an API that allows extensions to retrieve wait
>>> event numbers that are allocated by the backend under
>>> PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
>>> Say something like:
>>> int GetExtensionWaitEvent(const char *wait_event_name);
>>
>> +1, that's something I've in mind to work on once/if this patch is/get
>> committed.
>
> Thanks for replying. If you are ok, I'll try to make a patch
> to allow extensions to define custom wait events.
Great, thank you!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN
@ 2023-07-03 04:18 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 25+ messages in thread
From: Michael Paquier @ 2023-07-03 04:18 UTC (permalink / raw)
To: Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>; +Cc: Masahiro Ikeda <ikedamsh@oss.nttdata.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Jun 09, 2023 at 02:58:42PM +0900, Michael Paquier wrote:
> FWIW, I'm OK with the patch, without the need to worry about the
> performance. Even if that's the case, we could just mark all these as
> inline and move on..
I am attempting to get all that done for the beginning of the
development cycle, so the first refactoring patch has been applied.
Now into the second, main one..
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2023-07-03 04:18 UTC | newest]
Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-05-15 08:07 Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
2023-05-15 18:29 ` Andres Freund <andres@anarazel.de>
2023-05-15 22:30 ` Michael Paquier <michael@paquier.xyz>
2023-05-16 00:17 ` Andres Freund <andres@anarazel.de>
2023-05-16 00:38 ` Michael Paquier <michael@paquier.xyz>
2023-05-16 01:01 ` Andres Freund <andres@anarazel.de>
2023-05-16 05:14 ` Michael Paquier <michael@paquier.xyz>
2023-05-16 05:38 ` Kirk Wolak <wolakk@gmail.com>
2023-05-16 06:10 ` Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
2023-05-17 13:22 ` Robert Haas <robertmhaas@gmail.com>
2023-05-17 22:48 ` Michael Paquier <michael@paquier.xyz>
2023-05-17 23:28 ` Michael Paquier <michael@paquier.xyz>
2023-05-17 23:38 ` Andres Freund <andres@anarazel.de>
2023-05-18 16:28 ` Robert Haas <robertmhaas@gmail.com>
2023-05-18 22:36 ` Michael Paquier <michael@paquier.xyz>
2023-05-19 07:48 ` Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
2023-06-08 01:57 ` Masahiro Ikeda <ikedamsh@oss.nttdata.com>
2023-06-08 23:15 ` Michael Paquier <michael@paquier.xyz>
2023-06-09 04:26 ` Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
2023-06-09 05:58 ` Michael Paquier <michael@paquier.xyz>
2023-06-09 09:20 ` Masahiro Ikeda <ikedamsh@oss.nttdata.com>
2023-06-09 10:11 ` Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
2023-07-03 04:18 ` Michael Paquier <michael@paquier.xyz>
2023-05-16 06:16 ` Michael Paquier <michael@paquier.xyz>
2023-05-17 06:29 ` Drouvot, Bertrand <bertranddrouvot.pg@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox