Re: [PATCH] "could not reattach to shared memory" on Windows

Hello,

Alvaro Herrera <alvherre@commandprompt.com> wrote:
 > Tsutomu Yamada wrote:
 >
 > > This patch using VirtualAlloc()/VirtualFree() to avoid failing in
 > > reattach to shared memory.
 > >
 > > Can this be added to CommitFest ?
 >
 > Since this fixes a very annoying bug present in older versions, I think
 > this should be backpatched all the way back to 8.2.
 >
 > Some notes about the patch itself:
 >
 > - please use ereport() instead of elog() for error messages
 > - Are you really putting the pgwin32_ReserveSharedMemory declaration
 > inside a function?  Please move that into the appropriate header file.
 > - Failure to reserve memory in pgwin32_ReserveSharedMemory should be a
 > FATAL error I think, not simply LOG.

In this case,
the parent process operates child's memory by using VirtualAlloc().
If VirtualAlloc failed and be a FATAL error, master process will be stopped.

I think that is not preferable.
So, when VirtualAlloc failed, parent reports error and terminates child.

Revised patch

- move function declaration to include/port/win32.h
- add error check.
  when VirtualAlloc failed, parent will terminate child process.

Thanks.

--
Tsutomu Yamada
SRA OSS, Inc. Japan

Index: src/backend/port/win32_shmem.c
===================================================================
RCS file: /mnt/prj/pg/cvsmirror/pg/pgsql/src/backend/port/win32_shmem.c,v
retrieving revision 1.11
diff -c -r1.11 win32_shmem.c
*** src/backend/port/win32_shmem.c    11 Jun 2009 14:49:00 -0000    1.11
--- src/backend/port/win32_shmem.c    15 Jul 2009 08:56:34 -0000
***************
*** 18,23 ****
--- 18,24 ----

  unsigned long UsedShmemSegID = 0;
  void       *UsedShmemSegAddr = NULL;
+ static Size UsedShmemSegSize = 0;

  static void pgwin32_SharedMemoryDelete(int status, Datum shmId);

***************
*** 233,238 ****
--- 234,240 ----

      /* Save info for possible future use */
      UsedShmemSegAddr = memAddress;
+     UsedShmemSegSize = size;
      UsedShmemSegID = (unsigned long) hmap2;

      return hdr;
***************
*** 257,262 ****
--- 259,273 ----
      Assert(UsedShmemSegAddr != NULL);
      Assert(IsUnderPostmaster);

+     /* release memory region
+      * that reserved by parant process
+      */
+     if (VirtualFree(UsedShmemSegAddr, 0, MEM_RELEASE) == 0)
+     {
+         elog(LOG, "failed to release reserved memory region (addr=%p): %lu",
+              UsedShmemSegAddr, GetLastError());
+     }
+
      hdr = (PGShmemHeader *) MapViewOfFileEx((HANDLE) UsedShmemSegID, FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, 0,
UsedShmemSegAddr);
      if (!hdr)
          elog(FATAL, "could not reattach to shared memory (key=%d, addr=%p): %lu",
***************
*** 302,304 ****
--- 313,337 ----
      if (!CloseHandle((HANDLE) DatumGetInt32(shmId)))
          elog(LOG, "could not close handle to shared memory: %lu", GetLastError());
  }
+
+ /*
+  * pgwin32_ReserveSharedMemory(HANDLE pChild)
+  * Reserve shared memory area,
+  * BEFORE child process allocates memory for DLL and/or others.
+  */
+ int
+ pgwin32_ReserveSharedMemory(HANDLE pChild)
+ {
+     void *memAddress;
+
+     Assert(UsedShmemSegAddr != NULL);
+     Assert(UsedShmemSegSize != 0);
+     memAddress = VirtualAllocEx(pChild, UsedShmemSegAddr, UsedShmemSegSize,
+                                 MEM_RESERVE, PAGE_READWRITE);
+     if (memAddress == NULL) {
+         elog(LOG, "could not reserve shared memory region (addr=%p): %lu",
+              UsedShmemSegAddr, GetLastError());
+         return false;
+     }
+     return true;
+ }
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /mnt/prj/pg/cvsmirror/pg/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.584
diff -c -r1.584 postmaster.c
*** src/backend/postmaster/postmaster.c    8 Jul 2009 18:55:35 -0000    1.584
--- src/backend/postmaster/postmaster.c    15 Jul 2009 07:37:09 -0000
***************
*** 3643,3648 ****
--- 3643,3660 ----
          elog(LOG, "could not close handle to backend parameter file: error code %d",
               (int) GetLastError());

+     /* reserve shared memory area before ResumeThread() */
+     if (!pgwin32_ReserveSharedMemory(pi.hProcess))
+     {
+         if (!TerminateProcess(pi.hProcess, 255))
+             ereport(ERROR,
+                     (errmsg_internal("could not terminate process that failed to reserve memory: error code %d",
+                                      (int) GetLastError())));
+         CloseHandle(pi.hProcess);
+         CloseHandle(pi.hThread);
+         return -1;            /* elog() made by pgwin32_ReserveSharedMemory() */
+     }
+
      /*
       * Now that the backend variables are written out, we start the child
       * thread so it can start initializing while we set up the rest of the
Index: src/include/port/win32.h
===================================================================
RCS file: /mnt/prj/pg/cvsmirror/pg/pgsql/src/include/port/win32.h,v
retrieving revision 1.88
diff -c -r1.88 win32.h
*** src/include/port/win32.h    11 Jun 2009 14:49:12 -0000    1.88
--- src/include/port/win32.h    15 Jul 2009 08:39:23 -0000
***************
*** 288,293 ****
--- 288,296 ----
  extern int    pgwin32_is_service(void);
  #endif

+ /* in backend/port/win32_shmem.c */
+ extern int    pgwin32_ReserveSharedMemory(HANDLE);
+
  /* in port/win32error.c */
  extern void _dosmaperr(unsigned long);