Обсуждение: 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@hagander.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@hagander.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@hagander.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