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

On Tue, Jul 21, 2009 at 14:06, Magnus Hagander<magnus(at)hagander(dot)net> wrote:
> On Wed, Jul 15, 2009 at 11:20, Tsutomu Yamada<tsutomu(at)sraoss(dot)co(dot)jp> wrote:
>> Hello,
>>
>> Alvaro Herrera <alvherre(at)commandprompt(dot)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/