From 125deab4030d7cf8918df19fb67fc4c8bee27570 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sat, 1 Apr 2023 12:36:12 +1300
Subject: [PATCH v2] Teach WaitEventSetWait to report multiple events on
 Windows.

The WaitEventSet implementation on Windows has always reported only one
event at a time, and always the "lowest" in its event array.  Since
commit 7389aad6 started using WaitEventSet to handle incoming socket
connections, this unfairness changes the accept priority when using
multiple server sockets.  If one of them has a non-empty listen queue
due to incoming connections, the other might never be serviced.  The
previously coding based on select() was fair in that way.

Fix, by teaching WaitEventSetWait() to poll for extra events.  No change
in behavior in the common case of callers with nevents=1 (for example
the main FEBE socket code), but for the postmaster's main loop we'll
drain all the events that can fit in the output buffer, which is
deliberately set large enough to handle the maximum possible number of
sockets.  This brings the Windows behavior in line with Unix.

Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CA%2BhUKG%2BA2dk29hr5zRP3HVJQ-_PncNJM6HVQ7aaYLXLRBZU-xw%40mail.gmail.com

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index f4123e7de7..334236aefc 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -1933,14 +1933,10 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 #elif defined(WAIT_USE_WIN32)
 
 /*
- * Wait using Windows' WaitForMultipleObjects().
+ * Wait using Windows' WaitForMultipleObjects().  Each call only "consumes" one
+ * event, so we keep calling until we've filled up our output buffer.
  *
- * Unfortunately this will only ever return a single readiness notification at
- * a time.  Note that while the official documentation for
- * WaitForMultipleObjects is ambiguous about multiple events being "consumed"
- * with a single bWaitAll = FALSE call,
- * https://blogs.msdn.microsoft.com/oldnewthing/20150409-00/?p=44273 confirms
- * that only one event is "consumed".
+ * https://blogs.msdn.microsoft.com/oldnewthing/20150409-00/?p=44273
  */
 static inline int
 WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
@@ -2025,106 +2021,145 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 	 */
 	cur_event = (WaitEvent *) &set->events[rc - WAIT_OBJECT_0 - 1];
 
-	occurred_events->pos = cur_event->pos;
-	occurred_events->user_data = cur_event->user_data;
-	occurred_events->events = 0;
-
-	if (cur_event->events == WL_LATCH_SET)
-	{
-		/*
-		 * We cannot use set->latch->event to reset the fired event if we
-		 * aren't waiting on this latch now.
-		 */
-		if (!ResetEvent(set->handles[cur_event->pos + 1]))
-			elog(ERROR, "ResetEvent failed: error code %lu", GetLastError());
-
-		if (set->latch && set->latch->is_set)
-		{
-			occurred_events->fd = PGINVALID_SOCKET;
-			occurred_events->events = WL_LATCH_SET;
-			occurred_events++;
-			returned_events++;
-		}
-	}
-	else if (cur_event->events == WL_POSTMASTER_DEATH)
-	{
-		/*
-		 * Postmaster apparently died.  Since the consequences of falsely
-		 * returning WL_POSTMASTER_DEATH could be pretty unpleasant, we take
-		 * the trouble to positively verify this with PostmasterIsAlive(),
-		 * even though there is no known reason to think that the event could
-		 * be falsely set on Windows.
-		 */
-		if (!PostmasterIsAliveInternal())
-		{
-			if (set->exit_on_postmaster_death)
-				proc_exit(1);
-			occurred_events->fd = PGINVALID_SOCKET;
-			occurred_events->events = WL_POSTMASTER_DEATH;
-			occurred_events++;
-			returned_events++;
-		}
-	}
-	else if (cur_event->events & WL_SOCKET_MASK)
+	for (;;)
 	{
-		WSANETWORKEVENTS resEvents;
-		HANDLE		handle = set->handles[cur_event->pos + 1];
-
-		Assert(cur_event->fd);
+		int			next_pos;
+		int			count;
 
-		occurred_events->fd = cur_event->fd;
+		occurred_events->pos = cur_event->pos;
+		occurred_events->user_data = cur_event->user_data;
+		occurred_events->events = 0;
 
-		ZeroMemory(&resEvents, sizeof(resEvents));
-		if (WSAEnumNetworkEvents(cur_event->fd, handle, &resEvents) != 0)
-			elog(ERROR, "failed to enumerate network events: error code %d",
-				 WSAGetLastError());
-		if ((cur_event->events & WL_SOCKET_READABLE) &&
-			(resEvents.lNetworkEvents & FD_READ))
+		if (cur_event->events == WL_LATCH_SET)
 		{
-			/* data available in socket */
-			occurred_events->events |= WL_SOCKET_READABLE;
-
-			/*------
-			 * WaitForMultipleObjects doesn't guarantee that a read event will
-			 * be returned if the latch is set at the same time.  Even if it
-			 * did, the caller might drop that event expecting it to reoccur
-			 * on next call.  So, we must force the event to be reset if this
-			 * WaitEventSet is used again in order to avoid an indefinite
-			 * hang.  Refer https://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx
-			 * for the behavior of socket events.
-			 *------
+			/*
+			 * We cannot use set->latch->event to reset the fired event if we
+			 * aren't waiting on this latch now.
 			 */
-			cur_event->reset = true;
-		}
-		if ((cur_event->events & WL_SOCKET_WRITEABLE) &&
-			(resEvents.lNetworkEvents & FD_WRITE))
-		{
-			/* writeable */
-			occurred_events->events |= WL_SOCKET_WRITEABLE;
-		}
-		if ((cur_event->events & WL_SOCKET_CONNECTED) &&
-			(resEvents.lNetworkEvents & FD_CONNECT))
-		{
-			/* connected */
-			occurred_events->events |= WL_SOCKET_CONNECTED;
+			if (!ResetEvent(set->handles[cur_event->pos + 1]))
+				elog(ERROR, "ResetEvent failed: error code %lu", GetLastError());
+
+			if (set->latch && set->latch->is_set)
+			{
+				occurred_events->fd = PGINVALID_SOCKET;
+				occurred_events->events = WL_LATCH_SET;
+				occurred_events++;
+				returned_events++;
+			}
 		}
-		if ((cur_event->events & WL_SOCKET_ACCEPT) &&
-			(resEvents.lNetworkEvents & FD_ACCEPT))
+		else if (cur_event->events == WL_POSTMASTER_DEATH)
 		{
-			/* incoming connection could be accepted */
-			occurred_events->events |= WL_SOCKET_ACCEPT;
+			/*
+			 * Postmaster apparently died.  Since the consequences of falsely
+			 * returning WL_POSTMASTER_DEATH could be pretty unpleasant, we
+			 * take the trouble to positively verify this with
+			 * PostmasterIsAlive(), even though there is no known reason to
+			 * think that the event could be falsely set on Windows.
+			 */
+			if (!PostmasterIsAliveInternal())
+			{
+				if (set->exit_on_postmaster_death)
+					proc_exit(1);
+				occurred_events->fd = PGINVALID_SOCKET;
+				occurred_events->events = WL_POSTMASTER_DEATH;
+				occurred_events++;
+				returned_events++;
+			}
 		}
-		if (resEvents.lNetworkEvents & FD_CLOSE)
+		else if (cur_event->events & WL_SOCKET_MASK)
 		{
-			/* EOF/error, so signal all caller-requested socket flags */
-			occurred_events->events |= (cur_event->events & WL_SOCKET_MASK);
-		}
+			WSANETWORKEVENTS resEvents;
+			HANDLE		handle = set->handles[cur_event->pos + 1];
 
-		if (occurred_events->events != 0)
-		{
-			occurred_events++;
-			returned_events++;
+			Assert(cur_event->fd);
+
+			occurred_events->fd = cur_event->fd;
+
+			ZeroMemory(&resEvents, sizeof(resEvents));
+			if (WSAEnumNetworkEvents(cur_event->fd, handle, &resEvents) != 0)
+				elog(ERROR, "failed to enumerate network events: error code %d",
+					 WSAGetLastError());
+			if ((cur_event->events & WL_SOCKET_READABLE) &&
+				(resEvents.lNetworkEvents & FD_READ))
+			{
+				/* data available in socket */
+				occurred_events->events |= WL_SOCKET_READABLE;
+
+				/*------
+				 * WaitForMultipleObjects doesn't guarantee that a read event
+				 * will be returned if the latch is set at the same time.  Even
+				 * if it did, the caller might drop that event expecting it to
+				 * reoccur on next call.  So, we must force the event to be
+				 * reset if this WaitEventSet is used again in order to avoid
+				 * an indefinite hang.
+				 *
+				 * Refer
+				 * https://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx
+				 * for the behavior of socket events.
+				 *------
+				 */
+				cur_event->reset = true;
+			}
+			if ((cur_event->events & WL_SOCKET_WRITEABLE) &&
+				(resEvents.lNetworkEvents & FD_WRITE))
+			{
+				/* writeable */
+				occurred_events->events |= WL_SOCKET_WRITEABLE;
+			}
+			if ((cur_event->events & WL_SOCKET_CONNECTED) &&
+				(resEvents.lNetworkEvents & FD_CONNECT))
+			{
+				/* connected */
+				occurred_events->events |= WL_SOCKET_CONNECTED;
+			}
+			if ((cur_event->events & WL_SOCKET_ACCEPT) &&
+				(resEvents.lNetworkEvents & FD_ACCEPT))
+			{
+				/* incoming connection could be accepted */
+				occurred_events->events |= WL_SOCKET_ACCEPT;
+			}
+			if (resEvents.lNetworkEvents & FD_CLOSE)
+			{
+				/* EOF/error, so signal all caller-requested socket flags */
+				occurred_events->events |= (cur_event->events & WL_SOCKET_MASK);
+			}
+
+			if (occurred_events->events != 0)
+			{
+				occurred_events++;
+				returned_events++;
+			}
 		}
+
+		/* Is the output buffer full? */
+		if (returned_events == nevents)
+			break;
+
+		/* Have we run out of possible events? */
+		next_pos = cur_event->pos + 1;
+		if (next_pos == set->nevents)
+			break;
+
+		/*
+		 * Poll the rest of the event handles in the array starting at
+		 * next_pos being careful to skip over the initial signal handle too.
+		 * This time we use a zero timeout.
+		 */
+		count = set->nevents - next_pos;
+		rc = WaitForMultipleObjects(count,
+									set->handles + 1 + next_pos,
+									false,
+									0);
+
+		/*
+		 * We don't distinguish between errors and WAIT_TIMEOUT here because
+		 * we already have events to report.
+		 */
+		if (rc < WAIT_OBJECT_0 || rc >= WAIT_OBJECT_0 + count)
+			break;
+
+		/* We have another event to decode. */
+		cur_event = &set->events[next_pos + (rc - WAIT_OBJECT_0)];
 	}
 
 	return returned_events;
-- 
2.40.0

