On Windows, ensure shared memory handle gets closed if not being used.

tglsfdc · tglsfdc · commit bc6b03bb8106 · 2015-10-13T11:21:33.000-04:00
Postmaster child processes that aren't supposed to be attached to shared
memory were not bothering to close the shared memory mapping handle they
inherit from the postmaster process.  That's mostly harmless, since the
handle vanishes anyway when the child process exits -- but the syslogger
process, if used, doesn't get killed and restarted during recovery from a
backend crash.  That meant that Windows doesn't see the shared memory
mapping as becoming free, so it doesn't delete it and the postmaster is
unable to create a new one, resulting in failure to recover from crashes
whenever logging_collector is turned on.

Per report from Dmitry Vasilyev.  It's a bit astonishing that we'd not
figured this out long ago, since it's been broken from the very beginnings
of out native Windows support; probably some previously-unexplained trouble
reports trace to this.

A secondary problem is that on Cygwin (perhaps only in older versions?),
exec() may not detach from the shared memory segment after all, in which
case these child processes did remain attached to shared memory, posing
the risk of an unexpected shared memory clobber if they went off the rails
somehow.  That may be a long-gone bug, but we can deal with it now if it's
still live, by detaching within the infrastructure introduced here to deal
with closing the handle.

Back-patch to all supported branches.

Tom Lane and Amit Kapila
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
@@ -217,7 +217,7 @@ InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size)
 
 /****************************************************************************/
 /*	IpcMemoryDetach(status, shmaddr)	removes a shared memory segment		*/
-/*										from process' address spaceq		*/
+/*										from process' address space			*/
 /*	(called as an on_shmem_exit callback, hence funny argument list)		*/
 /****************************************************************************/
 static void
@@ -537,9 +537,10 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port)
 /*
  * PGSharedMemoryReAttach
  *
- * Re-attach to an already existing shared memory segment.  In the non
- * EXEC_BACKEND case this is not used, because postmaster children inherit
- * the shared memory segment attachment via fork().
+ * This is called during startup of a postmaster child process to re-attach to
+ * an already existing shared memory segment.  This is needed only in the
+ * EXEC_BACKEND case; otherwise postmaster children inherit the shared memory
+ * segment attachment via fork().
  *
  * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
  * routine.  The caller must have already restored them to the postmaster's
@@ -572,16 +573,52 @@ PGSharedMemoryReAttach(void)
 
 	UsedShmemSegAddr = hdr;		/* probably redundant */
 }
+
+/*
+ * PGSharedMemoryNoReAttach
+ *
+ * This is called during startup of a postmaster child process when we choose
+ * *not* to re-attach to the existing shared memory segment.  We must clean up
+ * to leave things in the appropriate state.  This is not used in the non
+ * EXEC_BACKEND case, either.
+ *
+ * The child process startup logic might or might not call PGSharedMemoryDetach
+ * after this; make sure that it will be a no-op if called.
+ *
+ * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
+ * routine.  The caller must have already restored them to the postmaster's
+ * values.
+ */
+void
+PGSharedMemoryNoReAttach(void)
+{
+	Assert(UsedShmemSegAddr != NULL);
+	Assert(IsUnderPostmaster);
+
+#ifdef __CYGWIN__
+	/* cygipc (currently) appears to not detach on exec. */
+	PGSharedMemoryDetach();
+#endif
+
+	/* For cleanliness, reset UsedShmemSegAddr to show we're not attached. */
+	UsedShmemSegAddr = NULL;
+	/* And the same for UsedShmemSegID. */
+	UsedShmemSegID = 0;
+}
+
 #endif   /* EXEC_BACKEND */
 
 /*
  * PGSharedMemoryDetach
  *
  * Detach from the shared memory segment, if still attached.  This is not
- * intended for use by the process that originally created the segment
- * (it will have an on_shmem_exit callback registered to do that).  Rather,
- * this is for subprocesses that have inherited an attachment and want to
- * get rid of it.
+ * intended to be called explicitly by the process that originally created the
+ * segment (it will have an on_shmem_exit callback registered to do that).
+ * Rather, this is for subprocesses that have inherited an attachment and want
+ * to get rid of it.
+ *
+ * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
+ * routine.
  */
 void
 PGSharedMemoryDetach(void)
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
@@ -16,7 +16,7 @@
 #include "storage/ipc.h"
 #include "storage/pg_shmem.h"
 
-HANDLE		UsedShmemSegID = 0;
+HANDLE		UsedShmemSegID = INVALID_HANDLE_VALUE;
 void	   *UsedShmemSegAddr = NULL;
 static Size UsedShmemSegSize = 0;
 
@@ -82,7 +82,6 @@ GetSharedMemName(void)
  * we only care about shmem segments that are associated with the intended
  * DataDir.  This is an important consideration since accidental matches of
  * shmem segment IDs are reasonably common.
- *
  */
 bool
 PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
@@ -114,7 +113,6 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
  * or recycle any existing segment. On win32, we always create a new segment,
  * since there is no need for recycling (segments go away automatically
  * when the last backend exits)
- *
  */
 PGShmemHeader *
 PGSharedMemoryCreate(Size size, bool makePrivate, int port)
@@ -211,9 +209,6 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port)
 		elog(LOG, "could not close handle to shared memory: error code %lu", GetLastError());
 
 
-	/* Register on-exit routine to delete the new segment */
-	on_shmem_exit(pgwin32_SharedMemoryDelete, PointerGetDatum(hmap2));
-
 	/*
 	 * Get a pointer to the new shared memory segment. Map the whole segment
 	 * at once, and let the system decide on the initial address.
@@ -246,14 +241,18 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port)
 	UsedShmemSegSize = size;
 	UsedShmemSegID = hmap2;
 
+	/* Register on-exit routine to delete the new segment */
+	on_shmem_exit(pgwin32_SharedMemoryDelete, PointerGetDatum(hmap2));
+
 	return hdr;
 }
 
 /*
  * PGSharedMemoryReAttach
  *
- * Re-attach to an already existing shared memory segment. Use the
- * handle inherited from the postmaster.
+ * This is called during startup of a postmaster child process to re-attach to
+ * an already existing shared memory segment, using the handle inherited from
+ * the postmaster.
  *
  * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
  * routine.  The caller must have already restored them to the postmaster's
@@ -288,37 +287,88 @@ PGSharedMemoryReAttach(void)
 	UsedShmemSegAddr = hdr;		/* probably redundant */
 }
 
+/*
+ * PGSharedMemoryNoReAttach
+ *
+ * This is called during startup of a postmaster child process when we choose
+ * *not* to re-attach to the existing shared memory segment.  We must clean up
+ * to leave things in the appropriate state.
+ *
+ * The child process startup logic might or might not call PGSharedMemoryDetach
+ * after this; make sure that it will be a no-op if called.
+ *
+ * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
+ * routine.  The caller must have already restored them to the postmaster's
+ * values.
+ */
+void
+PGSharedMemoryNoReAttach(void)
+{
+	Assert(UsedShmemSegAddr != NULL);
+	Assert(IsUnderPostmaster);
+
+	/*
+	 * Under Windows we will not have mapped the segment, so we don't need to
+	 * un-map it.  Just reset UsedShmemSegAddr to show we're not attached.
+	 */
+	UsedShmemSegAddr = NULL;
+
+	/*
+	 * We *must* close the inherited shmem segment handle, else Windows will
+	 * consider the existence of this process to mean it can't release the
+	 * shmem segment yet.  We can now use PGSharedMemoryDetach to do that.
+	 */
+	PGSharedMemoryDetach();
+}
+
 /*
  * PGSharedMemoryDetach
  *
  * Detach from the shared memory segment, if still attached.  This is not
- * intended for use by the process that originally created the segment. Rather,
- * this is for subprocesses that have inherited an attachment and want to
- * get rid of it.
+ * intended to be called explicitly by the process that originally created the
+ * segment (it will have an on_shmem_exit callback registered to do that).
+ * Rather, this is for subprocesses that have inherited an attachment and want
+ * to get rid of it.
+ *
+ * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
+ * routine.
  */
 void
 PGSharedMemoryDetach(void)
 {
+	/* Unmap the view, if it's mapped */
 	if (UsedShmemSegAddr != NULL)
 	{
 		if (!UnmapViewOfFile(UsedShmemSegAddr))
-			elog(LOG, "could not unmap view of shared memory: error code %lu", GetLastError());
+			elog(LOG, "could not unmap view of shared memory: error code %lu",
+				 GetLastError());
 
 		UsedShmemSegAddr = NULL;
 	}
+
+	/* And close the shmem handle, if we have one */
+	if (UsedShmemSegID != INVALID_HANDLE_VALUE)
+	{
+		if (!CloseHandle(UsedShmemSegID))
+			elog(LOG, "could not close handle to shared memory: error code %lu",
+				 GetLastError());
+
+		UsedShmemSegID = INVALID_HANDLE_VALUE;
+	}
 }
 
 
 /*
- *	pgwin32_SharedMemoryDelete(status, shmId)		deletes a shared memory segment
- *	(called as an on_shmem_exit callback, hence funny argument list)
+ * pgwin32_SharedMemoryDelete
+ *
+ * Detach from and delete the shared memory segment
+ * (called as an on_shmem_exit callback, hence funny argument list)
  */
 static void
 pgwin32_SharedMemoryDelete(int status, Datum shmId)
 {
+	Assert(DatumGetPointer(shmId) == UsedShmemSegID);
 	PGSharedMemoryDetach();
-	if (!CloseHandle(DatumGetPointer(shmId)))
-		elog(LOG, "could not close handle to shared memory: error code %lu", GetLastError());
 }
 
 /*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
@@ -4585,14 +4585,17 @@ SubPostmasterMain(int argc, char *argv[])
 	/*
 	 * If appropriate, physically re-attach to shared memory segment. We want
 	 * to do this before going any further to ensure that we can attach at the
-	 * same address the postmaster used.
+	 * same address the postmaster used.  On the other hand, if we choose not
+	 * to re-attach, we may have other cleanup to do.
 	 */
 	if (strcmp(argv[1], "--forkbackend") == 0 ||
 		strcmp(argv[1], "--forkavlauncher") == 0 ||
 		strcmp(argv[1], "--forkavworker") == 0 ||
 		strcmp(argv[1], "--forkboot") == 0 ||
 		strncmp(argv[1], "--forkbgworker=", 15) == 0)
 		PGSharedMemoryReAttach();
+	else
+		PGSharedMemoryNoReAttach();
 
 	/* autovacuum needs this set before calling InitProcess */
 	if (strcmp(argv[1], "--forkavlauncher") == 0)
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
@@ -48,6 +48,7 @@ extern HANDLE UsedShmemSegID;
 extern void *UsedShmemSegAddr;
 
 extern void PGSharedMemoryReAttach(void);
+extern void PGSharedMemoryNoReAttach(void);
 #endif
 
 extern PGShmemHeader *PGSharedMemoryCreate(Size size, bool makePrivate,
