On Windows, ensure shared memory handle gets closed if not being used.
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 13 Oct 2015 15:21:33 +0000 (11:21 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 13 Oct 2015 15:21:33 +0000 (11:21 -0400)
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

src/backend/port/sysv_shmem.c
src/backend/port/win32_shmem.c
src/backend/postmaster/postmaster.c
src/include/storage/pg_shmem.h

index 8be5bbe1aba55d85be74d10109ee561d17c3a536..437ae4625dc7bd51589a6575ad79895e5e4e7444 100644 (file)
@@ -195,7 +195,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
@@ -583,9 +583,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
@@ -619,16 +620,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)
index db676278a69ab2061b78e10a1681613af60079f1..110bdcc703dce07b8a29b051051b8b2f1ce16a89 100644 (file)
@@ -17,7 +17,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;
 
@@ -83,7 +83,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)
@@ -115,7 +114,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,
@@ -218,9 +216,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.
@@ -254,6 +249,9 @@ 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));
+
    *shim = hdr;
    return hdr;
 }
@@ -261,8 +259,9 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
 /*
  * 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
@@ -298,37 +297,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());
 }
 
 /*
index 24e84043e29adbea692ab4119e9e98bd8deba25c..90c2f4a5e3286de20047640433779b220d5ee6d6 100644 (file)
@@ -4628,7 +4628,8 @@ 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 ||
@@ -4636,6 +4637,8 @@ SubPostmasterMain(int argc, char *argv[])
        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)
index 0b169aff69d885e26a88288e3a9a92f0b2d5d67a..9dbcbce0692f8ee794c22ff3098a45cfe4640dc5 100644 (file)
@@ -61,6 +61,7 @@ extern void *UsedShmemSegAddr;
 
 #ifdef EXEC_BACKEND
 extern void PGSharedMemoryReAttach(void);
+extern void PGSharedMemoryNoReAttach(void);
 #endif
 
 extern PGShmemHeader *PGSharedMemoryCreate(Size size, bool makePrivate,