On Sun, Jul 12, 2020 at 7:22 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> [...complaints about 0005 and 0006...] We already have
> PGEVT_CONNRESET and PGEVT_CONNDESTROY as application-visible connection
> state change events, and I don't see why those aren't sufficient.
I think Horiguchi-san's general idea of using event callbacks for this
sounds much more promising than my earlier idea of exposing a change
counter (that really was terrible). If it can be done with existing
events then that's even better. Perhaps he and/or I can look into
that for the next CF.
In the meantime, here's a rebase of the more straightforward patches
in the stack. These are the ones that deal only with fixed sets of
file descriptors, and they survive check-world on Linux,
Linux+EXEC_BACKEND (with ASLR disabled) and FreeBSD, and at least
check on macOS and Windows (my CI recipes need more work to get
check-world working on those two). There's one user-visible change
that I'd appreciate feedback on: I propose to drop the FATAL error
when the postmaster goes away, to make things more consistent. See
below for more on that.
Responding to earlier review from Horiguchi-san:
On Tue, Mar 10, 2020 at 12:20 PM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
> > 0002: "Use a long lived WaitEventSet for WaitLatch()."
> >
> > In the last version, I had a new function WaitMyLatch(), but that
> > didn't help with recoveryWakeupLatch. Switching between latches
> > doesn't require a syscall, so I didn't want to have a separate WES and
> > function just for that. So I went back to using plain old
> > WaitLatch(), and made it "modify" the latch every time before waiting
> > on CommonWaitSet. An alternative would be to get rid of the concept
> > of latches other than MyLatch, and change the function to
> > WaitMyLatch(). A similar thing happens for exit_on_postmaster_death,
> > for which I didn't want to have a separate WES, so I just set that
> > flag every time. Thoughts?
>
> It is surely an improvement from that we create a full-fledged WES
> every time. The name CommonWaitSet gives an impression as if it is
> used for a variety of waitevent sets, but it is used only by
> WaitLatch. So I would name it LatchWaitSet. With that name I won't be
> surprised by that the function is pointing WL_LATCH_SET by index 0
> without any explanation when calling ModifyWaitSet.
Ok, I changed it to LatchWaitSet. I also replaced the index 0 with a
symbolic name LatchWaitSetLatchPos, to make that clearer.
> @@ -700,7 +739,11 @@ FreeWaitEventSet(WaitEventSet *set)
> ReleaseExternalFD();
> #elif defined(WAIT_USE_KQUEUE)
> close(set->kqueue_fd);
> - ReleaseExternalFD();
> + if (set->kqueue_fd >= 0)
> + {
> + close(set->kqueue_fd);
> + ReleaseExternalFD();
> + }
>
> Did you forget to remove the close() outside the if block?
> Don't we need the same amendment for epoll_fd with kqueue_fd?
Hmm, maybe I screwed that up when resolving a conflict with the
ReleaseExternalFD() stuff. Fixed.
> WaitLatch is defined as "If the latch is already set (and WL_LATCH_SET
> is given), the function returns immediately.". But now the function
> reacts to latch even if WL_LATCH_SET is not set. I think actually it
> is alwys set so I think we need to modify Assert and function comment
> following the change.
It seems a bit silly to call WaitLatch() if you don't want to wait for
a latch, but I think we can keep that comment and logic by assigning
set->latch = NULL when you wait without WL_LATCH_SET. I tried that in
the attached.
> > 0004: "Introduce RemoveWaitEvent()."
> >
> > We'll need that to be able to handle connections that are closed and
> > reopened under the covers by libpq (replication, postgres_fdw). We
> > also wanted this on a couple of other threads for multiplexing FDWs,
> > to be able to adjust the wait set dynamically for a proposed async
> > Append feature.
> >
> > The implementation is a little naive, and leaves holes in the
> > "pollfds" and "handles" arrays (for poll() and win32 implementations).
> > That could be improved with a bit more footwork in a later patch.
> >
> > XXX The Windows version is based on reading documentation. I'd be
> > very interested to know if check-world passes (especially
> > contrib/postgres_fdw and src/test/recovery). Unfortunately my
> > appveyor.yml fu is not yet strong enough.
>
> I didn't find the documentation about INVALID_HANDLE_VALUE in
> lpHandles. Could you give me the URL for that?
I was looking for how you do the equivalent of Unix file descriptor -1
in a call to poll(), and somewhere I read that INVALID_HANDLE_VALUE
has the right effect. I can't find that reference now. Apparently it
works because that's the pseudo-handle value -1 that is returned by
GetCurrentProcess(), and waiting for your own process waits forever so
it's a suitable value for holes in an array of event handles. We
should probably call GetCurrentProcess() instead, but really that is
just stand-in code: we should rewrite it so that we don't need holes!
That might require a second array for use by the poll and win32
implementations. Let's return to that in a later CF?
On Mon, Mar 30, 2020 at 6:15 PM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
> > 0008: "Use WL_EXIT_ON_PM_DEATH in FeBeWaitSet."
> >
> > The FATAL message you get if you happen to be waiting for IO rather
> > than waiting somewhere else seems arbitrarily different. By switching
> > to a standard automatic exit, it opens the possibility of using
> > FeBeWaitSet in a couple more places that would otherwise need to
> > create their own WES (see also [1]). Thoughts?
>
> Don't we really need any message on backend disconnection due to
> postmaster death? As for authentication code, it seems to me the
> rationale for not writing the log is that the connection has not been
> established at the time. (That depends on what we think the
> "connection" is.)
To me, it looks like this variation is just from a time when
postmaster death handling was less standardised. The comment (removed
by this patch) even introduces the topic of postmaster exit as if
you've never heard of it, in this arbitrary location in the tree, one
wait point among many (admittedly one that is often reached). I don't
think there is any good reason for random timing to affect whether or
not you get a hand crafted FATAL message.
However, if others don't like this change, we could drop this patch
and still use FeBeWaitSet for walsender.c. It'd just need to handle
postmaster death explicitly. It felt weird to be adding new code that
has to handle postmaster death explicitly, which is what led me to
notice that the existing FeBeWaitSet coding (and resulting user
experience) is different from other code.
> > 0009: "Use FeBeWaitSet for walsender.c."
> >
> > Enabled by 0008.
>
> It works and doesn't change behavior. But I found it a bit difficult
> to find what events the WaitEventSetWait waits. Maybe a comment at
> the caller sites would be sufficient. I think any comment about the
> bare number "0" as the event position of ModifyWaitEvent is also
> needed.
Agreed. I changed it to use symbolic names
FeBeWaitSet{Socket,Latch}Pos in the new code and also in the
pre-existing code like this.
> By the way I found that pqcomm.c uses -1 instead of PGINVALID_SOCKET
> for AddWaitEventToSet.
Oh yeah, that's a pre-existing problem. Fixed, since that code was
changed by the patch anyway.
From 1f8e1a2a1e9c35e70a68b29bebeddf8cc67304c1 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 13 Jul 2020 13:32:47 +1200
Subject: [PATCH v5 1/7] Use a long lived WaitEventSet for WaitLatch().
Create LatchWaitSet at backend startup time, and use it to
implement WaitLatch(). This avoids repeated epoll/kqueue set-up and
tear-down system calls, and makes sure we don't run into EMFILE later
due to lack of file descriptors.
Reorder SubPostmasterMain() slightly so that we restore the
postmaster pipe and Windows signal before we reach
InitPostmasterChild(), to make this work in EXEC_BACKEND
builds.
Reviewed-by: Kyotaro Horiguchi <horikyota....@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com
---
src/backend/postmaster/postmaster.c | 24 ++++++-------
src/backend/storage/ipc/latch.c | 55 ++++++++++++++++++++++++++---
src/backend/utils/init/miscinit.c | 2 ++
src/include/storage/latch.h | 1 +
4 files changed, 66 insertions(+), 16 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index dec02586c7..fc5ba11fb8 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4896,9 +4896,6 @@ SubPostmasterMain(int argc, char *argv[])
IsPostmasterEnvironment = true;
whereToSendOutput = DestNone;
- /* Setup as postmaster child */
- InitPostmasterChild();
-
/* Setup essential subsystems (to ensure elog() behaves sanely) */
InitializeGUCOptions();
@@ -4913,6 +4910,18 @@ SubPostmasterMain(int argc, char *argv[])
/* Close the postmaster's sockets (as soon as we know them) */
ClosePostmasterPorts(strcmp(argv[1], "--forklog") == 0);
+ /*
+ * Start our win32 signal implementation. This has to be done after we
+ * read the backend variables, because we need to pick up the signal pipe
+ * from the parent process.
+ */
+#ifdef WIN32
+ pgwin32_signal_initialize();
+#endif
+
+ /* Setup as postmaster child */
+ InitPostmasterChild();
+
/*
* Set up memory area for GSS information. Mirrors the code in ConnCreate
* for the non-exec case.
@@ -4956,15 +4965,6 @@ SubPostmasterMain(int argc, char *argv[])
if (strcmp(argv[1], "--forkavworker") == 0)
AutovacuumWorkerIAm();
- /*
- * Start our win32 signal implementation. This has to be done after we
- * read the backend variables, because we need to pick up the signal pipe
- * from the parent process.
- */
-#ifdef WIN32
- pgwin32_signal_initialize();
-#endif
-
/* In EXEC_BACKEND case we will not have inherited these settings */
pqinitmask();
PG_SETMASK(&BlockSig);
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 91fa4b619b..fd4716ffaa 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -56,6 +56,7 @@
#include "storage/latch.h"
#include "storage/pmsignal.h"
#include "storage/shmem.h"
+#include "utils/memutils.h"
/*
* Select the fd readiness primitive to use. Normally the "most modern"
@@ -129,6 +130,11 @@ struct WaitEventSet
#endif
};
+/* A common WaitEventSet used to implement WatchLatch() */
+static WaitEventSet *LatchWaitSet;
+
+#define LatchWaitSetLatchPos 0
+
#ifndef WIN32
/* Are we currently in WaitLatch? The signal handler would like to know. */
static volatile sig_atomic_t waiting = false;
@@ -242,6 +248,24 @@ InitializeLatchSupport(void)
#endif
}
+void
+InitializeLatchWaitSet(void)
+{
+ int latch_pos PG_USED_FOR_ASSERTS_ONLY;
+
+ Assert(LatchWaitSet == NULL);
+
+ /* Set up the WaitEventSet used by WaitLatch(). */
+ LatchWaitSet = CreateWaitEventSet(TopMemoryContext, 2);
+ latch_pos = AddWaitEventToSet(LatchWaitSet, WL_LATCH_SET, PGINVALID_SOCKET,
+ MyLatch, NULL);
+ if (IsUnderPostmaster)
+ AddWaitEventToSet(LatchWaitSet, WL_EXIT_ON_PM_DEATH,
+ PGINVALID_SOCKET, NULL, NULL);
+
+ Assert(latch_pos == LatchWaitSetLatchPos);
+}
+
/*
* Initialize a process-local latch.
*/
@@ -365,8 +389,31 @@ int
WaitLatch(Latch *latch, int wakeEvents, long timeout,
uint32 wait_event_info)
{
- return WaitLatchOrSocket(latch, wakeEvents, PGINVALID_SOCKET, timeout,
- wait_event_info);
+ WaitEvent event;
+
+ /* Postmaster-managed callers must handle postmaster death somehow. */
+ Assert(!IsUnderPostmaster ||
+ (wakeEvents & WL_EXIT_ON_PM_DEATH) ||
+ (wakeEvents & WL_POSTMASTER_DEATH));
+
+ /*
+ * Some callers may have a latch other than MyLatch, or no latch at all, or
+ * want to handle postmaster death differently. It's cheap to assign
+ * those, so just do it every time.
+ */
+ if (!(wakeEvents & WL_LATCH_SET))
+ latch = NULL;
+ ModifyWaitEvent(LatchWaitSet, LatchWaitSetLatchPos, WL_LATCH_SET, latch);
+ LatchWaitSet->exit_on_postmaster_death =
+ ((wakeEvents & WL_EXIT_ON_PM_DEATH) != 0);
+
+ if (WaitEventSetWait(LatchWaitSet,
+ (wakeEvents & WL_TIMEOUT) ? timeout : -1,
+ &event, 1,
+ wait_event_info) == 0)
+ return WL_TIMEOUT;
+ else
+ return event.events;
}
/*
@@ -830,7 +877,8 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch,
/*
* Change the event mask and, in the WL_LATCH_SET case, the latch associated
- * with the WaitEvent.
+ * with the WaitEvent. The latch may be changed to NULL to disable the latch
+ * temporarily, and then set back to a latch later.
*
* 'pos' is the id returned by AddWaitEventToSet.
*/
@@ -862,7 +910,6 @@ ModifyWaitEvent(WaitEventSet *set, int pos, uint32 events, Latch *latch)
if (event->events & WL_LATCH_SET &&
events != event->events)
{
- /* we could allow to disable latch events for a while */
elog(ERROR, "cannot modify latch event");
}
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index cca9704d2d..cf8f9579c3 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -120,6 +120,7 @@ InitPostmasterChild(void)
InitializeLatchSupport();
MyLatch = &LocalLatchData;
InitLatch(MyLatch);
+ InitializeLatchWaitSet();
/*
* If possible, make this process a group leader, so that the postmaster
@@ -152,6 +153,7 @@ InitStandaloneProcess(const char *argv0)
InitializeLatchSupport();
MyLatch = &LocalLatchData;
InitLatch(MyLatch);
+ InitializeLatchWaitSet();
/* Compute paths, no postmaster to inherit from */
if (my_exec_path[0] == '\0')
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 46ae56cae3..7c742021fb 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -176,6 +176,7 @@ extern int WaitLatch(Latch *latch, int wakeEvents, long timeout,
uint32 wait_event_info);
extern int WaitLatchOrSocket(Latch *latch, int wakeEvents,
pgsocket sock, long timeout, uint32 wait_event_info);
+extern void InitializeLatchWaitSet(void);
/*
* Unix implementation uses SIGUSR1 for inter-process signaling.
--
2.20.1
From 76026728c2ba2fff65e2bdad4ebd97e674afc594 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 13 Jul 2020 13:44:36 +1200
Subject: [PATCH v5 2/7] Use WaitLatch() for condition variables.
Previously, condition_variable.c created its own long lived
WaitEventSet to avoid extra system calls. WaitLatch() now
does that, so there is no point in wasting an extra kernel
descriptor.
Reviewed-by: Kyotaro Horiguchi <horikyota....@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com
---
src/backend/storage/lmgr/condition_variable.c | 28 ++++---------------
1 file changed, 5 insertions(+), 23 deletions(-)
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 37b6a4eecd..2ec00397b4 100644
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -30,9 +30,6 @@
/* Initially, we are not prepared to sleep on any condition variable. */
static ConditionVariable *cv_sleep_target = NULL;
-/* Reusable WaitEventSet. */
-static WaitEventSet *cv_wait_event_set = NULL;
-
/*
* Initialize a condition variable.
*/
@@ -62,23 +59,6 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
{
int pgprocno = MyProc->pgprocno;
- /*
- * If first time through in this process, create a WaitEventSet, which
- * we'll reuse for all condition variable sleeps.
- */
- if (cv_wait_event_set == NULL)
- {
- WaitEventSet *new_event_set;
-
- new_event_set = CreateWaitEventSet(TopMemoryContext, 2);
- AddWaitEventToSet(new_event_set, WL_LATCH_SET, PGINVALID_SOCKET,
- MyLatch, NULL);
- AddWaitEventToSet(new_event_set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
- /* Don't set cv_wait_event_set until we have a correct WES. */
- cv_wait_event_set = new_event_set;
- }
-
/*
* If some other sleep is already prepared, cancel it; this is necessary
* because we have just one static variable tracking the prepared sleep,
@@ -135,6 +115,7 @@ ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
long cur_timeout = -1;
instr_time start_time;
instr_time cur_time;
+ int wait_events;
/*
* If the caller didn't prepare to sleep explicitly, then do so now and
@@ -166,19 +147,20 @@ ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
INSTR_TIME_SET_CURRENT(start_time);
Assert(timeout >= 0 && timeout <= INT_MAX);
cur_timeout = timeout;
+ wait_events = WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH;
}
+ else
+ wait_events = WL_LATCH_SET | WL_EXIT_ON_PM_DEATH;
while (true)
{
- WaitEvent event;
bool done = false;
/*
* Wait for latch to be set. (If we're awakened for some other
* reason, the code below will cope anyway.)
*/
- (void) WaitEventSetWait(cv_wait_event_set, cur_timeout, &event, 1,
- wait_event_info);
+ (void) WaitLatch(MyLatch, wait_events, cur_timeout, wait_event_info);
/* Reset latch before examining the state of the wait list. */
ResetLatch(MyLatch);
--
2.20.1
From 33e2f433cfeb7234cdef23c12466736707cda7ca Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 13 Jul 2020 13:46:55 +1200
Subject: [PATCH v5 3/7] Introduce a WaitEventSet for the stats collector.
This avoids avoids several system calls for every wait.
Reviewed-by: Kyotaro Horiguchi <horikyota....@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com
---
src/backend/postmaster/pgstat.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 88992c2da2..7dbe5d62fd 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4458,6 +4458,8 @@ PgstatCollectorMain(int argc, char *argv[])
int len;
PgStat_Msg msg;
int wr;
+ WaitEvent event;
+ WaitEventSet *wes;
/*
* Ignore all signals usually bound to some action in the postmaster,
@@ -4485,6 +4487,12 @@ PgstatCollectorMain(int argc, char *argv[])
pgStatRunningInCollector = true;
pgStatDBHash = pgstat_read_statsfiles(InvalidOid, true, true);
+ /* Prepare to wait for our latch or data in our socket. */
+ wes = CreateWaitEventSet(CurrentMemoryContext, 3);
+ 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);
+
/*
* Loop to process messages until we get SIGQUIT or detect ungraceful
* death of our parent postmaster.
@@ -4672,10 +4680,7 @@ PgstatCollectorMain(int argc, char *argv[])
/* Sleep until there's something to do */
#ifndef WIN32
- wr = WaitLatchOrSocket(MyLatch,
- WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_SOCKET_READABLE,
- pgStatSock, -1L,
- WAIT_EVENT_PGSTAT_MAIN);
+ wr = WaitEventSetWait(wes, -1L, &event, 1, WAIT_EVENT_PGSTAT_MAIN);
#else
/*
@@ -4688,18 +4693,15 @@ PgstatCollectorMain(int argc, char *argv[])
* to not provoke "using stale statistics" complaints from
* backend_read_statsfile.
*/
- wr = WaitLatchOrSocket(MyLatch,
- WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_SOCKET_READABLE | WL_TIMEOUT,
- pgStatSock,
- 2 * 1000L /* msec */ ,
- WAIT_EVENT_PGSTAT_MAIN);
+ wr = WaitEventSetWait(wes, 2 * 1000L /* msec */, &event, 1,
+ WAIT_EVENT_PGSTAT_MAIN);
#endif
/*
* Emergency bailout if postmaster has died. This is to avoid the
* necessity for manual cleanup of all postmaster children.
*/
- if (wr & WL_POSTMASTER_DEATH)
+ if (wr == 1 && event.events == WL_POSTMASTER_DEATH)
break;
} /* end of outer loop */
@@ -4708,6 +4710,8 @@ PgstatCollectorMain(int argc, char *argv[])
*/
pgstat_write_statsfiles(true, true);
+ FreeWaitEventSet(wes);
+
exit(0);
}
--
2.20.1
From 3a12cc7a75c73f9f63b22a48467ace221b59ccc1 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 24 Feb 2020 23:51:09 +1300
Subject: [PATCH v5 4/7] 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....@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 2ae507a902..aec0926d93 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 ac986c0505..6e91581c0b 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 ff88d4da1e1e96f785be6f277187576e84bb4eda Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 24 Feb 2020 23:48:29 +1300
Subject: [PATCH v5 5/7] 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....@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 aec0926d93..2d5cfbd6f8 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 6e91581c0b..98fb60ffca 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 cf8f9579c3..bcbf414db5 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -176,7 +176,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
@@ -195,7 +195,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 b1152475ac..d96f69070d 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 4363808a8d714c8c157d47946002a1ce0b2b9599 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 24 Feb 2020 23:48:29 +1300
Subject: [PATCH v5 6/7] 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....@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGJAC4Oqao%3DqforhNey20J8CiG2R%3DoBPqvfR0vOJrFysGw%40mail.gmail.com
---
src/backend/replication/walsender.c | 32 ++++++++++++++---------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 5e2210dd7b..97d9a06620 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1297,7 +1297,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 */
@@ -1314,13 +1314,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);
@@ -1398,6 +1396,7 @@ WalSndWaitForWal(XLogRecPtr loc)
for (;;)
{
+ WaitEvent event;
long sleeptime;
/* Clear any already-pending wakeups */
@@ -1493,15 +1492,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 */
@@ -2344,9 +2342,9 @@ WalSndLoop(WalSndSendDataCallback send_data)
{
long sleeptime;
int wakeEvents;
+ WaitEvent event;
- wakeEvents = WL_LATCH_SET | WL_EXIT_ON_PM_DEATH | WL_TIMEOUT |
- WL_SOCKET_READABLE;
+ wakeEvents = WL_SOCKET_READABLE;
/*
* Use fresh timestamp, not last_processing, to reduce the chance
@@ -2358,9 +2356,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
