Thread: [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 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 14 Jul 2009 10:11:44 -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,335 ---- 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. + */ + void + 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()); + } + } 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 13 Jul 2009 08:40:36 -0000 *************** *** 3643,3648 **** --- 3643,3655 ---- elog(LOG, "could not close handle to backend parameter file: error code %d", (int) GetLastError()); + { + /* reserve shared memory area before ResumeThread() */ + /* XXX: if it fail ? */ + extern void pgwin32_ReserveSharedMemory(HANDLE); + pgwin32_ReserveSharedMemory(pi.hProcess); + } + /* * 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
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
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
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: >> 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
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);
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 HaganderSelf: 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/
Attachment
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
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 HaganderSelf: 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 HaganderSelf: 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.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. 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 HaganderMe: 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 HaganderMe: 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 HaganderMe: 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. -- greg http://mit.edu/~gsstark/resume.pdf
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 HaganderMe: 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.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. > > 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 HaganderMe: 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/
Attachment
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Hi, We've come across this issue on 8.2.15 on a Windows Server 2008 instance. I noticed the patch hasn't been applied to the 8.2 branch yet. Any chances that this will be part of an eventual 8.2.16 release? Do you need more testing and feedback before commiting the patch? Thanks, Etienne Dube > * *From*: Magnus Hagander <magnus@hagander.net> > * *To*: Tom Lane <tgl@sss.pgh.pa.us> > * *Cc*: Tsutomu Yamada <tsutomu@sraoss.co.jp>, Alvaro Herrera > <alvherre@commandprompt.com>, pgsql-hackers@postgresql.org, Dave > Page <dpage@pgadmin.org> > * *Subject*: Re: [PATCH] "could not reattach to shared memory" on > Windows > * *Date*: Tue, 11 Aug 2009 17:14:08 +0200 > * *Message-id*: > <9837222c0908110814n414b2fcbxcaf7c0e1fcc05999@mail.gmail.com > <http://archives.postgresql.org/pgsql-hackers/2009-08/msg00894.php>> > > ------------------------------------------------------------------------ > 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/ > >
IIRC, we've had zero reports on whether the patch worked at all on 8.2 in an environment where the problem actually existed. So yes, some testing and feedback would be much apprecaited. //Magnus 2010/2/8 Etienne Dube <etdube@gmail.com>: > Hi, > > We've come across this issue on 8.2.15 on a Windows Server 2008 instance. I noticed the patch hasn't been applied to the8.2 branch yet. Any chances that this will be part of an eventual 8.2.16 release? Do you need more testing and feedbackbefore commiting the patch? > > Thanks, > > Etienne Dube > > >> * *From*: Magnus Hagander <magnus@hagander.net> >> * *To*: Tom Lane <tgl@sss.pgh.pa.us> >> * *Cc*: Tsutomu Yamada <tsutomu@sraoss.co.jp>, Alvaro Herrera >> <alvherre@commandprompt.com>, pgsql-hackers@postgresql.org, Dave >> Page <dpage@pgadmin.org> >> * *Subject*: Re: [PATCH] "could not reattach to shared memory" on >> Windows >> * *Date*: Tue, 11 Aug 2009 17:14:08 +0200 >> * *Message-id*: >> <9837222c0908110814n414b2fcbxcaf7c0e1fcc05999@mail.gmail.com >> <http://archives.postgresql.org/pgsql-hackers/2009-08/msg00894.php>> >> >> ------------------------------------------------------------------------ >> 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/ >> >> > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > IIRC, we've had zero reports on whether the patch worked at all on 8.2 > in an environment where the problem actually existed. So yes, some > testing and feedback would be much apprecaited. > > //Magnus > Thanks for your quick reply. We upgraded to Service Pack 2 and it solved the problem. Nevertheless, I'll try to reproduce the issue under a Win2008 SP1 VM to see whether the patch makes a difference. 8.2.x win32 binaries are built using MinGW right? Etienne > 2010/2/8 Etienne Dube <etdube@gmail.com>: > >> Hi, >> >> We've come across this issue on 8.2.15 on a Windows Server 2008 instance. I noticed the patch hasn't been applied to the8.2 branch yet. Any chances that this will be part of an eventual 8.2.16 release? Do you need more testing and feedbackbefore commiting the patch? >> >> Thanks, >> >> Etienne Dube >> >> >> >>> * *From*: Magnus Hagander <magnus@hagander.net> >>> * *To*: Tom Lane <tgl@sss.pgh.pa.us> >>> * *Cc*: Tsutomu Yamada <tsutomu@sraoss.co.jp>, Alvaro Herrera >>> <alvherre@commandprompt.com>, pgsql-hackers@postgresql.org, Dave >>> Page <dpage@pgadmin.org> >>> * *Subject*: Re: [PATCH] "could not reattach to shared memory" on >>> Windows >>> * *Date*: Tue, 11 Aug 2009 17:14:08 +0200 >>> * *Message-id*: >>> <9837222c0908110814n414b2fcbxcaf7c0e1fcc05999@mail.gmail.com >>> <http://archives.postgresql.org/pgsql-hackers/2009-08/msg00894.php>> >>> >>> ------------------------------------------------------------------------ >>> 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/ >>> >>> >>> >> -- >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >> >> > > > >
On 09/02/2010 4:09 PM, Etienne Dube wrote: > Magnus Hagander wrote: >> IIRC, we've had zero reports on whether the patch worked at all on 8.2 >> in an environment where the problem actually existed. So yes, some >> testing and feedback would be much apprecaited. >> >> //Magnus > > Thanks for your quick reply. > We upgraded to Service Pack 2 and it solved the problem. Nevertheless, > I'll try to reproduce the issue under a Win2008 SP1 VM to see whether > the patch makes a difference. 8.2.x win32 binaries are built using > MinGW right? > > Etienne > > The "could not reattach to shared memory" bug came back to bite us, this time on a production machine running Windows Server 2008 R2 x64. I manually applied the patch against the 8.2.17 sources and installed the build on a test server. It has been running for two days without any issue. We'll see after a while if the patch actually fixes the problem (it seems to happen only after the postgres service has been up and running for some time) but in case you want to include this fix in a future 8.2.18 release, I've attached the new patch to apply against the REL8_2_STABLE branch. Etienne
Attachment
On Tue, Jul 20, 2010 at 17:31, Etienne Dube <etdube@gmail.com> wrote: > On 09/02/2010 4:09 PM, Etienne Dube wrote: >> >> Magnus Hagander wrote: >>> >>> IIRC, we've had zero reports on whether the patch worked at all on 8.2 >>> in an environment where the problem actually existed. So yes, some >>> testing and feedback would be much apprecaited. >>> >>> //Magnus >> >> Thanks for your quick reply. >> We upgraded to Service Pack 2 and it solved the problem. Nevertheless, >> I'll try to reproduce the issue under a Win2008 SP1 VM to see whether the >> patch makes a difference. 8.2.x win32 binaries are built using MinGW right? >> >> Etienne >> >> > > > The "could not reattach to shared memory" bug came back to bite us, this > time on a production machine running Windows Server 2008 R2 x64. I manually > applied the patch against the 8.2.17 sources and installed the build on a > test server. It has been running for two days without any issue. We'll see > after a while if the patch actually fixes the problem (it seems to happen > only after the postgres service has been up and running for some time) but > in case you want to include this fix in a future 8.2.18 release, I've > attached the new patch to apply against the REL8_2_STABLE branch. Yes, I think it's time to backpatch this to 8.2 - it has worked very well on 8.3 and 8.4, and we've had a couple of good reports on 8.2 by now. So I've done that, so it should be in the next 8.2 version. In fact, there was a small bug in the patch that broke all non-win32 platforms, so I fixed that while at it :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/