On Tue, Jul 21, 2009 at 14:06, Magnus Hagander<mag...@hagander.net> wrote:
> On Wed, Jul 15, 2009 at 11:20, Tsutomu Yamada<tsut...@sraoss.co.jp> wrote:
>> Hello,
>>
>> Alvaro Herrera <alvhe...@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.
>
> This patch looks a lot like one I've had sitting in my tree since
> before I left for three weeks of vacation, based on the same
> suggestion on the list. I will check if we have any actual functional
> differences, and merge yours with mine. The one I had worked fine in
> my testing.
>
> Once that is done, I propose the following:
>
> * Apply to HEAD. That will give us buildfarm coverage.
> * Produce a modified 8.4.0 *and* 8.3.7 binary for this, and ask people
> to test this. Both people with and without the problem.
> * Assuming it works for all users, backpatch to 8.2, 8.3 and 8.4.
Attached are two updated versions of this patch, one for 8.4 and one
for 8.3. They differ only in line numbers. I've merged your patch with
mine, which mainly contained of more comments. One functionality check
- to make sure the VirtualAllocEx() call returns the same address as
our base one. It should always do this, but my patch adds a check t
make sure this is true.
Dave has built binaries for 8.3.7 and 8.4.0 for this, available at:
http://developer.pgadmin.org/~dpage/postgres_exe_virtualalloc-8_3.zip
http://developer.pgadmin.org/~dpage/postgres_exe_virtualalloc-8_4.zip
We would like as many people as possible to test this both on systems
that currently experience the problem and systems that don't, and let
us know the status. To test, just replace your current postgres.exe
binary with the one in the appropriate ZIP file above. Obviously, take
a backup before you do it! These binaries contain just this one patch
- the rest of what's been applied to the 8.3 and 8.4 branches for the
next minor version is *not* included.
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
*** src/backend/port/win32_shmem.c
--- src/backend/port/win32_shmem.c
***************
*** 18,23 ****
--- 18,24 ----
unsigned long UsedShmemSegID = 0;
void *UsedShmemSegAddr = NULL;
+ static Size UsedShmemSegSize = 0;
static void pgwin32_SharedMemoryDelete(int status, Datum shmId);
***************
*** 229,234 **** PGSharedMemoryCreate(Size size, bool makePrivate, int port)
--- 230,236 ----
/* Save info for possible future use */
UsedShmemSegAddr = memAddress;
+ UsedShmemSegSize = size;
UsedShmemSegID = (unsigned long) hmap2;
return hdr;
***************
*** 253,258 **** PGSharedMemoryReAttach(void)
--- 255,267 ----
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
+ /*
+ * Release memory region reservation that was made by the postmaster
+ */
+ if (VirtualFree(UsedShmemSegAddr, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "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",
***************
*** 298,300 **** pgwin32_SharedMemoryDelete(int status, Datum shmId)
--- 307,359 ----
if (!CloseHandle((HANDLE) DatumGetInt32(shmId)))
elog(LOG, "could not close handle to shared memory: %lu", GetLastError());
}
+
+ /*
+ * pgwin32_ReserveSharedMemoryRegion(hChild)
+ *
+ * Reserve the memory region that will be used for shared memory in a child
+ * process. It is called before the child process starts, to make sure the
+ * memory is available.
+ *
+ * Once the child starts, DLLs loading in different order or threads getting
+ * scheduled differently may allocate memory which can conflict with the
+ * address space we need for our shared memory. By reserving the shared
+ * memory region before the child starts, and freeing it only just before we
+ * attempt to get access to the shared memory forces these allocations to
+ * be given different address ranges that don't conflict.
+ *
+ * NOTE! This function executes in the postmaster, and should for this
+ * reason not use elog(FATAL) since that would take down the postmaster.
+ */
+ int
+ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
+ {
+ void *address;
+
+ Assert(UsedShmemSegAddr != NULL);
+ Assert(UsedShmemSegSize != 0);
+
+ address = VirtualAllocEx(hChild, UsedShmemSegAddr, UsedShmemSegSize,
+ MEM_RESERVE, PAGE_READWRITE);
+ if (address == NULL) {
+ /* Don't use FATAL since we're running in the postmaster */
+ elog(LOG, "could not reserve shared memory region (addr=%p) for child %lu: %lu",
+ UsedShmemSegAddr, hChild, GetLastError());
+ return false;
+ }
+ if (address != UsedShmemSegAddr)
+ {
+ /*
+ * Should never happen - in theory if allocation granularity causes strange
+ * effects it could, so check just in case.
+ *
+ * Don't use FATAL since we're running in the postmaster.
+ */
+ elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
+ address, UsedShmemSegAddr);
+ VirtualFree(address, 0, MEM_RELEASE);
+ return false;
+ }
+
+ return true;
+ }
*** src/backend/postmaster/postmaster.c
--- src/backend/postmaster/postmaster.c
***************
*** 3447,3453 **** internal_forkexec(int argc, char *argv[], Port *port)
return -1; /* log made by save_backend_variables */
}
! /* Drop the shared memory that is now inherited to the backend */
if (!UnmapViewOfFile(param))
elog(LOG, "could not unmap view of backend parameter file: error code %d",
(int) GetLastError());
--- 3447,3453 ----
return -1; /* log made by save_backend_variables */
}
! /* Drop the parameter shared memory that is now inherited to the backend */
if (!UnmapViewOfFile(param))
elog(LOG, "could not unmap view of backend parameter file: error code %d",
(int) GetLastError());
***************
*** 3456,3461 **** internal_forkexec(int argc, char *argv[], Port *port)
--- 3456,3480 ----
(int) GetLastError());
/*
+ * Reserve the memory region used by our main shared memory segment before we
+ * resume the child process.
+ */
+ if (!pgwin32_ReserveSharedMemoryRegion(pi.hProcess))
+ {
+ /*
+ * Failed to reserve the memory, so terminate the newly created
+ * process and give up.
+ */
+ 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; /* logging done made by pgwin32_ReserveSharedMemoryRegion() */
+ }
+
+ /*
* 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
* parent state.
*** src/include/port/win32.h
--- src/include/port/win32.h
***************
*** 286,291 **** extern int pgwin32_is_admin(void);
--- 286,294 ----
extern int pgwin32_is_service(void);
#endif
+ /* in backend/port/win32_shmem.c */
+ extern int pgwin32_ReserveSharedMemoryRegion(HANDLE);
+
/* in port/win32error.c */
extern void _dosmaperr(unsigned long);
*** src/backend/port/win32_shmem.c
--- src/backend/port/win32_shmem.c
***************
*** 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 **** PGSharedMemoryCreate(Size size, bool makePrivate, int port)
--- 234,240 ----
/* Save info for possible future use */
UsedShmemSegAddr = memAddress;
+ UsedShmemSegSize = size;
UsedShmemSegID = (unsigned long) hmap2;
return hdr;
***************
*** 257,262 **** PGSharedMemoryReAttach(void)
--- 259,271 ----
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
+ /*
+ * Release memory region reservation that was made by the postmaster
+ */
+ if (VirtualFree(UsedShmemSegAddr, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "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 **** pgwin32_SharedMemoryDelete(int status, Datum shmId)
--- 311,363 ----
if (!CloseHandle((HANDLE) DatumGetInt32(shmId)))
elog(LOG, "could not close handle to shared memory: %lu", GetLastError());
}
+
+ /*
+ * pgwin32_ReserveSharedMemoryRegion(hChild)
+ *
+ * Reserve the memory region that will be used for shared memory in a child
+ * process. It is called before the child process starts, to make sure the
+ * memory is available.
+ *
+ * Once the child starts, DLLs loading in different order or threads getting
+ * scheduled differently may allocate memory which can conflict with the
+ * address space we need for our shared memory. By reserving the shared
+ * memory region before the child starts, and freeing it only just before we
+ * attempt to get access to the shared memory forces these allocations to
+ * be given different address ranges that don't conflict.
+ *
+ * NOTE! This function executes in the postmaster, and should for this
+ * reason not use elog(FATAL) since that would take down the postmaster.
+ */
+ int
+ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
+ {
+ void *address;
+
+ Assert(UsedShmemSegAddr != NULL);
+ Assert(UsedShmemSegSize != 0);
+
+ address = VirtualAllocEx(hChild, UsedShmemSegAddr, UsedShmemSegSize,
+ MEM_RESERVE, PAGE_READWRITE);
+ if (address == NULL) {
+ /* Don't use FATAL since we're running in the postmaster */
+ elog(LOG, "could not reserve shared memory region (addr=%p) for child %lu: %lu",
+ UsedShmemSegAddr, hChild, GetLastError());
+ return false;
+ }
+ if (address != UsedShmemSegAddr)
+ {
+ /*
+ * Should never happen - in theory if allocation granularity causes strange
+ * effects it could, so check just in case.
+ *
+ * Don't use FATAL since we're running in the postmaster.
+ */
+ elog(LOG, "reserved shared memory region got incorrect address %p, expected %p",
+ address, UsedShmemSegAddr);
+ VirtualFree(address, 0, MEM_RELEASE);
+ return false;
+ }
+
+ return true;
+ }
*** src/backend/postmaster/postmaster.c
--- src/backend/postmaster/postmaster.c
***************
*** 3635,3641 **** internal_forkexec(int argc, char *argv[], Port *port)
return -1; /* log made by save_backend_variables */
}
! /* Drop the shared memory that is now inherited to the backend */
if (!UnmapViewOfFile(param))
elog(LOG, "could not unmap view of backend parameter file: error code %d",
(int) GetLastError());
--- 3635,3641 ----
return -1; /* log made by save_backend_variables */
}
! /* Drop the parameter shared memory that is now inherited to the backend */
if (!UnmapViewOfFile(param))
elog(LOG, "could not unmap view of backend parameter file: error code %d",
(int) GetLastError());
***************
*** 3644,3649 **** internal_forkexec(int argc, char *argv[], Port *port)
--- 3644,3668 ----
(int) GetLastError());
/*
+ * Reserve the memory region used by our main shared memory segment before we
+ * resume the child process.
+ */
+ if (!pgwin32_ReserveSharedMemoryRegion(pi.hProcess))
+ {
+ /*
+ * Failed to reserve the memory, so terminate the newly created
+ * process and give up.
+ */
+ 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; /* logging done made by pgwin32_ReserveSharedMemoryRegion() */
+ }
+
+ /*
* 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
* parent state.
*** src/include/port/win32.h
--- src/include/port/win32.h
***************
*** 288,293 **** extern int pgwin32_is_admin(void);
--- 288,296 ----
extern int pgwin32_is_service(void);
#endif
+ /* in backend/port/win32_shmem.c */
+ extern int pgwin32_ReserveSharedMemoryRegion(HANDLE);
+
/* in port/win32error.c */
extern void _dosmaperr(unsigned long);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
