Tracking wait event for latches
Hi all,
As I mentioned $subject a couple of months back after looking at the
wait event facility here:
/messages/by-id/CAB7nPqTJpgAvOK4qSC96Fpm5W+aCtJ9D=3Vn9AfiEYsur=-juw@mail.gmail.com
I have actually taken some time to implement this idea.
The particular case that I had in mind was to be able to track in
pg_stat_activity processes that are waiting on a latch for synchronous
replication, and here is what this patch gives in this case:
=# alter system set synchronous_standby_names = 'foo';
ALTER SYSTEM
=# select pg_reload_conf();
pg_reload_conf
----------------
t
(1 row)
=# -- Do something
[...]
And from another session:
=# select wait_event_type, wait_event from pg_stat_activity where pid = 83316;
wait_event_type | wait_event
-----------------+------------
Latch | SyncRep
(1 row)
This is a boring patch, and it relies on the wait event facility that
has been added recently in 9.6. Note a couple of things though:
1) There is something like 30 code paths calling WaitLatch in the
backend code, all those events are classified and documented similarly
to LWLock events.
2) After discussing this stuff while at PGCon, it does not seem worth
to have any kind of APIs to be able to add in shared memory custom
latch names that extensions could load through _PG_init(). In
replacement to that, there is a latch type flag called "Extension"
that can be used for this purpose.
Comments are welcome, I am adding that in the 9.7 queue.
Regards,
--
Michael
Attachments:
wait-event-latch.patchinvalid/octet-stream; name=wait-event-latch.patchDownload+365-42
On Thu, May 19, 2016 at 4:14 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Comments are welcome, I am adding that in the 9.7 queue.
Take that as 10.0 as things are going.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, May 20, 2016 at 8:14 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Hi all,
As I mentioned $subject a couple of months back after looking at the
wait event facility here:
/messages/by-id/CAB7nPqTJpgAvOK4qSC96Fpm5W+aCtJ9D=3Vn9AfiEYsur=-juw@mail.gmail.com
I have actually taken some time to implement this idea.The particular case that I had in mind was to be able to track in
pg_stat_activity processes that are waiting on a latch for synchronous
replication, and here is what this patch gives in this case:
=# alter system set synchronous_standby_names = 'foo';
ALTER SYSTEM
=# select pg_reload_conf();
pg_reload_conf
----------------
t
(1 row)
=# -- Do something
[...]And from another session:
=# select wait_event_type, wait_event from pg_stat_activity where pid = 83316;
wait_event_type | wait_event
-----------------+------------
Latch | SyncRep
(1 row)This is a boring patch, and it relies on the wait event facility that
has been added recently in 9.6. Note a couple of things though:
1) There is something like 30 code paths calling WaitLatch in the
backend code, all those events are classified and documented similarly
to LWLock events.
2) After discussing this stuff while at PGCon, it does not seem worth
to have any kind of APIs to be able to add in shared memory custom
latch names that extensions could load through _PG_init(). In
replacement to that, there is a latch type flag called "Extension"
that can be used for this purpose.
Comments are welcome, I am adding that in the 9.7 queue.
This is very cool, and I can't wait to have this feature! It'll be
useful for all kinds of developers and users. I wanted this today to
help debug something I am working on, and remembered this patch. I
have signed up as a reviewer for the September commitfest. But here
is some initial feedback based on a quick glance at it:
This patch allows identifiers to be specified by the WaitLatch and
WaitLatchOrSocket calls, but not for WaitEventSetWait, which is the
more general waiting primitive. I think it should be done by
WaitEventSetWait, and merely passed down by those wrappers functions.
These are actually names for *wait points*, not names for latches.
Some of the language in this patch makes it sound like they are latch
names/latch identifiers, which is inaccurate (the latches themselves
wouldn't be very interesting eg MyLatch). In some cases the main
thing of interest is actually a socket or timer anyway, not a latch,
so maybe it should appear as wait_event_type = WaitEventSet?
Is there a reason you chose names like 'WALWriterMain'? That
particular wait point is in the function WalWriterMain (note different
case). It seems odd to use the containing function names to guide
naming, but not use them exactly. Since this namespace needs to be
able to name wait points anywhere in the postgres source tree (and
maybe outside it too, for extensions), maybe it should be made
hierarchical, like 'walwriter.WalWriterMain' (or some other
organisational scheme).
I personally think it would be very useful for extensions to be able
to name their wait points. For example, I'd rather see
'postgres_fdw.pgfdw_get_result' or similar than a vague 'Extension'
string which appears not only for all wait points in an extension but
also for all extensions. I hope we can figure out a good way to do
that. Clearly that would involve some shmem registry machinery to
make the names visible across backends (a similar problem exists with
lock tranche names).
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 2, 2016 at 12:25 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
This patch allows identifiers to be specified by the WaitLatch and
WaitLatchOrSocket calls, but not for WaitEventSetWait, which is the
more general waiting primitive. I think it should be done by
WaitEventSetWait, and merely passed down by those wrappers functions.
The advantage of having WaitEventSetWait track is that we could track
the events of secure_read and secure_write. One potential problem by
doing so is if those routines are called during authentication. I
don't recall that's the case, but this needs a double-check.
These are actually names for *wait points*, not names for latches.
OK.
Some of the language in this patch makes it sound like they are latch
names/latch identifiers, which is inaccurate (the latches themselves
wouldn't be very interesting eg MyLatch). In some cases the main
thing of interest is actually a socket or timer anyway, not a latch,
so maybe it should appear as wait_event_type = WaitEventSet?
Hm. A latch could wait for multiple types things it is waiting for, so
don't you think we'd need to add more details in what is reported to
pg_stat_activity? There are two fields now, and in the context of this
patch:
- wait_event_type, which I'd like to think is associated to a latch,
so I named it so.
- wait_event, which is the code path that we are waiting at, like
SyncRep, the WAL writer main routine, etc.
Now if you would like to get a list of all the things that are being
waited for, shouldn't we add a third column to the set that has text[]
as return type? Let's name it wait_event_details, and for a latch we
have the following:
- WL_LATCH_SET
- WL_POSTMASTER_DEATH
- WL_SOCKET_READABLE
- WL_SOCKET_WRITEABLE
Compared to all the other existing wait types, that's a bit new and
that's exclusive to latches because we need a higher level of details.
Don't you think so? But actually I don't think that's necessary to go
into this level of details. We already know what a latch is waiting
for by looking at the code...
Is there a reason you chose names like 'WALWriterMain'? That
particular wait point is in the function WalWriterMain (note different
case). It seems odd to use the containing function names to guide
naming, but not use them exactly. Since this namespace needs to be
able to name wait points anywhere in the postgres source tree (and
maybe outside it too, for extensions), maybe it should be made
hierarchical, like 'walwriter.WalWriterMain' (or some other
organisational scheme).
Yeah, possibly this could be improved. I have put some thoughts in
having clear names for each one of them, so I'd rather keep them
simple.
I personally think it would be very useful for extensions to be able
to name their wait points. For example, I'd rather see
'postgres_fdw.pgfdw_get_result' or similar than a vague 'Extension'
string which appears not only for all wait points in an extension but
also for all extensions. I hope we can figure out a good way to do
that. Clearly that would involve some shmem registry machinery to
make the names visible across backends (a similar problem exists with
lock tranche names).
This patch is shaped this way intentionally based on the feedback I
received at PGCon (Robert and others). We could provide a routine that
extensions call in _PG_init to register a new latch event name in
shared memory, but I didn't see much use in doing so, take for example
the case of background worker, it is possible to register a custom
string for pg_stat_activity via pgstat_report_activity. One could take
advantage of having custom latch wait names in shared memory if an
extension has wait points with latches though... But I am still not
sure if that's worth the complexity.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 2, 2016 at 1:34 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
This patch is shaped this way intentionally based on the feedback I
received at PGCon (Robert and others). We could provide a routine that
extensions call in _PG_init to register a new latch event name in
shared memory, but I didn't see much use in doing so, take for example
the case of background worker, it is possible to register a custom
string for pg_stat_activity via pgstat_report_activity. One could take
advantage of having custom latch wait names in shared memory if an
extension has wait points with latches though... But I am still not
sure if that's worth the complexity.
I can't see how you could ever guarantee that it wouldn't just fail.
We allocate a certain amount of "slop" in the main shared memory
segment, but it's not infinite and can certainly be exhausted. It
seems like it would suck if you tried to load your extension and it
failed because there was no room left for more wait-point names.
Maybe it would suck less than not having wait-point names, but I'm not
really sure. I think we'd do better to get something that handles the
core stuff well and then consider extensions later or not at all. It
only matters if you're running multiple extensions that all use LWLock
tranches and you need to distinguish between waits on their various
LWLocks. But since LWLock contention is something we hope will be
infrequent I'm just not sure that case is common enough to justify
building a lot of mechanism.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 4, 2016 at 2:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 2, 2016 at 1:34 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:This patch is shaped this way intentionally based on the feedback I
received at PGCon (Robert and others). We could provide a routine that
extensions call in _PG_init to register a new latch event name in
shared memory, but I didn't see much use in doing so, take for example
the case of background worker, it is possible to register a custom
string for pg_stat_activity via pgstat_report_activity. One could take
advantage of having custom latch wait names in shared memory if an
extension has wait points with latches though... But I am still not
sure if that's worth the complexity.I can't see how you could ever guarantee that it wouldn't just fail.
We allocate a certain amount of "slop" in the main shared memory
segment, but it's not infinite and can certainly be exhausted. It
seems like it would suck if you tried to load your extension and it
failed because there was no room left for more wait-point names.
Maybe it would suck less than not having wait-point names, but I'm not
really sure. I think we'd do better to get something that handles the
core stuff well and then consider extensions later or not at all.
Yeah, that's as well my line of thoughts on the matter since the
beginning: keep it simple and done. What is written just after those
words is purely hand-waving and I have no way to prove it, but my
instinctive guess is that more than 90% of the real use cases where we
need to track the latch waits in pgstat would be covered without the
need of this extra shared memory infrastructure for extensions.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 8, 2016 at 10:11 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Yeah, that's as well my line of thoughts on the matter since the
beginning: keep it simple and done. What is written just after those
words is purely hand-waving and I have no way to prove it, but my
instinctive guess is that more than 90% of the real use cases where we
need to track the latch waits in pgstat would be covered without the
need of this extra shared memory infrastructure for extensions.
So, I have done some extra tests with my patch to see if I move the
event reporting down to WaitEventSetWait and see what is the effect on
secure_read and secure_write. And the conclusion is that I am seeing
no difference, so I changed the patch to the way suggested by Thomas.
In this test, what I have done was using the following pgbench script
with -C via an SSL connection:
\set id random(1,1000)
As the script file is not empty, a connection to the server is opened,
so this goes though secure_read at minimal cost on the backend.
Also, I have change the event ID notation as follows to be consistent
with the routine names:
WAL -> Wal
PG -> Pg
I also found that LATCH_RECOVERY_WAL_ALL was reporting
"XLogAfterAvailable" as name, which was incorrect.
Finally, I have changed the patch so as it does not track "Latch"
events, but "EventSet" events instead, per the suggestion of Thomas.
"WaitEventSet" is too close to wait_event in my taste so I shortened
the suggeston. There were also some conflicts caused by the recent
commit 887feefe, which are fixed.
Attached is an updated patch.
--
Michael
Attachments:
wait-event-set-v2.patchapplication/x-download; name=wait-event-set-v2.patchDownload+392-47
On Tue, Aug 2, 2016 at 10:31 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Attached is an updated patch.
Updated version for 2 minor issues:
1) s/stram/stream/
2) Docs used incorrect number
--
Michael
Attachments:
wait-event-set-v3.patchtext/x-diff; charset=US-ASCII; name=wait-event-set-v3.patchDownload+392-47
Hi, Michael!
On Thu, Aug 4, 2016 at 8:26 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Tue, Aug 2, 2016 at 10:31 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:Attached is an updated patch.
Updated version for 2 minor issues:
1) s/stram/stream/
2) Docs used incorrect number
I took a look at your patch. Couple of notes from me.
const char *
GetEventIdentifier(uint16 eventId)
{
const char *res;
switch (eventId)
{
case EVENT_ARCHIVER_MAIN:
res = "ArchiverMain";
break;
... long long list of events ...
case EVENT_WAL_SENDER_WRITE_DATA:
res = "WalSenderWriteData";
break;
default:
res = "???";
}
return res;
}
Would it be better to use an array here?
typedef enum EventIdentifier
{
EventIdentifier seems too general name for me, isn't it? Could we name it
WaitEventIdentifier? Or WaitEventId for shortcut?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Mon, Aug 22, 2016 at 12:09 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
I took a look at your patch. Couple of notes from me.
const char *
GetEventIdentifier(uint16 eventId)
{
const char *res;
switch (eventId)
{
case EVENT_ARCHIVER_MAIN:
res = "ArchiverMain";
break;
... long long list of events ...
case EVENT_WAL_SENDER_WRITE_DATA:
res = "WalSenderWriteData";
break;
default:
res = "???";
}
return res;
}Would it be better to use an array here?
typedef enum EventIdentifier
{
EventIdentifier seems too general name for me, isn't it? Could we name it
WaitEventIdentifier? Or WaitEventId for shortcut?
I'm also not sure about handling of secure_read() and secure_write()
functions.
In the current patch we're tracking latch event wait inside them. But we
setup latch only for blocking sockets and can do it multiple times in loop.
Would it be better to make separate wait events NETWORK_READ and
NETWORK_WRITE and setup them for the whole time spent in secure_read()
and secure_write()?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Mon, Aug 22, 2016 at 6:09 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
Hi, Michael!
On Thu, Aug 4, 2016 at 8:26 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
I took a look at your patch. Couple of notes from me.
Thanks!
const char *
GetEventIdentifier(uint16 eventId)
{
const char *res;
switch (eventId)
{
case EVENT_ARCHIVER_MAIN:
res = "ArchiverMain";
break;
... long long list of events ...
case EVENT_WAL_SENDER_WRITE_DATA:
res = "WalSenderWriteData";
break;
default:
res = "???";
}
return res;
}Would it be better to use an array here?
The reason why I chose this way is that there are a lot of them. It is
painful to maintain the order of the array elements in perfect mapping
with the list of IDs...
typedef enum EventIdentifier
{EventIdentifier seems too general name for me, isn't it? Could we name it
WaitEventIdentifier? Or WaitEventId for shortcut?
OK. So WaitEventIdentifier? The reason to include Identifier is for
consistency with lwlock structure notation.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 22, 2016 at 9:49 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
The reason why I chose this way is that there are a lot of them. It is
painful to maintain the order of the array elements in perfect mapping
with the list of IDs...
You can use stupid macro tricks to help with that problem...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 23, 2016 at 12:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 22, 2016 at 9:49 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:The reason why I chose this way is that there are a lot of them. It is
painful to maintain the order of the array elements in perfect mapping
with the list of IDs...You can use stupid macro tricks to help with that problem...
Yeah, still after thinking about it I think I would just go with an
array like lock types and be done with it. With a comment to mention
that the order should be respected things would be enough...
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 22, 2016 at 6:46 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
On Mon, Aug 22, 2016 at 12:09 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:I took a look at your patch. Couple of notes from me.
const char *
GetEventIdentifier(uint16 eventId)
{
const char *res;
switch (eventId)
{
case EVENT_ARCHIVER_MAIN:
res = "ArchiverMain";
break;
... long long list of events ...
case EVENT_WAL_SENDER_WRITE_DATA:
res = "WalSenderWriteData";
break;
default:
res = "???";
}
return res;
}Would it be better to use an array here?
Okay, I have switched to an array....
typedef enum EventIdentifier
{EventIdentifier seems too general name for me, isn't it? Could we name it
WaitEventIdentifier? Or WaitEventId for shortcut?
... And WaitEventIdentifier.
I'm also not sure about handling of secure_read() and secure_write()
functions.
In the current patch we're tracking latch event wait inside them. But we
setup latch only for blocking sockets and can do it multiple times in loop.
Would it be better to make separate wait events NETWORK_READ and
NETWORK_WRITE and setup them for the whole time spent in secure_read() and
secure_write()?
The whole point is to track a waiting event when we are sure that it
is going to wait, which is why the patch depends on WaitEventSetWait.
If we would set up those flags at the beginning and reset them of
secure_read and secure_write, we may actually track an event that is
not blocking.
--
Michael
Attachments:
wait-event-set-v4.patchtext/plain; charset=US-ASCII; name=wait-event-set-v4.patchDownload+328-47
+ <para>
+ <literal>EventSet</>: The server process is waiting on a socket
+ or a timer. This wait happens in a latch, an inter-process facility
+ using boolean variables letting a process sleep until it is set.
+ Latches support several type of operations, like postmaster death
+ handling, timeout and socket activity lookup, and are a useful
+ replacement for <function>sleep</> where signal handling matters.
+ </para>This paragraph seems a bit confused. I suggest something more like
this: "The server process is waiting for one or more sockets, a timer
or an interprocess latch. The wait happens in a WaitEventSet,
<productname>PostgreSQL</>'s portable IO multiplexing abstraction."
On Tue, Aug 23, 2016 at 7:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Mon, Aug 22, 2016 at 12:09 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:Would it be better to use an array here?
Okay, I have switched to an array....
I looked at various macro tricks[1]http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c but they're all pretty unpleasant!
+1 for the simple array with carefully managed order. About that
order...
+const char *const WaitEventNames[] = {
+ "ArchiverMain",
+ "AutoVacuumMain",
+ "BaseBackupThrottle",
+ "BgWorkerStartup",
+ "BgWorkerShutdown",
+ "BgWriterMain",
+ "BgWriterHibernate",
+ "CheckpointerMain",
+ "ExecuteGather",
+ "Extension",
+ "MessageQueuePutMessage",
+ "MessageQueueSend",
+ "MessageQueueReceive",
+ "MessageQueueInternal",
+ "ParallelFinish",
+ "PgStatMain",
+ "ProcSleep",
+ "ProcSignal",
+ "PgSleep",
+ "SecureRead",
+ "SecureWrite",
+ "SSLOpenServer",
+ "SyncRep",
+ "SysLoggerMain",
+ "RecoveryApplyDelay",
+ "RecoveryWalAll",
+ "RecoveryWalStream",
+ "WalReceiverWaitStart",
+ "WalReceiverMain",
+ "WalSenderMain",
+ "WalSenderWaitForWAL",
+ "WalSenderWriteData"
+ "WalWriterMain",
+};It looks like this array wants to be in alphabetical order, but it
isn't quite. Also, perhaps a compile time assertion about the size of
the array matching EVENT_LAST_TYPE could be useful?
+typedef enum WaitEventIdentifier
+{
+ EVENT_ARCHIVER_MAIN,
+ EVENT_AUTOVACUUM_MAIN,
+ EVENT_BASEBACKUP_THROTTLE,
+ EVENT_BGWORKER_STARTUP,
+ EVENT_BGWORKER_SHUTDOWN,
+ EVENT_BGWRITER_MAIN,
+ EVENT_BGWRITER_HIBERNATE,
+ EVENT_CHECKPOINTER_MAIN,
+ EVENT_EXECUTE_GATHER,
+ EVENT_EXTENSION,
+ EVENT_MQ_PUT_MESSAGE,
+ EVENT_MQ_SEND_BYTES,
+ EVENT_MQ_RECEIVE_BYTES,
+ EVENT_MQ_WAIT_INTERNAL,
+ EVENT_PARALLEL_WAIT_FINISH,
+ EVENT_PGSTAT_MAIN,
+ EVENT_PROC_SLEEP,
+ EVENT_PROC_SIGNAL,
+ EVENT_PG_SLEEP,
+ EVENT_SECURE_READ,
+ EVENT_SECURE_WRITE,
+ EVENT_SSL_OPEN_SERVER,
+ EVENT_SYNC_REP,
+ EVENT_SYSLOGGER_MAIN,
+ EVENT_RECOVERY_APPLY_DELAY,
+ EVENT_RECOVERY_WAL_ALL,
+ EVENT_RECOVERY_WAL_STREAM,
+ EVENT_WAL_RECEIVER_WAIT_START,
+ EVENT_WAL_RECEIVER_MAIN,
+ EVENT_WAL_SENDER_WRITE_DATA,
+ EVENT_WAL_SENDER_MAIN,
+ EVENT_WAL_SENDER_WAIT_WAL,
+ EVENT_WALWRITER_MAIN
+} WaitEventIdentifier;This is also nearly but not exactly in alphabetical order
(EVENT_PROC_SIGNAL comes after EVENT_PROC_SLEEP for example). But
it's not currently possible to have the strings and the enumerators
both in alphabetical order because they're not the same, even
accounting for camel-case to upper-case transliteration. I think at
least one of them should be sorted. Shouldn't they match fully and
then *both* be sorted alphabetically? For example
"MessageQueueInternal" doesn't match EVENT_MQ_WAIT_INTERNAL. Then
there are some cases where I'd expect underscores for consistency with
other enumerators and with the corresponding camel-case strings: you
have EVENT_WAL_SENDER_MAIN, but EVENT_WALWRITER_MAIN.
The documentation is in a slightly different order again but also not
exactly alphabetical: for example ProcSleep is listed before
ProcSignal.
Sorry if this all sounds terribly picky but I think we should try to
be strictly systematic here.
EventIdentifier seems too general name for me, isn't it? Could we name it
WaitEventIdentifier? Or WaitEventId for shortcut?... And WaitEventIdentifier.
+1 from me too for avoiding the overly general term 'event'. It does
seem a little odd to leave the enumerators names as EVENT_... though;
shouldn't these be WAIT_EVENT_... or WE_...? Or perhaps you could
consider WaitPointIdentifier and WP_SECURE_READ or
WaitEventPointIdentifier and WEP_SECURE_READ, if you buy my earlier
argument that what we are really naming here is point in the code
where we wait, not the events we're waiting for. Contrast with
LWLocks where we report the lock that you're waiting for, not the
place in the code where you're waiting for that lock.
On Wed, Aug 3, 2016 at 1:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Finally, I have changed the patch so as it does not track "Latch"
events, but "EventSet" events instead, per the suggestion of Thomas.
"WaitEventSet" is too close to wait_event in my taste so I shortened
the suggeston.
This is good, because showing "Latch" when we were really waiting for
a socket was misleading.
On the other hand, if we could *accurately* report whether it's a
"Latch", "Socket" or "Latch|Socket" that we're waiting for, it might
be cool to do that instead. One way to do that would be to use up
several class IDs: WAIT_EVENT_LATCH, WAIT_EVENT_LATCH_OR_SOCKET,
WAIT_EVENT_SOCKET (or perhaps WAIT_EVENT_LATCH | WAIT_EVENT_SOCKET,
reserving 2 or 3 upper bits from the 8 bit class ID for this). Then
we could figure out the right class ID by looking at set->latch and
set->nevents (perhaps an extra boolean would be needed to record
whether postmaster death is in there so we could deduce whether there
are any sockets). It would be interesting specifically for the case
of FDWs where it would be nice to be able to see clearly that it's
waiting for a remote server ("Socket"). It may also be interesting to
know if there is a timeout. Postmaster death doesn't seem newsworthy,
we're nearly always also waiting for that exceptional event so it'd
just be clutter to report it.
[1]: http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 21, 2016 at 8:13 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
+ <para> + <literal>EventSet</>: The server process is waiting on a socket + or a timer. This wait happens in a latch, an inter-process facility + using boolean variables letting a process sleep until it is set. + Latches support several type of operations, like postmaster death + handling, timeout and socket activity lookup, and are a useful + replacement for <function>sleep</> where signal handling matters. + </para>This paragraph seems a bit confused. I suggest something more like
this: "The server process is waiting for one or more sockets, a timer
or an interprocess latch. The wait happens in a WaitEventSet,
<productname>PostgreSQL</>'s portable IO multiplexing abstraction."
OK, I have tweaked the paragraph as follows using your suggestion:
+ <listitem>
+ <para>
+ <literal>EventSet</>: The server process is waiting on one or more
+ sockets, a time or an inter-process latch. The wait happens in a
+ <function>WaitEventSet</>, <productname>PostgreSQL</>'s portable
+ I/O multiplexing abstraction using boolean variables letting a
+ process sleep until it is set. It supports several type of
+ operations, like postmaster death handling, timeout and socket
+ activity lookup, and are a useful replacement for <function>sleep</>
+ where signal handling matters.
+ </para>
+ </listitem>On Tue, Aug 23, 2016 at 7:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Mon, Aug 22, 2016 at 12:09 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:Would it be better to use an array here?
Okay, I have switched to an array....
I looked at various macro tricks[1] but they're all pretty unpleasant!
+1 for the simple array with carefully managed order. About that
order...
[1] http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c
Yes, I recall bumping on this one, or something really similar to that...
+const char *const WaitEventNames[] = { [...] +};It looks like this array wants to be in alphabetical order, but it
isn't quite. Also, perhaps a compile time assertion about the size of
the array matching EVENT_LAST_TYPE could be useful?
In GetWaitEventIdentifier()? I'd think that just returning ??? would
have been fine if there is a non-matching call.
+typedef enum WaitEventIdentifier +{ [...] +} WaitEventIdentifier;This is also nearly but not exactly in alphabetical order
(EVENT_PROC_SIGNAL comes after EVENT_PROC_SLEEP for example). But
it's not currently possible to have the strings and the enumerators
both in alphabetical order because they're not the same, even
accounting for camel-case to upper-case transliteration. I think at
least one of them should be sorted. Shouldn't they match fully and
then *both* be sorted alphabetically? For example
"MessageQueueInternal" doesn't match EVENT_MQ_WAIT_INTERNAL. Then
there are some cases where I'd expect underscores for consistency with
other enumerators and with the corresponding camel-case strings: you
have EVENT_WAL_SENDER_MAIN, but EVENT_WALWRITER_MAIN.
Not wrong..
The documentation is in a slightly different order again but also not
exactly alphabetical: for example ProcSleep is listed before
ProcSignal.
Right.
Sorry if this all sounds terribly picky but I think we should try to
be strictly systematic here.
No worries about that, it matters a lot for this patch. The user-faced
documentation is what should do the decision-making I think. So let's
order the names, and adapt the enum depending on that. I have done so
after double-checking both lists, and added a comment for anybody
updating that in the fiture.
EventIdentifier seems too general name for me, isn't it? Could we name it
WaitEventIdentifier? Or WaitEventId for shortcut?... And WaitEventIdentifier.
+1 from me too for avoiding the overly general term 'event'. It does
seem a little odd to leave the enumerators names as EVENT_... though;
shouldn't these be WAIT_EVENT_... or WE_...? Or perhaps you could
consider WaitPointIdentifier and WP_SECURE_READ or
WaitEventPointIdentifier and WEP_SECURE_READ, if you buy my earlier
argument that what we are really naming here is point in the code
where we wait, not the events we're waiting for. Contrast with
LWLocks where we report the lock that you're waiting for, not the
place in the code where you're waiting for that lock.
Well, WE_ if I need make a choice for something else than EVENT_.
On the other hand, if we could *accurately* report whether it's a
"Latch", "Socket" or "Latch|Socket" that we're waiting for, it might
be cool to do that instead. One way to do that would be to use up
several class IDs: WAIT_EVENT_LATCH, WAIT_EVENT_LATCH_OR_SOCKET,
WAIT_EVENT_SOCKET (or perhaps WAIT_EVENT_LATCH | WAIT_EVENT_SOCKET,
reserving 2 or 3 upper bits from the 8 bit class ID for this). Then
we could figure out the right class ID by looking at set->latch and
set->nevents (perhaps an extra boolean would be needed to record
whether postmaster death is in there so we could deduce whether there
are any sockets). It would be interesting specifically for the case
of FDWs where it would be nice to be able to see clearly that it's
waiting for a remote server ("Socket"). It may also be interesting to
know if there is a timeout. Postmaster death doesn't seem newsworthy,
we're nearly always also waiting for that exceptional event so it'd
just be clutter to report it.
That's actually pretty close to what I mentioned upthread here:
/messages/by-id/CAB7nPqQx4OEym9cf22CY=5eWqqiAMjij6EBCoNReezt9-NvGkw@mail.gmail.com
In order to support that adding a column wait_event_details with
text[] makes the most sense I guess. Still I think that's another
discussion, this patch does already a lot.
So I have adjusted the patch in many ways, tweaked the order of the
items, and adjusted some of their names as suggested by Thomas.
Updated version attached.
--
Michael
Attachments:
wait-event-set-v5.patchtext/plain; charset=US-ASCII; name=wait-event-set-v5.patchDownload+333-47
On Wed, Sep 21, 2016 at 3:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Sep 21, 2016 at 8:13 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:It looks like this array wants to be in alphabetical order, but it
isn't quite. Also, perhaps a compile time assertion about the size of
the array matching EVENT_LAST_TYPE could be useful?In GetWaitEventIdentifier()? I'd think that just returning ??? would
have been fine if there is a non-matching call.
Yeah but that's at run time. I meant you could help developers
discover ASAP if they add a new item to one place but not the other
with a compile time assertion:
const char *
GetWaitEventIdentifier(uint16 eventId)
{
StaticAssertStmt(lengthof(WaitEventNames) == WE_LAST_TYPE + 1,
"WaitEventNames must match WaitEventIdentifiers");
if (eventId > WE_LAST_TYPE)
return "???";
return WaitEventNames[eventId];
}
+1 from me too for avoiding the overly general term 'event'. It does
seem a little odd to leave the enumerators names as EVENT_... though;
shouldn't these be WAIT_EVENT_... or WE_...? Or perhaps you could
consider WaitPointIdentifier and WP_SECURE_READ or
WaitEventPointIdentifier and WEP_SECURE_READ, if you buy my earlier
argument that what we are really naming here is point in the code
where we wait, not the events we're waiting for. Contrast with
LWLocks where we report the lock that you're waiting for, not the
place in the code where you're waiting for that lock.Well, WE_ if I need make a choice for something else than EVENT_.
You missed a couple that are hiding inside #ifdef WIN32:
From pgstat.c:
+ EVENT_PGSTAT_MAIN);
From syslogger.c:
+ EVENT_SYSLOGGER_MAIN);
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 21, 2016 at 1:03 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
Yeah but that's at run time. I meant you could help developers
discover ASAP if they add a new item to one place but not the other
with a compile time assertion:
const char *
GetWaitEventIdentifier(uint16 eventId)
{
StaticAssertStmt(lengthof(WaitEventNames) == WE_LAST_TYPE + 1,
"WaitEventNames must match WaitEventIdentifiers");
if (eventId > WE_LAST_TYPE)
return "???";
return WaitEventNames[eventId];
}
Ah, OK, good idea. I had AssertStmt in mind, not StaticAssertStmt.
You missed a couple that are hiding inside #ifdef WIN32:
From pgstat.c:
+ EVENT_PGSTAT_MAIN);From syslogger.c:
+ EVENT_SYSLOGGER_MAIN);
Oops. Fixed those ones and checked the builds on WIN32.
--
Michael
Attachments:
wait-event-set-v6.patchtext/plain; charset=US-ASCII; name=wait-event-set-v6.patchDownload+335-47
On Tue, Sep 20, 2016 at 7:13 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
This paragraph seems a bit confused. I suggest something more like
this: "The server process is waiting for one or more sockets, a timer
or an interprocess latch. The wait happens in a WaitEventSet,
<productname>PostgreSQL</>'s portable IO multiplexing abstraction."
I'm worried we're exposing an awful lot of internal detail here.
Moreover, it's pretty confusing that we have this general concept of
wait events in pg_stat_activity, and then here the specific type of
wait event we're waiting for is the ... wait event kind. Uh, what?
I have to admit that I like the individual event names quite a bit,
and I think the detail will be useful to users. But I wonder if
there's a better way to describe the class of events that we're
talking about that's not so dependent on internal data structures.
Maybe we could divide these waits into a couple of categories - e.g.
"Socket", "Timeout", "Process" - and then divide these detailed wait
events among those classes.
The "SecureRead" and "SecureWrite" wait events are going to be
confusing, because the connection isn't necessarily secure. I think
those should be called "ClientRead" and "ClientWrite".
Comprehensibility is more important than absolute consistency with the
C function names.
Another thing to think about is that there's no way to actually see
wait event information for a number of the processes which this patch
instruments, because they don't appear in pg_stat_activity.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 21, 2016 at 10:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I have to admit that I like the individual event names quite a bit,
and I think the detail will be useful to users. But I wonder if
there's a better way to describe the class of events that we're
talking about that's not so dependent on internal data structures.
Maybe we could divide these waits into a couple of categories - e.g.
"Socket", "Timeout", "Process" - and then divide these detailed wait
events among those classes.
pgstat.h is mentioning that there is 1 byte still free. I did not
notice that until a couple of minutes ago. There are 2 bytes used for
the event ID, and 1 byte for the class ID, but there are 4 bytes
available. Perhaps we could use this extra byte to store this extra
status information, then use it for WaitEventSet to build up a string
that will be stored in classId field? For example if a process is
waiting on a socket and a timeout, we'd write "Socket,Timeout" as a
text field.
The "SecureRead" and "SecureWrite" wait events are going to be
confusing, because the connection isn't necessarily secure. I think
those should be called "ClientRead" and "ClientWrite".
Comprehensibility is more important than absolute consistency with the
C function names.
Noted.
Another thing to think about is that there's no way to actually see
wait event information for a number of the processes which this patch
instruments, because they don't appear in pg_stat_activity.
We could create a new system to track the activity of system-related
processes, for example pg_stat_system_activity, or pg_system_activity,
and list all the processes that are not counted in max_connections...
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
