On Fri, Jul 31, 2020 at 9:42 AM Thomas Munro <thomas.mu...@gmail.com> wrote: > On Thu, Jul 30, 2020 at 5:50 PM Thomas Munro <thomas.mu...@gmail.com> wrote: > > I pushed those three patches, but will wait for more discussion on the rest. > > And here's a rebase, to make cfbot happy.
And again. To restate the two goals of the remaining patches: 1. Use FeBeWaitSet for walsender, instead of setting up and tearing down temporary WaitEventSets all the time. 2. Use the standard WL_EXIT_ON_PM_DEATH flag for FeBeWaitSet, instead of the special case ereport(FATAL, ... "terminating connection due to unexpected postmaster exit" ...). For point 2, the question I am raising is: why should users get a special FATAL message in some places and not others, for PM death? However, if people are attached to that behaviour, we could still achieve goal 1 without goal 2 by handling PM death explicitly in walsender.c and I'd be happy to post an alternative patch set like that.
From 72fa3b6858b3ab121fd60533ed7d6a4dcdbc4a5c Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.munro@gmail.com> Date: Mon, 24 Feb 2020 23:51:09 +1300 Subject: [PATCH v7 1/3] Use WL_EXIT_ON_PM_DEATH in FeBeWaitSet. Previously, we'd give either a FATAL message or a silent exit() when we detected postmaster death, depending on which wait point we were at. Make the exit more uniform, by using the standard exit facility. This will allow a later patch to use FeBeWaitSet in a new location without having to add more explicit handling for postmaster death, in line with our policy of reducing such clutter. Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Discussion: https://postgr.es/m/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com --- src/backend/libpq/be-secure.c | 28 ---------------------------- src/backend/libpq/pqcomm.c | 2 +- 2 files changed, 1 insertion(+), 29 deletions(-) diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c index 4cf139a223..746fce8288 100644 --- a/src/backend/libpq/be-secure.c +++ b/src/backend/libpq/be-secure.c @@ -184,28 +184,6 @@ retry: WaitEventSetWait(FeBeWaitSet, -1 /* no timeout */ , &event, 1, WAIT_EVENT_CLIENT_READ); - /* - * If the postmaster has died, it's not safe to continue running, - * because it is the postmaster's job to kill us if some other backend - * exits uncleanly. Moreover, we won't run very well in this state; - * helper processes like walwriter and the bgwriter will exit, so - * performance may be poor. Finally, if we don't exit, pg_ctl will be - * unable to restart the postmaster without manual intervention, so no - * new connections can be accepted. Exiting clears the deck for a - * postmaster restart. - * - * (Note that we only make this check when we would otherwise sleep on - * our latch. We might still continue running for a while if the - * postmaster is killed in mid-query, or even through multiple queries - * if we never have to wait for read. We don't want to burn too many - * cycles checking for this very rare condition, and this should cause - * us to exit quickly in most cases.) - */ - if (event.events & WL_POSTMASTER_DEATH) - ereport(FATAL, - (errcode(ERRCODE_ADMIN_SHUTDOWN), - errmsg("terminating connection due to unexpected postmaster exit"))); - /* Handle interrupt. */ if (event.events & WL_LATCH_SET) { @@ -296,12 +274,6 @@ retry: WaitEventSetWait(FeBeWaitSet, -1 /* no timeout */ , &event, 1, WAIT_EVENT_CLIENT_WRITE); - /* See comments in secure_read. */ - if (event.events & WL_POSTMASTER_DEATH) - ereport(FATAL, - (errcode(ERRCODE_ADMIN_SHUTDOWN), - errmsg("terminating connection due to unexpected postmaster exit"))); - /* Handle interrupt. */ if (event.events & WL_LATCH_SET) { diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 1e6b6db540..289d9bd2da 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -222,7 +222,7 @@ pq_init(void) AddWaitEventToSet(FeBeWaitSet, WL_SOCKET_WRITEABLE, MyProcPort->sock, NULL, NULL); AddWaitEventToSet(FeBeWaitSet, WL_LATCH_SET, -1, MyLatch, NULL); - AddWaitEventToSet(FeBeWaitSet, WL_POSTMASTER_DEATH, -1, NULL, NULL); + AddWaitEventToSet(FeBeWaitSet, WL_EXIT_ON_PM_DEATH, -1, NULL, NULL); } /* -------------------------------- -- 2.20.1
From ec7348f4fc794eed6cfd6bb3897086ef29c213ae Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.munro@gmail.com> Date: Mon, 24 Feb 2020 23:48:29 +1300 Subject: [PATCH v7 2/3] Introduce symbolic names for FeBeWaitSet positions. Previously we used hard coded 0 and 1 to refer to the socket and latch. Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Discussion: https://postgr.es/m/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com --- src/backend/libpq/be-secure.c | 4 ++-- src/backend/libpq/pqcomm.c | 18 +++++++++++++++--- src/backend/utils/init/miscinit.c | 4 ++-- src/include/libpq/libpq.h | 3 +++ 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c index 746fce8288..0531db8fce 100644 --- a/src/backend/libpq/be-secure.c +++ b/src/backend/libpq/be-secure.c @@ -179,7 +179,7 @@ retry: Assert(waitfor); - ModifyWaitEvent(FeBeWaitSet, 0, waitfor, NULL); + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, waitfor, NULL); WaitEventSetWait(FeBeWaitSet, -1 /* no timeout */ , &event, 1, WAIT_EVENT_CLIENT_READ); @@ -269,7 +269,7 @@ retry: Assert(waitfor); - ModifyWaitEvent(FeBeWaitSet, 0, waitfor, NULL); + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, waitfor, NULL); WaitEventSetWait(FeBeWaitSet, -1 /* no timeout */ , &event, 1, WAIT_EVENT_CLIENT_WRITE); diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 289d9bd2da..5e006ef493 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -191,6 +191,9 @@ WaitEventSet *FeBeWaitSet; void pq_init(void) { + int socket_pos PG_USED_FOR_ASSERTS_ONLY; + int latch_pos PG_USED_FOR_ASSERTS_ONLY; + /* initialize state variables */ PqSendBufferSize = PQ_SEND_BUFFER_SIZE; PqSendBuffer = MemoryContextAlloc(TopMemoryContext, PqSendBufferSize); @@ -219,10 +222,19 @@ pq_init(void) #endif FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, 3); - AddWaitEventToSet(FeBeWaitSet, WL_SOCKET_WRITEABLE, MyProcPort->sock, + socket_pos = AddWaitEventToSet(FeBeWaitSet, WL_SOCKET_WRITEABLE, + MyProcPort->sock, NULL, NULL); + latch_pos = AddWaitEventToSet(FeBeWaitSet, WL_LATCH_SET, PGINVALID_SOCKET, + MyLatch, NULL); + AddWaitEventToSet(FeBeWaitSet, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET, NULL, NULL); - AddWaitEventToSet(FeBeWaitSet, WL_LATCH_SET, -1, MyLatch, NULL); - AddWaitEventToSet(FeBeWaitSet, WL_EXIT_ON_PM_DEATH, -1, NULL, NULL); + + /* + * The event positions match the order we added them, but let's sanity + * check them to be sure. + */ + Assert(socket_pos == FeBeWaitSetSocketPos); + Assert(latch_pos == FeBeWaitSetLatchPos); } /* -------------------------------- diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 0f67b99cc5..69e945f3a1 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -202,7 +202,7 @@ SwitchToSharedLatch(void) MyLatch = &MyProc->procLatch; if (FeBeWaitSet) - ModifyWaitEvent(FeBeWaitSet, 1, WL_LATCH_SET, MyLatch); + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET, MyLatch); /* * Set the shared latch as the local one might have been set. This @@ -221,7 +221,7 @@ SwitchBackToLocalLatch(void) MyLatch = &LocalLatchData; if (FeBeWaitSet) - ModifyWaitEvent(FeBeWaitSet, 1, WL_LATCH_SET, MyLatch); + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET, MyLatch); SetLatch(MyLatch); } diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h index a55898c85a..3037b5789d 100644 --- a/src/include/libpq/libpq.h +++ b/src/include/libpq/libpq.h @@ -55,6 +55,9 @@ extern const PGDLLIMPORT PQcommMethods *PqCommMethods; */ extern WaitEventSet *FeBeWaitSet; +#define FeBeWaitSetSocketPos 0 +#define FeBeWaitSetLatchPos 1 + extern int StreamServerPort(int family, const char *hostName, unsigned short portNumber, const char *unixSocketDir, pgsocket ListenSocket[], int MaxListen); -- 2.20.1
From cc9731331feb91d804f3ba18596eee0eb4986ca6 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.munro@gmail.com> Date: Mon, 24 Feb 2020 23:48:29 +1300 Subject: [PATCH v7 3/3] Use FeBeWaitSet for walsender.c. This avoids the need to set up and tear down a new WaitEventSet every time we wait. Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Discussion: https://postgr.es/m/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com --- src/backend/replication/walsender.c | 35 ++++++++++++++--------------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index fe0d368a35..4b86fc4e30 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1293,7 +1293,7 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid, /* If we have pending write here, go to slow path */ for (;;) { - int wakeEvents; + WaitEvent event; long sleeptime; /* Check for input from the client */ @@ -1310,13 +1310,11 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid, sleeptime = WalSndComputeSleeptime(GetCurrentTimestamp()); - wakeEvents = WL_LATCH_SET | WL_EXIT_ON_PM_DEATH | - WL_SOCKET_WRITEABLE | WL_SOCKET_READABLE | WL_TIMEOUT; - /* Sleep until something happens or we time out */ - (void) WaitLatchOrSocket(MyLatch, wakeEvents, - MyProcPort->sock, sleeptime, - WAIT_EVENT_WAL_SENDER_WRITE_DATA); + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, + WL_SOCKET_WRITEABLE | WL_SOCKET_READABLE, NULL); + (void) WaitEventSetWait(FeBeWaitSet, sleeptime, &event, 1, + WAIT_EVENT_WAL_SENDER_WRITE_DATA); /* Clear any already-pending wakeups */ ResetLatch(MyLatch); @@ -1394,6 +1392,7 @@ WalSndWaitForWal(XLogRecPtr loc) for (;;) { + WaitEvent event; long sleeptime; /* Clear any already-pending wakeups */ @@ -1486,15 +1485,14 @@ WalSndWaitForWal(XLogRecPtr loc) */ sleeptime = WalSndComputeSleeptime(GetCurrentTimestamp()); - wakeEvents = WL_LATCH_SET | WL_EXIT_ON_PM_DEATH | - WL_SOCKET_READABLE | WL_TIMEOUT; + wakeEvents = WL_SOCKET_READABLE; if (pq_is_send_pending()) wakeEvents |= WL_SOCKET_WRITEABLE; - (void) WaitLatchOrSocket(MyLatch, wakeEvents, - MyProcPort->sock, sleeptime, - WAIT_EVENT_WAL_SENDER_WAIT_WAL); + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, wakeEvents, NULL); + (void) WaitEventSetWait(FeBeWaitSet, sleeptime, &event, 1, + WAIT_EVENT_WAL_SENDER_WAIT_WAL); } /* reactivate latch so WalSndLoop knows to continue */ @@ -2353,11 +2351,12 @@ WalSndLoop(WalSndSendDataCallback send_data) { long sleeptime; int wakeEvents; - - wakeEvents = WL_LATCH_SET | WL_EXIT_ON_PM_DEATH | WL_TIMEOUT; + WaitEvent event; if (!streamingDoneReceiving) - wakeEvents |= WL_SOCKET_READABLE; + wakeEvents = WL_SOCKET_READABLE; + else + wakeEvents = 0; /* * Use fresh timestamp, not last_processing, to reduce the chance @@ -2369,9 +2368,9 @@ WalSndLoop(WalSndSendDataCallback send_data) wakeEvents |= WL_SOCKET_WRITEABLE; /* Sleep until something happens or we time out */ - (void) WaitLatchOrSocket(MyLatch, wakeEvents, - MyProcPort->sock, sleeptime, - WAIT_EVENT_WAL_SENDER_MAIN); + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, wakeEvents, NULL); + (void) WaitEventSetWait(FeBeWaitSet, sleeptime, &event, 1, + WAIT_EVENT_WAL_SENDER_MAIN); } } } -- 2.20.1
