On Thu, Nov 19, 2020 at 4:49 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
> I'll add this to the next commitfest.
Let's see if this version fixes the Windows compile error and warning
reported by cfbot.
From 3eb542891a11d39047b28f6f33ae4e3d25bdd510 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sat, 8 Aug 2020 15:08:09 +1200
Subject: [PATCH v4 1/5] Optimize latches to send fewer signals.

Don't send signals to processes that aren't currently sleeping.

Author: Andres Freund <and...@anarazel.de>
Discussion: https://postgr.es/m/ca+hukgjjxpdpzbe0a3hyuywbvazuc89yx3jk9rfzgfv_khu...@mail.gmail.com
---
 src/backend/storage/ipc/latch.c | 24 ++++++++++++++++++++++++
 src/include/storage/latch.h     |  1 +
 2 files changed, 25 insertions(+)

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 24afc47d51..017b5c0c0f 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -274,6 +274,7 @@ void
 InitLatch(Latch *latch)
 {
 	latch->is_set = false;
+	latch->maybe_sleeping = false;
 	latch->owner_pid = MyProcPid;
 	latch->is_shared = false;
 
@@ -321,6 +322,7 @@ InitSharedLatch(Latch *latch)
 #endif
 
 	latch->is_set = false;
+	latch->maybe_sleeping = false;
 	latch->owner_pid = 0;
 	latch->is_shared = true;
 }
@@ -523,6 +525,10 @@ SetLatch(Latch *latch)
 
 	latch->is_set = true;
 
+	pg_memory_barrier();
+	if (!latch->maybe_sleeping)
+		return;
+
 #ifndef WIN32
 
 	/*
@@ -589,6 +595,7 @@ ResetLatch(Latch *latch)
 {
 	/* Only the owner should reset the latch */
 	Assert(latch->owner_pid == MyProcPid);
+	Assert(latch->maybe_sleeping == false);
 
 	latch->is_set = false;
 
@@ -1289,6 +1296,14 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
 		 * ordering, so that we cannot miss seeing is_set if a notification
 		 * has already been queued.
 		 */
+		if (set->latch && !set->latch->is_set)
+		{
+			/* about to sleep on a latch */
+			set->latch->maybe_sleeping = true;
+			pg_memory_barrier();
+			/* and recheck */
+		}
+
 		if (set->latch && set->latch->is_set)
 		{
 			occurred_events->fd = PGINVALID_SOCKET;
@@ -1299,6 +1314,9 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
 			occurred_events++;
 			returned_events++;
 
+			/* could have been set above */
+			set->latch->maybe_sleeping = false;
+
 			break;
 		}
 
@@ -1310,6 +1328,12 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
 		rc = WaitEventSetWaitBlock(set, cur_timeout,
 								   occurred_events, nevents);
 
+		if (set->latch)
+		{
+			Assert(set->latch->maybe_sleeping);
+			set->latch->maybe_sleeping = false;
+		}
+
 		if (rc == -1)
 			break;				/* timeout occurred */
 		else
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 7c742021fb..d0da7e5d31 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -110,6 +110,7 @@
 typedef struct Latch
 {
 	sig_atomic_t is_set;
+	sig_atomic_t maybe_sleeping;
 	bool		is_shared;
 	int			owner_pid;
 #ifdef WIN32
-- 
2.20.1

From e0709315a443c76058a5a61e59bb54e248981d5d Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Thu, 26 Nov 2020 10:18:29 +1300
Subject: [PATCH v4 2/5] Use SIGURG rather than SIGUSR1 for latches.

Traditionally, SIGUSR1 has been overloaded for ad-hoc signals,
procsignal.c signals and latch.c wakeups.  Move that last use over to a
new dedicated signal.  SIGURG is normally used to report out-of-band
socket data, but PostgreSQL doesn't use that.

The signal handler is now installed in all postmaster children by
InitializeLatchSupport().  Those wishing to disconnect from it should
call ShutdownLatchSupport() which will set it back to SIG_IGN.

Future patches will use this separation to avoid the need for a signal
handler on some operating systems.

Discussion: https://postgr.es/m/ca+hukgjjxpdpzbe0a3hyuywbvazuc89yx3jk9rfzgfv_khu...@mail.gmail.com
---
 src/backend/postmaster/bgworker.c    | 19 ++---------
 src/backend/postmaster/postmaster.c  |  4 +++
 src/backend/storage/ipc/latch.c      | 50 ++++++++++++++++++----------
 src/backend/storage/ipc/procsignal.c |  2 --
 src/include/storage/latch.h          | 11 +-----
 5 files changed, 40 insertions(+), 46 deletions(-)

diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 5a9a0e3435..9241b4edf4 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -654,22 +654,6 @@ bgworker_die(SIGNAL_ARGS)
 					MyBgworkerEntry->bgw_type)));
 }
 
-/*
- * Standard SIGUSR1 handler for unconnected workers
- *
- * Here, we want to make sure an unconnected worker will at least heed
- * latch activity.
- */
-static void
-bgworker_sigusr1_handler(SIGNAL_ARGS)
-{
-	int			save_errno = errno;
-
-	latch_sigusr1_handler();
-
-	errno = save_errno;
-}
-
 /*
  * Start a new background worker
  *
@@ -700,6 +684,7 @@ StartBackgroundWorker(void)
 	 */
 	if ((worker->bgw_flags & BGWORKER_SHMEM_ACCESS) == 0)
 	{
+		ShutdownLatchSupport();
 		dsm_detach_all();
 		PGSharedMemoryDetach();
 	}
@@ -727,7 +712,7 @@ StartBackgroundWorker(void)
 	else
 	{
 		pqsignal(SIGINT, SIG_IGN);
-		pqsignal(SIGUSR1, bgworker_sigusr1_handler);
+		pqsignal(SIGUSR1, SIG_IGN);
 		pqsignal(SIGFPE, SIG_IGN);
 	}
 	pqsignal(SIGTERM, bgworker_die);
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b7799ed1d2..1b64034fc0 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -655,6 +655,10 @@ PostmasterMain(int argc, char *argv[])
 	pqsignal_pm(SIGUSR2, dummy_handler);	/* unused, reserve for children */
 	pqsignal_pm(SIGCHLD, reaper);	/* handle child termination */
 
+#ifdef SIGURG
+	pqsignal_pm(SIGURG, SIG_IGN);	/* ignored */
+#endif
+
 	/*
 	 * No other place in Postgres should touch SIGTTIN/SIGTTOU handling.  We
 	 * ignore those signals in a postmaster environment, so that there is no
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 017b5c0c0f..8e5513260a 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -16,7 +16,7 @@
  *
  * When SetLatch is called from the same process that owns the latch,
  * SetLatch writes the byte directly to the pipe. If it's owned by another
- * process, SIGUSR1 is sent and the signal handler in the waiting process
+ * process, SIGURG is sent and the signal handler in the waiting process
  * writes the byte to the pipe on behalf of the signaling process.
  *
  * The Windows implementation uses Windows events that are inherited by all
@@ -148,6 +148,7 @@ static int	selfpipe_writefd = -1;
 static int	selfpipe_owner_pid = 0;
 
 /* Private function prototypes */
+static void latch_sigurg_handler(SIGNAL_ARGS);
 static void sendSelfPipeByte(void);
 static void drainSelfPipe(void);
 #endif							/* WIN32 */
@@ -244,6 +245,8 @@ InitializeLatchSupport(void)
 	/* Tell fd.c about these two long-lived FDs */
 	ReserveExternalFD();
 	ReserveExternalFD();
+
+	pqsignal(SIGURG, latch_sigurg_handler);
 #else
 	/* currently, nothing to do here for Windows */
 #endif
@@ -267,6 +270,24 @@ InitializeLatchWaitSet(void)
 	Assert(latch_pos == LatchWaitSetLatchPos);
 }
 
+void
+ShutdownLatchSupport(void)
+{
+	pqsignal(SIGURG, SIG_IGN);
+
+	if (LatchWaitSet)
+	{
+		FreeWaitEventSet(LatchWaitSet);
+		LatchWaitSet = NULL;
+	}
+
+	close(selfpipe_readfd);
+	close(selfpipe_writefd);
+	selfpipe_readfd = -1;
+	selfpipe_writefd = -1;
+	selfpipe_owner_pid = InvalidPid;
+}
+
 /*
  * Initialize a process-local latch.
  */
@@ -335,10 +356,6 @@ InitSharedLatch(Latch *latch)
  * any sort of locking here, meaning that we could fail to detect the error
  * if two processes try to own the same latch at about the same time.  If
  * there is any risk of that, caller must provide an interlock to prevent it.
- *
- * In any process that calls OwnLatch(), make sure that
- * latch_sigusr1_handler() is called from the SIGUSR1 signal handler,
- * as shared latches use SIGUSR1 for inter-process communication.
  */
 void
 OwnLatch(Latch *latch)
@@ -562,7 +579,7 @@ SetLatch(Latch *latch)
 			sendSelfPipeByte();
 	}
 	else
-		kill(owner_pid, SIGUSR1);
+		kill(owner_pid, SIGURG);
 #else
 
 	/*
@@ -1285,7 +1302,7 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
 		 * the pipe-buffer fill up we're still ok, because the pipe is in
 		 * nonblocking mode. It's unlikely for that to happen, because the
 		 * self pipe isn't filled unless we're blocking (waiting = true), or
-		 * from inside a signal handler in latch_sigusr1_handler().
+		 * from inside a signal handler in latch_sigurg_handler().
 		 *
 		 * On windows, we'll also notice if there's a pending event for the
 		 * latch when blocking, but there's no danger of anything filling up,
@@ -1953,22 +1970,21 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 }
 #endif
 
+#ifndef WIN32
 /*
- * SetLatch uses SIGUSR1 to wake up the process waiting on the latch.
- *
- * Wake up WaitLatch, if we're waiting.  (We might not be, since SIGUSR1 is
- * overloaded for multiple purposes; or we might not have reached WaitLatch
- * yet, in which case we don't need to fill the pipe either.)
+ * SetLatch uses SIGURG to wake up the process waiting on the latch.
  *
- * NB: when calling this in a signal handler, be sure to save and restore
- * errno around it.
+ * Wake up WaitLatch, if we're waiting.
  */
-#ifndef WIN32
-void
-latch_sigusr1_handler(void)
+static void
+latch_sigurg_handler(SIGNAL_ARGS)
 {
+	int			save_errno = errno;
+
 	if (waiting)
 		sendSelfPipeByte();
+
+	errno = save_errno;
 }
 #endif							/* !WIN32 */
 
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index ffe67acea1..a6b60ca15b 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -587,7 +587,5 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
 
 	SetLatch(MyLatch);
 
-	latch_sigusr1_handler();
-
 	errno = save_errno;
 }
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index d0da7e5d31..382311ab4a 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -163,6 +163,7 @@ extern void OwnLatch(Latch *latch);
 extern void DisownLatch(Latch *latch);
 extern void SetLatch(Latch *latch);
 extern void ResetLatch(Latch *latch);
+extern void ShutdownLatchSupport(void);
 
 extern WaitEventSet *CreateWaitEventSet(MemoryContext context, int nevents);
 extern void FreeWaitEventSet(WaitEventSet *set);
@@ -179,14 +180,4 @@ 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.
- * Win32 doesn't need this.
- */
-#ifndef WIN32
-extern void latch_sigusr1_handler(void);
-#else
-#define latch_sigusr1_handler()  ((void) 0)
-#endif
-
 #endif							/* LATCH_H */
-- 
2.20.1

From b955f1b6062de51d747b87a0efe668fc7e460a81 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Thu, 26 Nov 2020 10:21:15 +1300
Subject: [PATCH v4 3/5] Use signalfd for epoll latches.

Cut down on system calls and other overheads by waiting on a signalfd
instead of a signal handler and self-pipe.

Discussion: https://postgr.es/m/ca+hukgjjxpdpzbe0a3hyuywbvazuc89yx3jk9rfzgfv_khu...@mail.gmail.com
---
 src/backend/libpq/pqsignal.c    |   4 +-
 src/backend/storage/ipc/latch.c | 161 ++++++++++++++++++++++----------
 2 files changed, 115 insertions(+), 50 deletions(-)

diff --git a/src/backend/libpq/pqsignal.c b/src/backend/libpq/pqsignal.c
index 9289493118..8fb937ab59 100644
--- a/src/backend/libpq/pqsignal.c
+++ b/src/backend/libpq/pqsignal.c
@@ -35,13 +35,15 @@ sigset_t	UnBlockSig,
  * collection; it's essentially BlockSig minus SIGTERM, SIGQUIT, SIGALRM.
  *
  * UnBlockSig is the set of signals to block when we don't want to block
- * signals (is this ever nonzero??)
+ * signals.
  */
 void
 pqinitmask(void)
 {
 	sigemptyset(&UnBlockSig);
 
+	/* Note: InitializeLatchSupport() modifies UnBlockSig. */
+
 	/* First set all signals, then clear some. */
 	sigfillset(&BlockSig);
 	sigfillset(&StartupBlockSig);
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 8e5513260a..6d7bc8e328 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -3,21 +3,20 @@
  * latch.c
  *	  Routines for inter-process latches
  *
- * The Unix implementation uses the so-called self-pipe trick to overcome the
- * race condition involved with poll() (or epoll_wait() on linux) and setting
- * a global flag in the signal handler. When a latch is set and the current
- * process is waiting for it, the signal handler wakes up the poll() in
- * WaitLatch by writing a byte to a pipe. A signal by itself doesn't interrupt
- * poll() on all platforms, and even on platforms where it does, a signal that
- * arrives just before the poll() call does not prevent poll() from entering
- * sleep. An incoming byte on a pipe however reliably interrupts the sleep,
- * and causes poll() to return immediately even if the signal arrives before
- * poll() begins.
+ * The poll() implementation uses the so-called self-pipe trick to overcome the
+ * race condition involved with poll() and setting a global flag in the signal
+ * handler. When a latch is set and the current process is waiting for it, the
+ * signal handler wakes up the poll() in WaitLatch by writing a byte to a pipe.
+ * A signal by itself doesn't interrupt poll() on all platforms, and even on
+ * platforms where it does, a signal that arrives just before the poll() call
+ * does not prevent poll() from entering sleep. An incoming byte on a pipe
+ * however reliably interrupts the sleep, and causes poll() to return
+ * immediately even if the signal arrives before poll() begins.
  *
- * When SetLatch is called from the same process that owns the latch,
- * SetLatch writes the byte directly to the pipe. If it's owned by another
- * process, SIGURG is sent and the signal handler in the waiting process
- * writes the byte to the pipe on behalf of the signaling process.
+ * The epoll() implementation overcomes the race with a different technique: it
+ * keeps SIGURG blocked and consumes from signalfd() descriptor instead.  We
+ * don't need to register a signal handler or create our own self-pipe.  We
+ * assume that any system that has Linux epoll() also has Linux signalfd().
  *
  * The Windows implementation uses Windows events that are inherited by all
  * postmaster child processes. There's no need for the self-pipe trick there.
@@ -46,6 +45,7 @@
 #include <poll.h>
 #endif
 
+#include "libpq/pqsignal.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "port/atomics.h"
@@ -79,6 +79,10 @@
 #error "no wait set implementation available"
 #endif
 
+#ifdef WAIT_USE_EPOLL
+#include <sys/signalfd.h>
+#endif
+
 /* typedef in latch.h */
 struct WaitEventSet
 {
@@ -139,7 +143,14 @@ static WaitEventSet *LatchWaitSet;
 #ifndef WIN32
 /* Are we currently in WaitLatch? The signal handler would like to know. */
 static volatile sig_atomic_t waiting = false;
+#endif
 
+#ifdef WAIT_USE_EPOLL
+/* On Linux, we'll receive SIGURG via a signalfd file descriptor. */
+static int	signal_fd = -1;
+#endif
+
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 /* Read and write ends of the self-pipe */
 static int	selfpipe_readfd = -1;
 static int	selfpipe_writefd = -1;
@@ -150,8 +161,11 @@ static int	selfpipe_owner_pid = 0;
 /* Private function prototypes */
 static void latch_sigurg_handler(SIGNAL_ARGS);
 static void sendSelfPipeByte(void);
-static void drainSelfPipe(void);
-#endif							/* WIN32 */
+#endif
+
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_EPOLL)
+static void drain(void);
+#endif
 
 #if defined(WAIT_USE_EPOLL)
 static void WaitEventAdjustEpoll(WaitEventSet *set, WaitEvent *event, int action);
@@ -175,7 +189,7 @@ static inline int WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 void
 InitializeLatchSupport(void)
 {
-#ifndef WIN32
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 	int			pipefd[2];
 
 	if (IsUnderPostmaster)
@@ -247,8 +261,23 @@ InitializeLatchSupport(void)
 	ReserveExternalFD();
 
 	pqsignal(SIGURG, latch_sigurg_handler);
-#else
-	/* currently, nothing to do here for Windows */
+#endif
+
+#ifdef WAIT_USE_EPOLL
+	{
+		sigset_t	signalfd_mask;
+
+		/* Block SIGUSG, because we'll receive it through a signalfd. */
+		sigaddset(&UnBlockSig, SIGURG);
+
+		/* Set up the signalfd to receive SIGURG notifications. */
+		sigemptyset(&signalfd_mask);
+		sigaddset(&signalfd_mask, SIGURG);
+		signal_fd = signalfd(-1, &signalfd_mask, SFD_NONBLOCK);
+		if (signal_fd < 0)
+			elog(FATAL, "signalfd() failed");
+		ReserveExternalFD();
+	}
 #endif
 }
 
@@ -273,7 +302,9 @@ InitializeLatchWaitSet(void)
 void
 ShutdownLatchSupport(void)
 {
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 	pqsignal(SIGURG, SIG_IGN);
+#endif
 
 	if (LatchWaitSet)
 	{
@@ -281,11 +312,18 @@ ShutdownLatchSupport(void)
 		LatchWaitSet = NULL;
 	}
 
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 	close(selfpipe_readfd);
 	close(selfpipe_writefd);
 	selfpipe_readfd = -1;
 	selfpipe_writefd = -1;
 	selfpipe_owner_pid = InvalidPid;
+#endif
+
+#if defined(WAIT_USE_EPOLL)
+	close(signal_fd);
+	signal_fd = -1;
+#endif
 }
 
 /*
@@ -299,10 +337,10 @@ InitLatch(Latch *latch)
 	latch->owner_pid = MyProcPid;
 	latch->is_shared = false;
 
-#ifndef WIN32
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 	/* Assert InitializeLatchSupport has been called in this process */
 	Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid);
-#else
+#elif defined(WAIT_USE_WIN32)
 	latch->event = CreateEvent(NULL, TRUE, FALSE, NULL);
 	if (latch->event == NULL)
 		elog(ERROR, "CreateEvent failed: error code %lu", GetLastError());
@@ -363,7 +401,7 @@ OwnLatch(Latch *latch)
 	/* Sanity checks */
 	Assert(latch->is_shared);
 
-#ifndef WIN32
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 	/* Assert InitializeLatchSupport has been called in this process */
 	Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid);
 #endif
@@ -550,9 +588,9 @@ SetLatch(Latch *latch)
 
 	/*
 	 * See if anyone's waiting for the latch. It can be the current process if
-	 * we're in a signal handler. We use the self-pipe to wake up the
-	 * poll()/epoll_wait() in that case. If it's another process, send a
-	 * signal.
+	 * we're in a signal handler. We use the self-pipe or SIGURG to ourselves
+	 * to wake up WaitEventSetWaitBlock() without races in that case. If it's
+	 * another process, send a signal.
 	 *
 	 * Fetch owner_pid only once, in case the latch is concurrently getting
 	 * owned or disowned. XXX: This assumes that pid_t is atomic, which isn't
@@ -575,11 +613,17 @@ SetLatch(Latch *latch)
 		return;
 	else if (owner_pid == MyProcPid)
 	{
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 		if (waiting)
 			sendSelfPipeByte();
+#else
+		if (waiting)
+			raise(SIGURG);
+#endif
 	}
 	else
 		kill(owner_pid, SIGURG);
+
 #else
 
 	/*
@@ -875,8 +919,13 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch,
 	{
 		set->latch = latch;
 		set->latch_pos = event->pos;
-#ifndef WIN32
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
 		event->fd = selfpipe_readfd;
+#elif defined(WAIT_USE_EPOLL)
+		event->fd = signal_fd;
+#else
+		event->fd = PGINVALID_SOCKET;
+		return event->pos;
 #endif
 	}
 	else if (events == WL_POSTMASTER_DEATH)
@@ -951,12 +1000,13 @@ ModifyWaitEvent(WaitEventSet *set, int pos, uint32 events, Latch *latch)
 		if (latch && latch->owner_pid != MyProcPid)
 			elog(ERROR, "cannot wait on a latch owned by another process");
 		set->latch = latch;
+
 		/*
 		 * On Unix, we don't need to modify the kernel object because the
-		 * underlying pipe is the same for all latches so we can return
-		 * immediately.  On Windows, we need to update our array of handles,
-		 * but we leave the old one in place and tolerate spurious wakeups if
-		 * the latch is disabled.
+		 * underlying pipe (if there is one) is the same for all latches so we
+		 * can return immediately.  On Windows, we need to update our array of
+		 * handles, but we leave the old one in place and tolerate spurious
+		 * wakeups if the latch is disabled.
 		 */
 #if defined(WAIT_USE_WIN32)
 		if (!latch)
@@ -1440,8 +1490,8 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 		if (cur_event->events == WL_LATCH_SET &&
 			cur_epoll_event->events & (EPOLLIN | EPOLLERR | EPOLLHUP))
 		{
-			/* There's data in the self-pipe, clear it. */
-			drainSelfPipe();
+			/* Drain the signalfd. */
+			drain();
 
 			if (set->latch && set->latch->is_set)
 			{
@@ -1594,7 +1644,7 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 			cur_kqueue_event->filter == EVFILT_READ)
 		{
 			/* There's data in the self-pipe, clear it. */
-			drainSelfPipe();
+			drain();
 
 			if (set->latch && set->latch->is_set)
 			{
@@ -1710,7 +1760,7 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 			(cur_pollfd->revents & (POLLIN | POLLHUP | POLLERR | POLLNVAL)))
 		{
 			/* There's data in the self-pipe, clear it. */
-			drainSelfPipe();
+			drain();
 
 			if (set->latch && set->latch->is_set)
 			{
@@ -1970,7 +2020,8 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 }
 #endif
 
-#ifndef WIN32
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+
 /*
  * SetLatch uses SIGURG to wake up the process waiting on the latch.
  *
@@ -1986,10 +2037,8 @@ latch_sigurg_handler(SIGNAL_ARGS)
 
 	errno = save_errno;
 }
-#endif							/* !WIN32 */
 
 /* Send one byte to the self-pipe, to wake up WaitLatch */
-#ifndef WIN32
 static void
 sendSelfPipeByte(void)
 {
@@ -2019,45 +2068,58 @@ retry:
 		return;
 	}
 }
-#endif							/* !WIN32 */
+
+#endif
+
+#if defined(WAIT_USE_POLL) || defined(WAIT_USE_EPOLL)
 
 /*
- * Read all available data from the self-pipe
+ * Read all available data from self-pipe or signalfd.
  *
  * Note: this is only called when waiting = true.  If it fails and doesn't
  * return, it must reset that flag first (though ideally, this will never
  * happen).
  */
-#ifndef WIN32
 static void
-drainSelfPipe(void)
+drain(void)
 {
-	/*
-	 * There shouldn't normally be more than one byte in the pipe, or maybe a
-	 * few bytes if multiple processes run SetLatch at the same instant.
-	 */
-	char		buf[16];
+	char		buf[1024];
 	int			rc;
+	int			fd;
+
+#ifdef WAIT_USE_POLL
+	fd = selfpipe_readfd;
+#else
+	fd = signal_fd;
+#endif
 
 	for (;;)
 	{
-		rc = read(selfpipe_readfd, buf, sizeof(buf));
+		rc = read(fd, buf, sizeof(buf));
 		if (rc < 0)
 		{
 			if (errno == EAGAIN || errno == EWOULDBLOCK)
-				break;			/* the pipe is empty */
+				break;			/* the descriptor is empty */
 			else if (errno == EINTR)
 				continue;		/* retry */
 			else
 			{
 				waiting = false;
+#ifdef WAIT_USE_POLL
 				elog(ERROR, "read() on self-pipe failed: %m");
+#else
+				elog(ERROR, "read() on signalfd failed: %m");
+#endif
 			}
 		}
 		else if (rc == 0)
 		{
 			waiting = false;
+#ifdef WAIT_USE_POLL
 			elog(ERROR, "unexpected EOF on self-pipe");
+#else
+			elog(ERROR, "unexpected EOF on signalfd");
+#endif
 		}
 		else if (rc < sizeof(buf))
 		{
@@ -2067,4 +2129,5 @@ drainSelfPipe(void)
 		/* else buffer wasn't big enough, so read again */
 	}
 }
-#endif							/* !WIN32 */
+
+#endif
-- 
2.20.1

From d60d463cf4f386ec4c5687fa0b458383bb6e6387 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Thu, 19 Nov 2020 15:35:04 +1300
Subject: [PATCH v4 4/5] Use EVFILT_SIGNAL for kqueue latches.

This leaves only the poll implementation with a signal handler and the
traditional self-pipe trick.

Discussion: https://postgr.es/m/ca+hukgjjxpdpzbe0a3hyuywbvazuc89yx3jk9rfzgfv_khu...@mail.gmail.com
---
 src/backend/storage/ipc/latch.c | 58 +++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 6d7bc8e328..015f94c49c 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -18,6 +18,8 @@
  * don't need to register a signal handler or create our own self-pipe.  We
  * assume that any system that has Linux epoll() also has Linux signalfd().
  *
+ * The kqueue() implementation waits for SIGURG with EVFILT_SIGNAL.
+ *
  * The Windows implementation uses Windows events that are inherited by all
  * postmaster child processes. There's no need for the self-pipe trick there.
  *
@@ -150,7 +152,7 @@ static volatile sig_atomic_t waiting = false;
 static int	signal_fd = -1;
 #endif
 
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 /* Read and write ends of the self-pipe */
 static int	selfpipe_readfd = -1;
 static int	selfpipe_writefd = -1;
@@ -189,7 +191,7 @@ static inline int WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 void
 InitializeLatchSupport(void)
 {
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 	int			pipefd[2];
 
 	if (IsUnderPostmaster)
@@ -279,6 +281,11 @@ InitializeLatchSupport(void)
 		ReserveExternalFD();
 	}
 #endif
+
+#ifdef WAIT_USE_KQUEUE
+	/* Ignore SIGURG, because we'll receive it via kqueue. */
+	pqsignal(SIGURG, SIG_IGN);
+#endif
 }
 
 void
@@ -302,7 +309,7 @@ InitializeLatchWaitSet(void)
 void
 ShutdownLatchSupport(void)
 {
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 	pqsignal(SIGURG, SIG_IGN);
 #endif
 
@@ -312,7 +319,7 @@ ShutdownLatchSupport(void)
 		LatchWaitSet = NULL;
 	}
 
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 	close(selfpipe_readfd);
 	close(selfpipe_writefd);
 	selfpipe_readfd = -1;
@@ -337,7 +344,7 @@ InitLatch(Latch *latch)
 	latch->owner_pid = MyProcPid;
 	latch->is_shared = false;
 
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 	/* Assert InitializeLatchSupport has been called in this process */
 	Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid);
 #elif defined(WAIT_USE_WIN32)
@@ -401,7 +408,7 @@ OwnLatch(Latch *latch)
 	/* Sanity checks */
 	Assert(latch->is_shared);
 
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 	/* Assert InitializeLatchSupport has been called in this process */
 	Assert(selfpipe_readfd >= 0 && selfpipe_owner_pid == MyProcPid);
 #endif
@@ -613,7 +620,7 @@ SetLatch(Latch *latch)
 		return;
 	else if (owner_pid == MyProcPid)
 	{
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 		if (waiting)
 			sendSelfPipeByte();
 #else
@@ -919,13 +926,15 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch,
 	{
 		set->latch = latch;
 		set->latch_pos = event->pos;
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 		event->fd = selfpipe_readfd;
 #elif defined(WAIT_USE_EPOLL)
 		event->fd = signal_fd;
 #else
 		event->fd = PGINVALID_SOCKET;
+#ifdef WAIT_USE_EPOLL
 		return event->pos;
+#endif
 #endif
 	}
 	else if (events == WL_POSTMASTER_DEATH)
@@ -1146,6 +1155,18 @@ WaitEventAdjustKqueueAddPostmaster(struct kevent *k_ev, WaitEvent *event)
 	AccessWaitEvent(k_ev) = event;
 }
 
+static inline void
+WaitEventAdjustKqueueAddLatch(struct kevent *k_ev, WaitEvent *event)
+{
+	/* For now latch can only be added, not removed. */
+	k_ev->ident = SIGURG;
+	k_ev->filter = EVFILT_SIGNAL;
+	k_ev->flags = EV_ADD;
+	k_ev->fflags = 0;
+	k_ev->data = 0;
+	AccessWaitEvent(k_ev) = event;
+}
+
 /*
  * old_events is the previous event mask, used to compute what has changed.
  */
@@ -1177,6 +1198,14 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events)
 		 */
 		WaitEventAdjustKqueueAddPostmaster(&k_ev[count++], event);
 	}
+	else if (event->events == WL_LATCH_SET)
+	{
+		/*
+		 * Unlike all the other implementations, we detect latch wakeup using
+		 * signal events, rather than installing a signal handler.
+		 */
+		 WaitEventAdjustKqueueAddLatch(&k_ev[count++], event);
+	}
 	else
 	{
 		/*
@@ -1184,11 +1213,9 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events)
 		 * old event mask to the new event mask, since kevent treats readable
 		 * and writable as separate events.
 		 */
-		if (old_events == WL_LATCH_SET ||
-			(old_events & WL_SOCKET_READABLE))
+		if (old_events & WL_SOCKET_READABLE)
 			old_filt_read = true;
-		if (event->events == WL_LATCH_SET ||
-			(event->events & WL_SOCKET_READABLE))
+		if (event->events & WL_SOCKET_READABLE)
 			new_filt_read = true;
 		if (old_events & WL_SOCKET_WRITEABLE)
 			old_filt_write = true;
@@ -1641,11 +1668,8 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 		occurred_events->events = 0;
 
 		if (cur_event->events == WL_LATCH_SET &&
-			cur_kqueue_event->filter == EVFILT_READ)
+			cur_kqueue_event->filter == EVFILT_SIGNAL)
 		{
-			/* There's data in the self-pipe, clear it. */
-			drain();
-
 			if (set->latch && set->latch->is_set)
 			{
 				occurred_events->fd = PGINVALID_SOCKET;
@@ -2020,7 +2044,7 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 }
 #endif
 
-#if defined(WAIT_USE_POLL) || defined(WAIT_USE_KQUEUE)
+#if defined(WAIT_USE_POLL)
 
 /*
  * SetLatch uses SIGURG to wake up the process waiting on the latch.
-- 
2.20.1

Reply via email to