Documentation improvement and minor code cleanups for the latch facility.

Improve the documentation around weak-memory-ordering risks, and do a pass
of general editorialization on the comments in the latch code.  Make the
Windows latch code more like the Unix latch code where feasible; in
particular provide the same Assert checks in both implementations.
Fix poorly-placed WaitLatch call in syncrep.c.

This patch resolves, for the moment, concerns around weak-memory-ordering
bugs in latch-related code: we have documented the restrictions and checked
that existing calls meet them.  In 9.2 I hope that we will install suitable
memory barrier instructions in SetLatch/ResetLatch, so that their callers
don't need to be quite so careful.

src/backend/port/win32_latch.c

 /*-------------------------------------------------------------------------
  *
  * win32_latch.c
- *	  Windows implementation of latches.
+ *	  Routines for inter-process latches
  *
- * See unix_latch.c for information on usage.
+ * See unix_latch.c for header comments for the exported functions;
+ * the API presented here is supposed to be the same as there.
  *
  * The Windows implementation uses Windows events that are inherited by
  * all postmaster child processes.
@@ -24,7 +25,6 @@
 
 #include "miscadmin.h"
 #include "postmaster/postmaster.h"
-#include "replication/walsender.h"
 #include "storage/latch.h"
 #include "storage/shmem.h"
 
@@ -89,7 +89,7 @@ WaitLatch(volatile Latch *latch, int wakeEvents, long timeout)
 }
 
 int
-WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
+WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
 				  long timeout)
 {
 	DWORD		rc;
@@ -101,12 +101,15 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
 	int			pmdeath_eventno = 0;
 	long		timeout_ms;
 
-	Assert(wakeEvents != 0);
-
 	/* Ignore WL_SOCKET_* events if no valid socket is given */
 	if (sock == PGINVALID_SOCKET)
 		wakeEvents &= ~(WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE);
 
+	Assert(wakeEvents != 0);	/* must have at least one wake event */
+
+	if ((wakeEvents & WL_LATCH_SET) && latch->owner_pid != MyProcPid)
+		elog(ERROR, "cannot wait on a latch owned by another process");
+
 	/* Convert timeout to milliseconds for WaitForMultipleObjects() */
 	if (wakeEvents & WL_TIMEOUT)
 	{
@@ -122,8 +125,8 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
 	events[0] = latchevent;
 	events[1] = pgwin32_signal_event;
 	numevents = 2;
-	if (((wakeEvents & WL_SOCKET_READABLE) ||
-		 (wakeEvents & WL_SOCKET_WRITEABLE)))
+	if ((wakeEvents & WL_SOCKET_READABLE) ||
+		(wakeEvents & WL_SOCKET_WRITEABLE))
 	{
 		int			flags = 0;
 
@@ -152,7 +155,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
 		 */
 		if (!ResetEvent(latchevent))
 			elog(ERROR, "ResetEvent failed: error code %d", (int) GetLastError());
-		if (latch->is_set && (wakeEvents & WL_LATCH_SET))
+		if ((wakeEvents & WL_LATCH_SET) && latch->is_set)
 		{
 			result |= WL_LATCH_SET;
 			/*
@@ -171,17 +174,17 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
 		else if (rc == WAIT_OBJECT_0 + 1)
 			pgwin32_dispatch_queued_signals();
 
+		else if (rc == WAIT_TIMEOUT)
+		{
+			result |= WL_TIMEOUT;
+		}
 		else if ((wakeEvents & WL_POSTMASTER_DEATH) &&
 				 rc == WAIT_OBJECT_0 + pmdeath_eventno)
 		{
 			/* Postmaster died */
 			result |= WL_POSTMASTER_DEATH;
 		}
-		else if (rc == WAIT_TIMEOUT)
-		{
-			result |= WL_TIMEOUT;
-		}
-		else if ((wakeEvents & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE)) != 0 &&
+		else if ((wakeEvents & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE)) &&
 				 rc == WAIT_OBJECT_0 + 2)	/* socket is at event slot 2 */
 		{
 			WSANETWORKEVENTS resEvents;
@@ -206,7 +209,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
 		else if (rc != WAIT_OBJECT_0)
 			elog(ERROR, "unexpected return code from WaitForMultipleObjects(): %d", (int) rc);
 	}
-	while(result == 0);
+	while (result == 0);
 
 	/* Clean up the handle we created for the socket */
 	if (sockevent != WSA_INVALID_EVENT)
@@ -231,15 +234,10 @@ SetLatch(volatile Latch *latch)
 
 	/*
 	 * See if anyone's waiting for the latch. It can be the current process if
-	 * we're in a signal handler. Use a local variable here in case the latch
-	 * is just disowned between the test and the SetEvent call, and event
-	 * field set to NULL.
+	 * we're in a signal handler.
 	 *
-	 * Fetch handle field only once, in case the owner simultaneously disowns
-	 * the latch and clears handle. This assumes that HANDLE is atomic, which
-	 * isn't guaranteed to be true! In practice, it should be, and in the
-	 * worst case we end up calling SetEvent with a bogus handle, and SetEvent
-	 * will return an error with no harm done.
+	 * Use a local variable here just in case somebody changes the event field
+	 * concurrently (which really should not happen).
 	 */
 	handle = latch->event;
 	if (handle)
@@ -256,5 +254,8 @@ SetLatch(volatile Latch *latch)
 void
 ResetLatch(volatile Latch *latch)
 {
+	/* Only the owner should reset the latch */
+	Assert(latch->owner_pid == MyProcPid);
+
 	latch->is_set = false;
 }

src/backend/replication/syncrep.c

@@ -166,13 +166,6 @@ SyncRepWaitForLSN(XLogRecPtr XactCommitLSN)
 	{
 		int			syncRepState;
 
-		/*
-		 * Wait on latch for up to 60 seconds. This allows us to check for
-		 * postmaster death regularly while waiting. Note that timeout here
-		 * does not necessarily release from loop.
-		 */
-		WaitLatch(&MyProc->waitLatch, WL_LATCH_SET | WL_TIMEOUT, 60000000L);
-
 		/* Must reset the latch before testing state. */
 		ResetLatch(&MyProc->waitLatch);
 
@@ -184,6 +177,12 @@ SyncRepWaitForLSN(XLogRecPtr XactCommitLSN)
 		 * walsender changes the state to SYNC_REP_WAIT_COMPLETE, it will
 		 * never update it again, so we can't be seeing a stale value in that
 		 * case.
+		 *
+		 * Note: on machines with weak memory ordering, the acquisition of
+		 * the lock is essential to avoid race conditions: we cannot be sure
+		 * the sender's state update has reached main memory until we acquire
+		 * the lock.  We could get rid of this dance if SetLatch/ResetLatch
+		 * contained memory barriers.
 		 */
 		syncRepState = MyProc->syncRepState;
 		if (syncRepState == SYNC_REP_WAITING)
@@ -246,6 +245,13 @@ SyncRepWaitForLSN(XLogRecPtr XactCommitLSN)
 			SyncRepCancelWait();
 			break;
 		}
+
+		/*
+		 * Wait on latch for up to 60 seconds. This allows us to check for
+		 * cancel/die signal or postmaster death regularly while waiting. Note
+		 * that timeout here does not necessarily release from loop.
+		 */
+		WaitLatch(&MyProc->waitLatch, WL_LATCH_SET | WL_TIMEOUT, 60000000L);
 	}
 
 	/*

src/backend/storage/lmgr/proc.c

@@ -332,7 +332,7 @@ InitProcess(void)
 	MyProc->waitLSN.xrecoff = 0;
 	MyProc->syncRepState = SYNC_REP_NOT_WAITING;
 	SHMQueueElemInit(&(MyProc->syncRepLinks));
-	OwnLatch((Latch *) &MyProc->waitLatch);
+	OwnLatch(&MyProc->waitLatch);
 
 	/*
 	 * We might be reusing a semaphore that belonged to a failed process. So

src/include/storage/latch.h

@@ -3,6 +3,67 @@
  * latch.h
  *	  Routines for interprocess latches
  *
+ * A latch is a boolean variable, with operations that let processes sleep
+ * until it is set. A latch can be set from another process, or a signal
+ * handler within the same process.
+ *
+ * The latch interface is a reliable replacement for the common pattern of
+ * using pg_usleep() or select() to wait until a signal arrives, where the
+ * signal handler sets a flag variable. Because on some platforms an
+ * incoming signal doesn't interrupt sleep, and even on platforms where it
+ * does there is a race condition if the signal arrives just before
+ * entering the sleep, the common pattern must periodically wake up and
+ * poll the flag variable. The pselect() system call was invented to solve
+ * this problem, but it is not portable enough. Latches are designed to
+ * overcome these limitations, allowing you to sleep without polling and
+ * ensuring quick response to signals from other processes.
+ *
+ * There are two kinds of latches: local and shared. A local latch is
+ * initialized by InitLatch, and can only be set from the same process.
+ * A local latch can be used to wait for a signal to arrive, by calling
+ * SetLatch in the signal handler. A shared latch resides in shared memory,
+ * and must be initialized at postmaster startup by InitSharedLatch. Before
+ * a shared latch can be waited on, it must be associated with a process
+ * with OwnLatch. Only the process owning the latch can wait on it, but any
+ * process can set it.
+ *
+ * There are three basic operations on a latch:
+ *
+ * SetLatch		- Sets the latch
+ * ResetLatch	- Clears the latch, allowing it to be set again
+ * WaitLatch	- Waits for the latch to become set
+ *
+ * WaitLatch includes a provision for timeouts (which should hopefully not
+ * be necessary once the code is fully latch-ified) and a provision for
+ * postmaster child processes to wake up immediately on postmaster death.
+ * See unix_latch.c for detailed specifications for the exported functions.
+ *
+ * The correct pattern to wait for event(s) is:
+ *
+ * for (;;)
+ * {
+ *	   ResetLatch();
+ *	   if (work to do)
+ *		   Do Stuff();
+ *	   WaitLatch();
+ * }
+ *
+ * It's important to reset the latch *before* checking if there's work to
+ * do. Otherwise, if someone sets the latch between the check and the
+ * ResetLatch call, you will miss it and Wait will incorrectly block.
+ *
+ * To wake up the waiter, you must first set a global flag or something
+ * else that the wait loop tests in the "if (work to do)" part, and call
+ * SetLatch *after* that. SetLatch is designed to return quickly if the
+ * latch is already set.
+ *
+ * Presently, when using a shared latch for interprocess signalling, the
+ * flag variable(s) set by senders and inspected by the wait loop must
+ * be protected by spinlocks or LWLocks, else it is possible to miss events
+ * on machines with weak memory ordering (such as PPC).  This restriction
+ * will be lifted in future by inserting suitable memory barriers into
+ * SetLatch and ResetLatch.
+ *
  *
  * Portions Copyright (c) 1996-2011, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
@@ -51,16 +112,17 @@ extern int WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
 extern void SetLatch(volatile Latch *latch);
 extern void ResetLatch(volatile Latch *latch);
 
-#define TestLatch(latch) (((volatile Latch *) latch)->is_set)
+/* beware of memory ordering issues if you use this macro! */
+#define TestLatch(latch) (((volatile Latch *) (latch))->is_set)
 
 /*
- * Unix implementation uses SIGUSR1 for inter-process signaling, Win32 doesn't
- * need this.
+ * 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()
+#define latch_sigusr1_handler()  ((void) 0)
 #endif
 
 #endif   /* LATCH_H */