Fix incorrect order of lock file removal and failure to close() sockets.

Commit c9b0cbe98bd783e24a8c4d8d8ac472a494b81292 accidentally broke the
order of operations during postmaster shutdown: it resulted in removing
the per-socket lockfiles after, not before, postmaster.pid.  This creates
a race-condition hazard for a new postmaster that's started immediately
after observing that postmaster.pid has disappeared; if it sees the
socket lockfile still present, it will quite properly refuse to start.
This error appears to be the explanation for at least some of the
intermittent buildfarm failures we've seen in the pg_upgrade test.

Another problem, which has been there all along, is that the postmaster
has never bothered to close() its listen sockets, but has just allowed them
to close at process death.  This creates a different race condition for an
incoming postmaster: it might be unable to bind to the desired listen
address because the old postmaster is still incumbent.  This might explain
some odd failures we've seen in the past, too.  (Note: this is not related
to the fact that individual backends don't close their client communication
sockets.  That behavior is intentional and is not changed by this patch.)

Fix by adding an on_proc_exit function that closes the postmaster's ports
explicitly, and (in 9.3 and up) reshuffling the responsibility for where
to unlink the Unix socket files.  Lock file unlinking can stay where it
is, but teach it to unlink the lock files in reverse order of creation.

1 files changed, 25 insertions, 29 deletions

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index b27ac952173..63673b16dfb 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -174,12 +174,15 @@ PQcommMethods *PqCommMethods = &PqCommSocketMethods;
 void
 pq_init(void)
 {
+	/* initialize state variables */
 	PqSendBufferSize = PQ_SEND_BUFFER_SIZE;
 	PqSendBuffer = MemoryContextAlloc(TopMemoryContext, PqSendBufferSize);
 	PqSendPointer = PqSendStart = PqRecvPointer = PqRecvLength = 0;
 	PqCommBusy = false;
 	PqCommReadingMsg = false;
 	DoingCopyOut = false;
+
+	/* set up process-exit hook to close the socket */
 	on_proc_exit(socket_close, 0);
 
 	/*
@@ -285,28 +288,6 @@ socket_close(int code, Datum arg)
  */
 
 
-/* StreamDoUnlink()
- * Shutdown routine for backend connection
- * If any Unix sockets are used for communication, explicitly close them.
- */
-#ifdef HAVE_UNIX_SOCKETS
-static void
-StreamDoUnlink(int code, Datum arg)
-{
-	ListCell   *l;
-
-	/* Loop through all created sockets... */
-	foreach(l, sock_paths)
-	{
-		char	   *sock_path = (char *) lfirst(l);
-
-		unlink(sock_path);
-	}
-	/* Since we're about to exit, no need to reclaim storage */
-	sock_paths = NIL;
-}
-#endif   /* HAVE_UNIX_SOCKETS */
-
 /*
  * StreamServerPort -- open a "listening" port to accept connections.
  *
@@ -588,16 +569,11 @@ Lock_AF_UNIX(char *unixSocketDir, char *unixSocketPath)
 	 * Once we have the interlock, we can safely delete any pre-existing
 	 * socket file to avoid failure at bind() time.
 	 */
-	unlink(unixSocketPath);
+	(void) unlink(unixSocketPath);
 
 	/*
-	 * Arrange to unlink the socket file(s) at proc_exit.  If this is the
-	 * first one, set up the on_proc_exit function to do it; then add this
-	 * socket file to the list of files to unlink.
+	 * Remember socket file pathnames for later maintenance.
 	 */
-	if (sock_paths == NIL)
-		on_proc_exit(StreamDoUnlink, 0);
-
 	sock_paths = lappend(sock_paths, pstrdup(unixSocketPath));
 
 	return STATUS_OK;
@@ -858,6 +834,26 @@ TouchSocketFiles(void)
 	}
 }
 
+/*
+ * RemoveSocketFiles -- unlink socket files at postmaster shutdown
+ */
+void
+RemoveSocketFiles(void)
+{
+	ListCell   *l;
+
+	/* Loop through all created sockets... */
+	foreach(l, sock_paths)
+	{
+		char	   *sock_path = (char *) lfirst(l);
+
+		/* Ignore any error. */
+		(void) unlink(sock_path);
+	}
+	/* Since we're about to exit, no need to reclaim storage */
+	sock_paths = NIL;
+}
+
 
 /* --------------------------------
  * Low-level I/O routines begin here.