[PATCH] "could not reattach to shared memory" on Windows
Hello,
This patch using VirtualAlloc()/VirtualFree() to avoid failing in
reattach to shared memory.
Can this be added to CommitFest ?
Recent threads in pgsql-bugs are
http://archives.postgresql.org/pgsql-bugs/2009-07/msg00036.php
This fix is almost same as previous patch. debug code is deleted.
http://archives.postgresql.org/pgsql-bugs/2009-07/msg00078.php
Regards,
--
Tsutomu Yamada
SRA OSS, Inc. Japan
On Tue, Jul 14, 2009 at 6:22 AM, Tsutomu Yamada<tsutomu@sraoss.co.jp> wrote:
Hello,
This patch using VirtualAlloc()/VirtualFree() to avoid failing in
reattach to shared memory.Can this be added to CommitFest ?
Patches for CommitFest should be added here:
http://commitfest.postgresql.org/action/commitfest_view/open
...Robert
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.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tsutomu Yamada wrote:
This patch using VirtualAlloc()/VirtualFree() to avoid failing in
reattach to shared memory.
Since this fixes a very annoying bug present in older versions, I think
this should be backpatched all the way back to 8.2.
Agreed, but first we need some evidence that it actually fixes the
problem. How can we acquire such evidence?
- please use ereport() instead of elog() for error messages
This is only appropriate if they're user-facing messages, which probably
errors in this area are not ...
regards, tom lane
Alvaro Herrera 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.
That doesn't sound like a good idea, at least not before we have more
experience of how the patch is working in the field.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tsutomu Yamada wrote:
This patch using VirtualAlloc()/VirtualFree() to avoid failing in
reattach to shared memory.Since this fixes a very annoying bug present in older versions, I think
this should be backpatched all the way back to 8.2.Agreed, but first we need some evidence that it actually fixes the
problem. How can we acquire such evidence?
Send the patch to the people who has reported trouble and see if it
seems gone? If somebody is able to build patched Win32 packages I could
point a couple of guys in the spanish list to them.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Since this fixes a very annoying bug present in older versions, I think
this should be backpatched all the way back to 8.2.Agreed, but first we need some evidence that it actually fixes the
problem. How can we acquire such evidence?
Apply to CVS HEAD and have people test it. I wouldn'ẗ be opposed to
back-patching to 8.4 where it would receive more testing in real life.
If we're really uneasy about it, provide a switch to turn it off if it
causes problems.
- please use ereport() instead of elog() for error messages
This is only appropriate if they're user-facing messages, which probably
errors in this area are not ...
Heh, that's what we hope :-).
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Tue, Jul 14, 2009 at 10:28 AM, Alvaro
Herrera<alvherre@commandprompt.com> wrote:
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tsutomu Yamada wrote:
This patch using VirtualAlloc()/VirtualFree() to avoid failing in
reattach to shared memory.Since this fixes a very annoying bug present in older versions, I think
this should be backpatched all the way back to 8.2.Agreed, but first we need some evidence that it actually fixes the
problem. How can we acquire such evidence?Send the patch to the people who has reported trouble and see if it
seems gone? If somebody is able to build patched Win32 packages I could
point a couple of guys in the spanish list to them.
- identify some people with the problem and talk to them for: 1) get a
way to reproduce the error (a lot dificult, IIRC we try a few times i
fail to fail) or 2) get their support for test
- commit it for the first alpha release, or the just talked nigthly
stable builds...
- let the tests begin :)
so, apply it just before the alpha and if it not works remove it just
after the alpha...
last time i build a win32 binary (not whole package) for windows users
to test a patch they disappear very quickly...
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
On Tuesday, July 14, 2009, Alvaro Herrera <alvherre@commandprompt.com> wrote:
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tsutomu Yamada wrote:
This patch using VirtualAlloc()/VirtualFree() to avoid failing in
reattach to shared memory.Since this fixes a very annoying bug present in older versions, I think
this should be backpatched all the way back to 8.2.Agreed, but first we need some evidence that it actually fixes the
problem. How can we acquire such evidence?Send the patch to the people who has reported trouble and see if it
seems gone? If somebody is able to build patched Win32 packages I could
point a couple of guys in the spanish list to them.
I built a version which a guy is currently testing. He could reproduce
the bug easily, but last i heard, the patch was looking good.
Don't have the details here tho.
--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
Jaime Casanova wrote:
- identify some people with the problem and talk to them for: 1) get a
way to reproduce the error (a lot dificult, IIRC we try a few times i
fail to fail) or 2) get their support for test
For back-patching, we'd be maybe even more interested in getting people
who *don't* experience the problem to test it, to make sure it doesn't
break installations that work without it.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
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
Import Notes
Reply to msg id not found: AlvaroHerrerasmsgofTue14Jul2009100237-0400.
On Wed, Jul 15, 2009 at 11:20, Tsutomu Yamada<tsutomu@sraoss.co.jp> wrote:
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.
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.
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Tue, Jul 21, 2009 at 14:06, Magnus Hagander<magnus@hagander.net> wrote:
On Wed, Jul 15, 2009 at 11:20, Tsutomu Yamada<tsutomu@sraoss.co.jp> wrote:
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.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/
Attachments:
virtualalloc83.patchtext/x-patch; charset=US-ASCII; name=virtualalloc83.patchDownload
*** 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);
virtualalloc84.patchtext/x-patch; charset=US-ASCII; name=virtualalloc84.patchDownload
*** 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);
Hello,
Thank you for correcting patch.
However, I think the following block have to use VirualFree*Ex*().
(yes, this should never happen, maybe there is actually no problem.
but for logical correctness)
+ 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);
VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
+ return false;
+ }
Regards,
--
Tsutomu Yamada
SRA OSS, Inc. Japan
Import Notes
Reply to msg id not found: MagnusHagandersmsgofWed22Jul2009170540+0200.
On Thu, Jul 23, 2009 at 08:04, Tsutomu Yamada<tsutomu@sraoss.co.jp> wrote:
Hello,
Thank you for correcting patch.
However, I think the following block have to use VirualFree*Ex*().(yes, this should never happen, maybe there is actually no problem.
but for logical correctness)
That is definitely correct. I have updated the patch in my tree and
will make sure to include that in the eventual commit.
FYI, and others, I have received a couple of off-list reports from
people testing out the patch, and so far only positive results.
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, Jul 23, 2009 at 09:04, Magnus Hagander<magnus@hagander.net> wrote:
On Thu, Jul 23, 2009 at 08:04, Tsutomu Yamada<tsutomu@sraoss.co.jp> wrote:
Hello,
Thank you for correcting patch.
However, I think the following block have to use VirualFree*Ex*().(yes, this should never happen, maybe there is actually no problem.
but for logical correctness)That is definitely correct. I have updated the patch in my tree and
will make sure to include that in the eventual commit.FYI, and others, I have received a couple of off-list reports from
people testing out the patch, and so far only positive results.
I have applied this patch to HEAD so we can get buildfarm coverage.
Holding back on the batckpatch for a bit longer.
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Wed, Jul 22, 2009 at 17:05, Magnus Hagander<magnus@hagander.net> wrote:
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.zipWe 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.
It's been a couple of weeks now, and I've had a number of reports both
on-list, on-blog and in private, from people using this. I have not
yet had a single report of a problem caused by this patch (not
counting the case where there was a version mismatch - can't fault the
patch for that).
Given that, I say we apply this for 8.3 and 8.4 now. Comments?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
It's been a couple of weeks now, and I've had a number of reports both
on-list, on-blog and in private, from people using this. I have not
yet had a single report of a problem caused by this patch (not
counting the case where there was a version mismatch - can't fault the
patch for that).
Given that, I say we apply this for 8.3 and 8.4 now. Comments?
8.2 as well, no?
regards, tom lane
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
It's been a couple of weeks now, and I've had a number of reports both
on-list, on-blog and in private, from people using this. I have not
yet had a single report of a problem caused by this patch (not
counting the case where there was a version mismatch - can't fault the
patch for that).Given that, I say we apply this for 8.3 and 8.4 now. Comments?
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a
separate patch, so let's get it out in 8.3 and 8.4 first.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Mon, Aug 10, 2009 at 3:33 PM, Magnus Hagander<magnus@hagander.net> wrote:
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
It's been a couple of weeks now, and I've had a number of reports both
on-list, on-blog and in private, from people using this. I have not
yet had a single report of a problem caused by this patch (not
counting the case where there was a version mismatch - can't fault the
patch for that).Given that, I say we apply this for 8.3 and 8.4 now. Comments?
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a
separate patch, so let's get it out in 8.3 and 8.4 first.
Has anyone reported the problem on 8.2?
--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
On Mon, Aug 10, 2009 at 16:45, Dave Page<dpage@pgadmin.org> wrote:
On Mon, Aug 10, 2009 at 3:33 PM, Magnus Hagander<magnus@hagander.net> wrote:
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
It's been a couple of weeks now, and I've had a number of reports both
on-list, on-blog and in private, from people using this. I have not
yet had a single report of a problem caused by this patch (not
counting the case where there was a version mismatch - can't fault the
patch for that).Given that, I say we apply this for 8.3 and 8.4 now. Comments?
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a
separate patch, so let's get it out in 8.3 and 8.4 first.Has anyone reported the problem on 8.2?
Yes. I've seen reports of it all the way back to 8.0. It does seem to
have increased in frequently with Win2003 and Win2008 as the server
platforms, which means the newer versions have had a higher
percentage, but the issue definitely exists.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Mon, Aug 10, 2009 at 3:49 PM, Magnus Hagander<magnus@hagander.net> wrote:
Has anyone reported the problem on 8.2?
Yes. I've seen reports of it all the way back to 8.0. It does seem to
have increased in frequently with Win2003 and Win2008 as the server
platforms, which means the newer versions have had a higher
percentage, but the issue definitely exists.
I suppose there's some question of whether this is the kind of issue
we need to bother supporting for back-branches. The whole point of
supporting back branches is so that people who are already using them
can expect to have any known problems they might run into fixed.
If people are still running these old branches then presumably their
setup isn't prone to this problem. If they're going to update to
Win2003 or Win2008 then that's a whole new installation, not an
existing installation which might suddenly run into this problem.
Is the reason we support old branches so that people can install those
old branches in preference to newer ones? Or just so that people who
have already installed them can continue to rely on them?
The flaws in this line of argument are that a) I'm not entirely sure
my premise that someone who has been running fine won't suddenly run
into this problem is true. And b) nor am I entirely clear that you
have to reinstall Postgres or other apps when you upgrade Windows.
Magnus Hagander <magnus@hagander.net> writes:
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a
separate patch, so let's get it out in 8.3 and 8.4 first.
If it's at all hard to do, I could see deprecating 8.2 for Windows
instead.
regards, tom lane
On Mon, Aug 10, 2009 at 3:58 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a -
separate patch, so let's get it out in 8.3 and 8.4 first.If it's at all hard to do, I could see deprecating 8.2 for Windows
instead.
I could most definitely agree with that on a personal level - no more
Mingw/msys builds to maintain :-)
Alas, it's probably not practical to drop it without inconveniencing a
great many Windows users.
--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
Dave Page wrote:
If it's at all hard to do, I could see deprecating 8.2 for Windows
instead.I could most definitely agree with that on a personal level - no more
Mingw/msys builds to maintain :-)Alas, it's probably not practical to drop it without inconveniencing a
great many Windows users.
I hope you're not suggesting we drop Mingw/MSys as a build platform,
even if you personally don't want to build with it. I would have found
it much harder to do parallel restore for Windows (which works quite
differently from Unix, and so had to be specifically developed) if I had
been forced to use the MS tool set with which I don't ever otherwise work.
I don't think we should deprecate 8.2 on Windows unless we really can't
backport this fix reasonably.
cheers
andrew
On Mon, Aug 10, 2009 at 4:29 PM, Andrew Dunstan<andrew@dunslane.net> wrote:
I hope you're not suggesting we drop Mingw/MSys as a build platform, even if
you personally don't want to build with it. I would have found it much
harder to do parallel restore for Windows (which works quite differently
from Unix, and so had to be specifically developed) if I had been forced to
use the MS tool set with which I don't ever otherwise work.
Not at all - in fact we need it to maintain some of the other apps
like PostGIS or Slony. I'm just talking about my own use of it for
building PG release builds.
I don't think we should deprecate 8.2 on Windows unless we really can't
backport this fix reasonably.
Agreed. There are too many users, and it wouldn't be fair to them.
--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
On Mon, Aug 10, 2009 at 16:58, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a
separate patch, so let's get it out in 8.3 and 8.4 first.If it's at all hard to do, I could see deprecating 8.2 for Windows
instead.
I haven't looked at how much work it would be at all yet. So let's do
that before we decide to deprecate anything. As mentioned downthread,
8.2 is a very widespread release, and we really want to avoid
deprecating it.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Mon, Aug 10, 2009 at 13:41, Magnus Hagander<magnus@hagander.net> wrote:
On Wed, Jul 22, 2009 at 17:05, Magnus Hagander<magnus@hagander.net> wrote:
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.zipWe 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.It's been a couple of weeks now, and I've had a number of reports both
on-list, on-blog and in private, from people using this. I have not
yet had a single report of a problem caused by this patch (not
counting the case where there was a version mismatch - can't fault the
patch for that).Given that, I say we apply this for 8.3 and 8.4 now. Comments?
Backpatched to 8.3 and 8.4 for now.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Mon, Aug 10, 2009 at 19:33, Magnus Hagander<magnus@hagander.net> wrote:
On Mon, Aug 10, 2009 at 16:58, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a
separate patch, so let's get it out in 8.3 and 8.4 first.If it's at all hard to do, I could see deprecating 8.2 for Windows
instead.I haven't looked at how much work it would be at all yet. So let's do
that before we decide to deprecate anything. As mentioned downthread,
8.2 is a very widespread release, and we really want to avoid
deprecating it.
Here's an attempt at a backport to 8.2. I haven't examined it in
detail, but it passes "make check" on mingw.
Comments?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
virtualalloc_82.patchtext/x-diff; charset=US-ASCII; name=virtualalloc_82.patchDownload
*** a/src/backend/port/sysv_shmem.c
--- b/src/backend/port/sysv_shmem.c
***************
*** 49,54 **** typedef int IpcMemoryId; /* shared memory ID returned by shmget(2) */
--- 49,57 ----
unsigned long UsedShmemSegID = 0;
void *UsedShmemSegAddr = NULL;
+ #ifdef WIN32
+ Size UsedShmemSegSize = 0;
+ #endif
static void *InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size);
static void IpcMemoryDetach(int status, Datum shmaddr);
***************
*** 403,408 **** PGSharedMemoryCreate(Size size, bool makePrivate, int port)
--- 406,412 ----
/* Save info for possible future use */
UsedShmemSegAddr = memAddress;
+ UsedShmemSegSize = size;
UsedShmemSegID = (unsigned long) NextShmemSegID;
return hdr;
*** a/src/backend/port/win32/shmem.c
--- b/src/backend/port/win32/shmem.c
***************
*** 12,19 ****
--- 12,22 ----
*/
#include "postgres.h"
+ #include "miscadmin.h"
static DWORD s_segsize = 0;
+ extern void *UsedShmemSegAddr;
+ extern Size UsedShmemSegSize;
/* Detach from a shared mem area based on its address */
int
***************
*** 29,34 **** shmdt(const void *shmaddr)
--- 32,44 ----
void *
shmat(int memId, void *shmaddr, int flag)
{
+ /* Release the memory region reserved in the postmaster */
+ if (IsUnderPostmaster)
+ {
+ if (VirtualFree(shmaddr, 0, MEM_RELEASE) == 0)
+ elog(FATAL, "failed to release reserved memory region (addr=%p): %lu",
+ shmaddr, GetLastError());
+ }
/* TODO -- shmat needs to count # attached to shared mem */
void *lpmem = MapViewOfFileEx((HANDLE) memId,
FILE_MAP_WRITE | FILE_MAP_READ,
***************
*** 128,130 **** shmget(int memKey, int size, int flag)
--- 138,190 ----
return (int) hmap;
}
+
+ /*
+ * 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);
+ VirtualFreeEx(hChild, address, 0, MEM_RELEASE);
+ return false;
+ }
+
+ return true;
+ }
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 3184,3190 **** 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());
--- 3184,3190 ----
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());
***************
*** 3193,3198 **** internal_forkexec(int argc, char *argv[], Port *port)
--- 3193,3217 ----
(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.
*** a/src/include/port/win32.h
--- b/src/include/port/win32.h
***************
*** 263,268 **** extern int pgwin32_is_admin(void);
--- 263,271 ----
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);
On Tue, Aug 11, 2009 at 16:30, Magnus Hagander<magnus@hagander.net> wrote:
On Mon, Aug 10, 2009 at 19:33, Magnus Hagander<magnus@hagander.net> wrote:
On Mon, Aug 10, 2009 at 16:58, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Mon, Aug 10, 2009 at 16:10, Tom Lane<tgl@sss.pgh.pa.us> wrote:
8.2 as well, no?
8.2 has a different shmem implementation - the one that emulates sysv
shmem. The patch will need to be changed around for that, and I
haven't looked at that. It may be worthwhile to do that, but it's a
separate patch, so let's get it out in 8.3 and 8.4 first.If it's at all hard to do, I could see deprecating 8.2 for Windows
instead.I haven't looked at how much work it would be at all yet. So let's do
that before we decide to deprecate anything. As mentioned downthread,
8.2 is a very widespread release, and we really want to avoid
deprecating it.Here's an attempt at a backport to 8.2. I haven't examined it in
detail, but it passes "make check" on mingw.Comments?
I've also built a binary that should be copy:able on top of an 8.2.13
installation made from the standard installer, to test this feature.
Anybody on 8.2 on Windows, please give it a shot and let us know how
it works.
http://www.hagander.net/pgsql/postgres_exe_virtualalloc_8_2.zip
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/