Re: Win32 shmem

Attached is a first attempt at a "native win32 shmem implementatio",
based on previous discussions. Instead of emulating the sysv stuff. The
base stuff (using mapped pagefile) is still the same, of course.

Needs more testing, but has survived my tests so far. And unlike the
previous implementation, it does properly refuse to start a second
postmaster in a data directory where there is one or more backends still
running.

Does it seem like I've overlooked anything obvious in this? I do get the
feeling that this is too simple, but I don't know exactly where the
problem is :-)

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Attached is a first attempt at a "native win32 shmem implementatio",
> based on previous discussions. Instead of emulating the sysv stuff. The
> base stuff (using mapped pagefile) is still the same, of course.

> Needs more testing, but has survived my tests so far. And unlike the
> previous implementation, it does properly refuse to start a second
> postmaster in a data directory where there is one or more backends still
> running.

That's good...

> Does it seem like I've overlooked anything obvious in this? I do get the
> feeling that this is too simple, but I don't know exactly where the
> problem is :-)

I think you do still need the on_shmem_exit detach callback. Consider
the situation where the postmaster is trying to reinitialize after a
child crash. The Unix code is designed to detach and destroy the old
segment then create a new one. If that's not what you want to do then
this code still seems not right.

The coding of GetShareMemName seems involuted. I'd normally expect
success exit to be at the bottom of the routine, not deeply nested
like this. You may be saving one elog(FATAL) call this way, but
I'd argue that the two cases could do with different message texts
anyway. (Also, GetSharedMemName seems to read better.)

There seem to be a lot of system calls not checked for failure here.
Do they really not have any failure possibilities?

> errdetail("Failed system call was MapViewOfFileEx", size, szShareMem)));

Doesn't your compiler validate sprintf arguments?

I trust the committed file isn't going to have DOS line endings.

regards, tom lane

On Tue, Mar 20, 2007 at 07:41:32AM +0100, Magnus Hagander wrote:
> > > Does it seem like I've overlooked anything obvious in this? I do get the
> > > feeling that this is too simple, but I don't know exactly where the
> > > problem is :-)
> >
> > I think you do still need the on_shmem_exit detach callback. Consider
> > the situation where the postmaster is trying to reinitialize after a
> > child crash. The Unix code is designed to detach and destroy the old
> > segment then create a new one. If that's not what you want to do then
> > this code still seems not right.
>
> Ok, will look into that. Haven't tested that scenario.

That was indeed so. Added in new version, attached.

> > There seem to be a lot of system calls not checked for failure here.
> > Do they really not have any failure possibilities?

I looked it over, and didn't find "a lot". I found one or two (which are
now fixed). Are you referring to anything in particular?

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> I think you do still need the on_shmem_exit detach callback.
>>
>> Ok, will look into that. Haven't tested that scenario.

> That was indeed so. Added in new version, attached.

If it handles the restart-after-backend-crash scenario and correctly
locks out starting a fresh postmaster (after postmaster crash) until
all old backends are gone, then it's probably ready to commit for
more-widespread testing.

I note that sysv_shmem contains some #ifdef WIN32 and #ifdef __CYGWIN__
kluges; will it now be possible to remove those, or will the Cygwin
build still be using that code?

regards, tom lane

On Tue, Mar 20, 2007 at 12:12:45PM -0400, Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> >>> I think you do still need the on_shmem_exit detach callback.
> >>
> >> Ok, will look into that. Haven't tested that scenario.
>
> > That was indeed so. Added in new version, attached.
>
> If it handles the restart-after-backend-crash scenario and correctly
> locks out starting a fresh postmaster (after postmaster crash) until
> all old backends are gone, then it's probably ready to commit for
> more-widespread testing.

It does, at least in my tests. I have found one thing that needs to be
chagned for terminal server sessions, and then I need to update the build
system to use it on mingw as well. Will do that and then commit.

> I note that sysv_shmem contains some #ifdef WIN32 and #ifdef __CYGWIN__
> kluges; will it now be possible to remove those, or will the Cygwin
> build still be using that code?

I *think* so. I *think* the CYGWIN port does not rely on #ifdef WIN32s
anymore (which is corret given that it's not really win32). If I do a grep
of the sourcecode, I get a bunch of things like
./utils/mmgr/mcxt.c:#if defined(WIN32) || defined(__CYGWIN__)
which would indicate that at least some places know they're different.

I can include removal of those in my change, but I'm not in a position to
test them myself. I guess we could do it and see if the buildfarm breaks,
and if that revert it.

//Magnus