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

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

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/

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 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.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 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