Re: [PATCHES] Win32 shmem
On Tue, Mar 20, 2007 at 12:12:45PM -0400, Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Win32 shmem
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
/*-
*
* win32_shmem.c
* Implement shared memory using win32 facilities
*
* Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
* IDENTIFICATION
* $PostgreSQL$
*
*-
*/
#include postgres.h
#include miscadmin.h
#include storage/ipc.h
#include storage/pg_shmem.h
unsigned long UsedShmemSegID = 0;
void *UsedShmemSegAddr = NULL;
static void pgwin32_SharedMemoryDelete(int status, Datum shmId);
/*
* Generate shared memory segment name. Expand the data directory, to generate
* an identifier unique for this data directory. Then replace all backslashes
* with forward slashes, since backslashes aren't permitted in global object names.
*
* XXX: What happens with junctions? It's only someone breaking things on purpose,
* and this is still better than before, but we might want to do something about
* that sometime in the future.
*/
static char *
GetSharedMemName(void)
{
char *retptr;
DWORD bufsize;
DWORD r;
char *cp;
bufsize = GetFullPathName(DataDir, 0, NULL, NULL);
if (bufsize == 0)
elog(FATAL, could not get size for full pathname of datadir %s: %lu,
DataDir, GetLastError());
retptr = malloc(bufsize+1+11); // 1 NULL and 11 for PostgreSQL
if (retptr == NULL)
elog(FATAL, could not allocate memory for shared memory name);
strcpy(retptr,PostgreSQL:);
r = GetFullPathName(DataDir, bufsize, retptr+11, NULL);
if (r == 0 || r bufsize)
elog(FATAL, could not generate full pathname for datadir %s: %lu,
DataDir, GetLastError());
for (cp = retptr; *cp; cp++)
if (*cp == '\\')
*cp = '/';
return retptr;
}
/*
* PGSharedMemoryIsInUse
*
* Is a previously-existing shmem segment still existing and in use?
*
* The point of this exercise is to detect the case where a prior postmaster
* crashed, but it left child backends that are still running. Therefore
* we only care about shmem segments that are associated with the intended
* DataDir. This is an important consideration since accidental matches of
* shmem segment IDs are reasonably common.
*
*/
bool
PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
{
char *szShareMem;
HANDLE hmap;
szShareMem = GetSharedMemName();
hmap = OpenFileMapping(FILE_MAP_READ, FALSE, szShareMem);
free(szShareMem);
if (hmap == NULL)
return false;
CloseHandle(hmap);
return true;
}
/*
* PGSharedMemoryCreate
*
* Create a shared memory segment of the given size and initialize its
* standard header.
*
* makePrivate means to always create a new segment, rather than attach to
* or recycle any existing segment. On win32, we always create a new segment,
* since there is no need for recycling (segments go away automatically
* when the last backend exits)
*
*/
PGShmemHeader *
PGSharedMemoryCreate(Size size, bool makePrivate, int port)
{
void *memAddress;
PGShmemHeader *hdr;
HANDLE hmap, hmap2;
char *szShareMem;
/* Room for a header? */
Assert(size MAXALIGN(sizeof(PGShmemHeader)));
szShareMem = GetSharedMemName();
UsedShmemSegAddr = NULL;
hmap = CreateFileMapping((HANDLE) 0x, /* Use the pagefile */
NULL, /* Default security attrs */
PAGE_READWRITE, /* Memory is Read/Write */
0L, /* Size Upper 32 Bits */
(DWORD) size, /* Size Lower 32 bits */
szShareMem);
if (!hmap)
ereport(FATAL,
(errmsg(could not create shared memory segment: %lu, GetLastError()),
errdetail(Failed system call was CreateFileMapping(size=%lu, name=%s), size, szShareMem)));
/*
* If the segment already existed, CreateFileMapping() will return a handle to the
* existing one.
*/
if (GetLastError() == ERROR_ALREADY_EXISTS)
{
/*
* When recycling a shared memory segment, it may take a short while
* before it gets dropped from the global namespace. So re-try after
*
Re: [PATCHES] Win32 shmem
Magnus Hagander [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
[PATCHES] 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
/*-
*
* win32_shmem.c
* Implement shared memory using win32 facilities
*
* Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
* IDENTIFICATION
* $PostgreSQL$
*
*-
*/
#include postgres.h
#include miscadmin.h
#include storage/ipc.h
#include storage/pg_shmem.h
typedef HANDLE IpcMemoryKey; /* shared memory key passed to shmget(2) */
unsigned long UsedShmemSegID = 0;
void *UsedShmemSegAddr = NULL;
/*
* Generate shared memory segment name. Expand the data directory, to generate
* an identifier unique for this data directory. Then replace all backslashes
* with forward slashes, since backslashes aren't permitted in global object names.
*
* XXX: What happens with junctions? It's only someone breaking things on purpose,
* and this is still better than before, but we might want to do something about
* that sometime in the future.
*/
static char *
GetShareMemName(void)
{
char *retptr;
DWORD bufsize;
bufsize = GetFullPathName(DataDir, 0, NULL, NULL);
if (bufsize 0)
{
retptr = malloc(bufsize+1+11); // 1 NULL and 11 for PostgreSQL
if (retptr)
{
DWORD r;
strcpy(retptr,PostgreSQL:);
r = GetFullPathName(DataDir, bufsize, retptr+11, NULL);
if (r = bufsize r != 0)
{
char *cp;
for (cp = retptr; *cp; cp++)
if (*cp == '\\')
*cp = '/';
return retptr;
}
}
}
elog(FATAL, could not generate full pathname for datadir %s: %lu,
DataDir, GetLastError());
/* Silence compiler */
return NULL;
}
/*
* PGSharedMemoryIsInUse
*
* Is a previously-existing shmem segment still existing and in use?
*
* The point of this exercise is to detect the case where a prior postmaster
* crashed, but it left child backends that are still running. Therefore
* we only care about shmem segments that are associated with the intended
* DataDir. This is an important consideration since accidental matches of
* shmem segment IDs are reasonably common.
*
*/
bool
PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
{
char *szShareMem;
HANDLE hmap;
szShareMem = GetShareMemName();
printf(Attempting duplicate check on '%s'\n, szShareMem);
hmap = OpenFileMapping(FILE_MAP_READ, FALSE, szShareMem);
free(szShareMem);
if (hmap == NULL)
return false;
CloseHandle(hmap);
return true;
}
/*
* PGSharedMemoryCreate
*
* Create a shared memory segment of the given size and initialize its
* standard header.
*
* makePrivate means to always create a new segment, rather than attach to
* or recycle any existing segment. On win32, we always create a new segment,
* since there is no need for recycling (segments go away automatically
* when the last backend exits)
*
*/
PGShmemHeader *
PGSharedMemoryCreate(Size size, bool makePrivate, int port)
{
void *memAddress;
PGShmemHeader *hdr;
HANDLE hmap, hmap2;
char *szShareMem;
/* Room for a header? */
Assert(size MAXALIGN(sizeof(PGShmemHeader)));
szShareMem = GetShareMemName();
/* Make sure PGSharedMemoryAttach doesn't fail without need */
UsedShmemSegAddr = NULL;
hmap = CreateFileMapping((HANDLE) 0x, /* Use the swap file */
NULL,
PAGE_READWRITE, /* Memory is Read/Write */
0L, /* Size Upper 32 Bits */
(DWORD) size, /* Size Lower 32 bits */
szShareMem);
if (!hmap)
ereport(FATAL,
(errmsg(could not create shared memory segment: %lu, GetLastError()),
errdetail(Failed system call was CreateFileMapping(size=%lu, name=%s), size, szShareMem)));
if (GetLastError() == ERROR_ALREADY_EXISTS)
ereport(FATAL,
(errmsg(pre-existing shared memory block is still in use),
errhint(Check if there are any old server processes still running, and terminate them.)));
/*
* Make the handle inheritable
*/
if (!DuplicateHandle(GetCurrentProcess(), hmap, GetCurrentProcess(), hmap2, 0, TRUE, DUPLICATE_SAME_ACCESS))
ereport(FATAL,
(errmsg(could not create shared memory segment: %lu, GetLastError()),
errdetail(Failed system call was DuplicateHandle)));
CloseHandle(hmap);
memAddress = MapViewOfFileEx(hmap2, FILE_MAP_WRITE | FILE_MAP_READ, 0, 0, 0, NULL);
if (!memAddress)
{
Re: [PATCHES] Win32 shmem
Magnus Hagander [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
