Re: WaitLatchOrSocket seems to not count to 4 right...
This entry has been waiting on author input for a while (our current threshold is roughly two weeks), so I've marked it Returned with Feedback. Once you think the patchset is ready for review again, you (or any interested party) can resurrect the patch entry by visiting https://commitfest.postgresql.org/38/3533/ and changing the status to "Needs Review", and then changing the status again to "Move to next CF". (Don't forget the second step; hopefully we will have streamlined this in the near future!) Thanks, --Jacob
Re: WaitLatchOrSocket seems to not count to 4 right...
Hi, On 2022-02-11 10:47:45 +1300, Thomas Munro wrote: > I'd originally sketched this out for another project, but I don't > think I need it for that anymore. After this exchange I couldn't > resist fleshing it out for a commitfest, just on useability grounds. > Thoughts? This currently doesn't apply: http://cfbot.cputube.org/patch_37_3533.log Marked as waiting-on-author for now. Are you aiming this for 15? Greetings, Andres Freund
Re: WaitLatchOrSocket seems to not count to 4 right...
On Tue, Feb 8, 2022 at 1:51 PM Thomas Munro wrote:
> (and one day we should make it dynamic and change udata to hold
> an index instead of a pointer...)
Here's a patch like that.
I'd originally sketched this out for another project, but I don't
think I need it for that anymore. After this exchange I couldn't
resist fleshing it out for a commitfest, just on useability grounds.
Thoughts?
From 7d011ed1de06ee6aad850f72399f0d3c9217458f Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 11 Feb 2022 10:39:46 +1300
Subject: [PATCH] Expand WaitEventSet dynamically.
Previously, CreateWaitEventSet() took an argument to say how many events
could be added. Teach it to grow automatically instead. Also expose a
"reserve" function in case a caller knows how many it needs and would
like to avoid reallocation.
Discussion: https://postgr.es/m/CA%2BhUKG%2BiaLp6ETo%2Bu0632yVUGO6eSMx4hKBbcc7zy88thBQy%3Dg%40mail.gmail.com
---
src/backend/executor/nodeAppend.c | 2 +-
src/backend/libpq/pqcomm.c | 2 +-
src/backend/postmaster/pgstat.c| 2 +-
src/backend/postmaster/syslogger.c | 2 +-
src/backend/storage/ipc/latch.c| 201 +++--
src/include/storage/latch.h| 3 +-
6 files changed, 110 insertions(+), 102 deletions(-)
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 7937f1c88f..3ad0441756 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -1029,7 +1029,7 @@ ExecAppendAsyncEventWait(AppendState *node)
/* We should never be called when there are no valid async subplans. */
Assert(node->as_nasyncremain > 0);
- node->as_eventset = CreateWaitEventSet(CurrentMemoryContext, nevents);
+ node->as_eventset = CreateWaitEventSet(CurrentMemoryContext);
AddWaitEventToSet(node->as_eventset, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
NULL, NULL);
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 22eb04948e..99fcc447b0 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -204,7 +204,7 @@ pq_init(void)
(errmsg("could not set socket to nonblocking mode: %m")));
#endif
- FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, 3);
+ FeBeWaitSet = CreateWaitEventSet(TopMemoryContext);
socket_pos = AddWaitEventToSet(FeBeWaitSet, WL_SOCKET_WRITEABLE,
MyProcPort->sock, NULL, NULL);
latch_pos = AddWaitEventToSet(FeBeWaitSet, WL_LATCH_SET, PGINVALID_SOCKET,
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 0646f53098..b4e791146c 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3528,7 +3528,7 @@ PgstatCollectorMain(int argc, char *argv[])
pgStatDBHash = pgstat_read_statsfiles(InvalidOid, true, true);
/* Prepare to wait for our latch or data in our socket. */
- wes = CreateWaitEventSet(CurrentMemoryContext, 3);
+ wes = CreateWaitEventSet(CurrentMemoryContext);
AddWaitEventToSet(wes, WL_LATCH_SET, PGINVALID_SOCKET, MyLatch, NULL);
AddWaitEventToSet(wes, WL_POSTMASTER_DEATH, PGINVALID_SOCKET, NULL, NULL);
AddWaitEventToSet(wes, WL_SOCKET_READABLE, pgStatSock, NULL, NULL);
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 25e2131e31..d4a9a8af34 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -311,7 +311,7 @@ SysLoggerMain(int argc, char *argv[])
* syslog pipe, which implies that all other backends have exited
* (including the postmaster).
*/
- wes = CreateWaitEventSet(CurrentMemoryContext, 2);
+ wes = CreateWaitEventSet(CurrentMemoryContext);
AddWaitEventToSet(wes, WL_LATCH_SET, PGINVALID_SOCKET, MyLatch, NULL);
#ifndef WIN32
AddWaitEventToSet(wes, WL_SOCKET_READABLE, syslogPipe[0], NULL, NULL);
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 5bb609b368..b5974d0f06 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -84,9 +84,32 @@
#error "no wait set implementation available"
#endif
+/* Each implementation needs an array of a certain object type. */
+#if defined(WAIT_USE_EPOLL)
+typedef struct epoll_event WaitEventImpl;
+#elif defined(WAIT_USE_KQUEUE)
+typedef struct kevent WaitEventImpl;
+#elif defined(WAIT_USE_POLL)
+typedef struct pollfd WaitEventImpl;
+#elif defined(WAIT_USE_WIN32)
+typedef HANDLE WaitEventImpl;
+#endif
+
+/* Windows needs an extra element in events_impl. */
+#if defined(WAIT_USE_WIN32)
+#define EXTRA_EVENT_IMPL 1
+#else
+#define EXTRA_EVENT_IMPL 0
+#endif
+
+/* Can be set lower to exercise reallocation code. */
+#define INITIAL_WAIT_EVENT_SET_SIZE 4
+
/* typedef in latch.h */
struct WaitEventSet
{
+ MemoryContext context;
+
int nevents; /* number of registered events */
int nevents_space; /* maximum number of events in this set */
@@ -112,26 +135,17 @@ struct WaitEventSet
*/
bool exit_on_postmaster_death;
+ /*
+ * Array, of nevents_space
Re: WaitLatchOrSocket seems to not count to 4 right...
On 2022/02/08 9:51, Thomas Munro wrote: an index instead of a pointer...) or, since it's a bit silly to add both of those events, maybe we should do: - if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster) - AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET, - NULL, NULL); - if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster) AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET, NULL, NULL); + else if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster) + AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET, + +1 Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: WaitLatchOrSocket seems to not count to 4 right...
On Tue, Feb 8, 2022 at 1:48 PM Fujii Masao wrote:
> On 2022/02/08 7:00, Greg Stark wrote:
> > Unless I'm misreading this code I think the nevents in
> > WaitLatchOrSocket should really be 4 not 3. At least there are 4 calls
> > to AddWaitEventToSet in it and I think it's possible to trigger all 4.
>
> Good catch! I think you're right.
>
> As the quick test, I confirmed that the assertion failure happened when I
> passed four possible events to WaitLatchOrSocket() in postgres_fdw.
>
> TRAP: FailedAssertion("set->nevents < set->nevents_space", File: "latch.c",
> Line: 868, PID: 54424)
Yeah, the assertion shows there's no problem, but we should change it
to 4 (and one day we should make it dynamic and change udata to hold
an index instead of a pointer...) or, since it's a bit silly to add
both of those events, maybe we should do:
- if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
- AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
-
if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster)
AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
NULL, NULL);
+ else if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
+ AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
+
Re: WaitLatchOrSocket seems to not count to 4 right...
On 2022/02/08 7:00, Greg Stark wrote:
Unless I'm misreading this code I think the nevents in
WaitLatchOrSocket should really be 4 not 3. At least there are 4 calls
to AddWaitEventToSet in it and I think it's possible to trigger all 4.
Good catch! I think you're right.
As the quick test, I confirmed that the assertion failure happened when I
passed four possible events to WaitLatchOrSocket() in postgres_fdw.
TRAP: FailedAssertion("set->nevents < set->nevents_space", File: "latch.c",
Line: 868, PID: 54424)
0 postgres0x000107efa49f ExceptionalCondition
+ 223
1 postgres0x000107cbca0c AddWaitEventToSet +
76
2 postgres0x000107cbd86e WaitLatchOrSocket +
430
3 postgres_fdw.so 0x00010848b1aa pgfdw_get_result +
218
4 postgres_fdw.so 0x00010848accb do_sql_command + 75
5 postgres_fdw.so 0x00010848c6b8
configure_remote_session + 40
6 postgres_fdw.so 0x00010848c32d connect_pg_server +
1629
7 postgres_fdw.so 0x00010848aa06 make_new_connection
+ 566
8 postgres_fdw.so 0x000108489a06 GetConnection + 550
9 postgres_fdw.so 0x000108497ba4
postgresBeginForeignScan + 260
10 postgres0x000107a7d79f ExecInitForeignScan
+ 943
11 postgres0x000107a5c8ab ExecInitNode + 683
12 postgres0x000107a5028a InitPlan + 1386
13 postgres0x000107a4fb66
standard_ExecutorStart + 806
14 postgres0x000107a4f833 ExecutorStart + 83
15 postgres0x000107d0277f PortalStart + 735
16 postgres0x000107cfe150 exec_simple_query +
1168
17 postgres0x000107cfd39e PostgresMain + 2110
18 postgres0x000107c07e72 BackendRun + 50
19 postgres0x000107c07438 BackendStartup + 552
20 postgres0x000107c0621c ServerLoop + 716
21 postgres0x000107c039f9 PostmasterMain + 6441
22 postgres0x000107ae20d9 main + 809
23 libdyld.dylib 0x7fff2045cf3d start + 1
24 ??? 0x0003 0x0 + 3
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
WaitLatchOrSocket seems to not count to 4 right...
Unless I'm misreading this code I think the nevents in WaitLatchOrSocket should really be 4 not 3. At least there are 4 calls to AddWaitEventToSet in it and I think it's possible to trigger all 4. I guess it's based on knowing that nobody would actually set WL_EXIT_ON_PM_DEATH and WL_POSTMASTER_DEATH on the same waitset? -- greg
