From d73d14c271653dff10c349738df79ea03b85236c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 2 Aug 2015 14:54:44 -0400 Subject: [PATCH] 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. --- src/backend/libpq/pqcomm.c | 54 +++++++++++++---------------- src/backend/postmaster/postmaster.c | 47 +++++++++++++++++++++++++ src/backend/utils/init/miscinit.c | 6 +++- src/include/libpq/libpq.h | 1 + 4 files changed, 78 insertions(+), 30 deletions(-) diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index b27ac95217..63673b16df 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. diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 1bb3138a03..000524dcb9 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -370,6 +370,7 @@ static DNSServiceRef bonjour_sdref = NULL; /* * postmaster.c - function prototypes */ +static void CloseServerPorts(int status, Datum arg); static void unlink_external_pid_file(int status, Datum arg); static void getInstallationPaths(const char *argv0); static void checkDataDir(void); @@ -900,6 +901,11 @@ PostmasterMain(int argc, char *argv[]) * interlock (thanks to whoever decided to put socket files in /tmp :-(). * For the same reason, it's best to grab the TCP socket(s) before the * Unix socket(s). + * + * Also note that this internally sets up the on_proc_exit function that + * is responsible for removing both data directory and socket lockfiles; + * so it must happen before opening sockets so that at exit, the socket + * lockfiles go away after CloseServerPorts runs. */ CreateDataDirLockFile(true); @@ -924,10 +930,15 @@ PostmasterMain(int argc, char *argv[]) /* * Establish input sockets. + * + * First, mark them all closed, and set up an on_proc_exit function that's + * charged with closing the sockets again at postmaster shutdown. */ for (i = 0; i < MAXLISTEN; i++) ListenSocket[i] = PGINVALID_SOCKET; + on_proc_exit(CloseServerPorts, 0); + if (ListenAddresses) { char *rawstring; @@ -1271,6 +1282,42 @@ PostmasterMain(int argc, char *argv[]) } +/* + * on_proc_exit callback to close server's listen sockets + */ +static void +CloseServerPorts(int status, Datum arg) +{ + int i; + + /* + * First, explicitly close all the socket FDs. We used to just let this + * happen implicitly at postmaster exit, but it's better to close them + * before we remove the postmaster.pid lockfile; otherwise there's a race + * condition if a new postmaster wants to re-use the TCP port number. + */ + for (i = 0; i < MAXLISTEN; i++) + { + if (ListenSocket[i] != PGINVALID_SOCKET) + { + StreamClose(ListenSocket[i]); + ListenSocket[i] = PGINVALID_SOCKET; + } + } + + /* + * Next, remove any filesystem entries for Unix sockets. To avoid race + * conditions against incoming postmasters, this must happen after closing + * the sockets and before removing lock files. + */ + RemoveSocketFiles(); + + /* + * We don't do anything about socket lock files here; those will be + * removed in a later on_proc_exit callback. + */ +} + /* * on_proc_exit callback to delete external_pid_file */ diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index ac3e764e8b..5bf595c9e5 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -1018,7 +1018,11 @@ CreateLockFile(const char *filename, bool amPostmaster, if (lock_files == NIL) on_proc_exit(UnlinkLockFiles, 0); - lock_files = lappend(lock_files, pstrdup(filename)); + /* + * Use lcons so that the lock files are unlinked in reverse order of + * creation; this is critical! + */ + lock_files = lcons(pstrdup(filename), lock_files); } /* diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h index c408e5b551..efb2dacbba 100644 --- a/src/include/libpq/libpq.h +++ b/src/include/libpq/libpq.h @@ -59,6 +59,7 @@ extern int StreamServerPort(int family, char *hostName, extern int StreamConnection(pgsocket server_fd, Port *port); extern void StreamClose(pgsocket sock); extern void TouchSocketFiles(void); +extern void RemoveSocketFiles(void); extern void pq_init(void); extern int pq_getbytes(char *s, size_t len); extern int pq_getstring(StringInfo s);