Re: Postgres service stops when I kill client backend on Windows - Mailing list pgsql-hackers

I wrote:
> This is kind of a mess :-(.  But it does look like what we want is
> for SubPostmasterMain to do more than nothing when it chooses not to
> reattach.  Probably that should include resetting UsedShmemSegAddr to
> NULL, as well as closing the handle.

After poking around a bit more, I propose the attached patch.  I've
checked that this is happy with an EXEC_BACKEND Unix build, but I'm not
able to test it on Windows ... would somebody do that?

BTW, it appears from this that Cygwin builds have been broken right along
in a different way: according to the code in sysv_shmem's
PGSharedMemoryReAttach, Cygwin does cause a re-attach to occur, which we
were not undoing for putatively-not-connected-to-shmem child processes.
That's a robustness problem because it breaks the postmaster's expectation
that it's safe to not reinitialize shmem after a crash of one of those
processes.  I believe this patch fixes that problem as well, though if
anyone can test it on Cygwin that wouldn't be a bad thing either.

            regards, tom lane

diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 8be5bbe..c7a3a91 100644
*** a/src/backend/port/sysv_shmem.c
--- b/src/backend/port/sysv_shmem.c
*************** PGSharedMemoryReAttach(void)
*** 619,624 ****
--- 619,652 ----

      UsedShmemSegAddr = hdr;        /* probably redundant */
  }
+
+ /*
+  * PGSharedMemoryNoReAttach
+  *
+  * Clean up if we choose *not* to re-attach to an already existing shared
+  * memory segment.  This is not used in the non EXEC_BACKEND case, either.
+  *
+  * 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 */

  /*
*************** PGSharedMemoryReAttach(void)
*** 629,634 ****
--- 657,665 ----
   * (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
index db67627..8152522 100644
*** a/src/backend/port/win32_shmem.c
--- b/src/backend/port/win32_shmem.c
***************
*** 17,23 ****
  #include "storage/ipc.h"
  #include "storage/pg_shmem.h"

! HANDLE        UsedShmemSegID = 0;
  void       *UsedShmemSegAddr = NULL;
  static Size UsedShmemSegSize = 0;

--- 17,23 ----
  #include "storage/ipc.h"
  #include "storage/pg_shmem.h"

! HANDLE        UsedShmemSegID = INVALID_HANDLE_VALUE;
  void       *UsedShmemSegAddr = NULL;
  static Size UsedShmemSegSize = 0;

*************** PGSharedMemoryCreate(Size size, bool mak
*** 218,226 ****
          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.
--- 218,223 ----
*************** PGSharedMemoryCreate(Size size, bool mak
*** 254,259 ****
--- 251,259 ----
      UsedShmemSegSize = size;
      UsedShmemSegID = hmap2;

+     /* Register on-exit routine to delete the new segment */
+     on_shmem_exit(pgwin32_SharedMemoryDelete, PointerGetDatum(hmap2));
+
      *shim = hdr;
      return hdr;
  }
*************** PGSharedMemoryReAttach(void)
*** 299,321 ****
  }

  /*
   * 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.
   */
  void
  PGSharedMemoryDetach(void)
  {
      if (UsedShmemSegAddr != NULL)
      {
          if (!UnmapViewOfFile(UsedShmemSegAddr))
!             elog(LOG, "could not unmap view of shared memory: error code %lu", GetLastError());

          UsedShmemSegAddr = NULL;
      }
  }


--- 299,368 ----
  }

  /*
+  * PGSharedMemoryNoReAttach
+  *
+  * Clean up if we choose *not* to re-attach to an already existing shared
+  * memory segment.
+  *
+  * 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
+      * (this is important in case somebody calls PGSharedMemoryDetach later).
+      */
+     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
!  * (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());

          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;
+     }
  }


*************** PGSharedMemoryDetach(void)
*** 326,334 ****
  static void
  pgwin32_SharedMemoryDelete(int status, Datum shmId)
  {
      PGSharedMemoryDetach();
-     if (!CloseHandle(DatumGetPointer(shmId)))
-         elog(LOG, "could not close handle to shared memory: error code %lu", GetLastError());
  }

  /*
--- 373,380 ----
  static void
  pgwin32_SharedMemoryDelete(int status, Datum shmId)
  {
+     Assert(DatumGetPointer(shmId) == UsedShmemSegID);
      PGSharedMemoryDetach();
  }

  /*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 24e8404..90c2f4a 100644
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** SubPostmasterMain(int argc, char *argv[]
*** 4628,4634 ****
      /*
       * 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.
       */
      if (strcmp(argv[1], "--forkbackend") == 0 ||
          strcmp(argv[1], "--forkavlauncher") == 0 ||
--- 4628,4635 ----
      /*
       * 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.  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 ||
*************** SubPostmasterMain(int argc, char *argv[]
*** 4636,4641 ****
--- 4637,4644 ----
          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
index 0b169af..9dbcbce 100644
*** a/src/include/storage/pg_shmem.h
--- b/src/include/storage/pg_shmem.h
*************** extern void *UsedShmemSegAddr;
*** 61,66 ****
--- 61,67 ----

  #ifdef EXEC_BACKEND
  extern void PGSharedMemoryReAttach(void);
+ extern void PGSharedMemoryNoReAttach(void);
  #endif

  extern PGShmemHeader *PGSharedMemoryCreate(Size size, bool makePrivate,