Thread: windows shared memory error
I am seeing Postgres 8.3.7 running as a service on Windows Server 2003
repeatedly fail to restart after a backend crash because of the
following code in port/win32_shmem.c:
/* * 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
shortwhile * before it gets dropped from the global namespace. So re-try after * sleeping for a second.
*/ CloseHandle(hmap); /* Close the old handle, since we got a
valid * one to the previous segment. */
Sleep(1000);
hmap = CreateFileMapping((HANDLE) 0xFFFFFFFF, NULL,
PAGE_READWRITE, 0L, (DWORD) size, szShareMem); if (!hmap) ereport(FATAL,
(errmsg("couldnot create shared memory segment:
%lu", GetLastError()), errdetail("Failed system call was
CreateFileMapping(size=%lu, name=%s).", (unsigned long) size, szShareMem)));
if (GetLastError() == ERROR_ALREADY_EXISTS) ereport(FATAL, (errmsg("pre-existing shared
memoryblock is still in
use"), errhint("Check if there are any old server processes
still running, and terminate them."))); }
It strikes me that we really need to try reconnecting to the shared
memory here several times, and maybe the backoff need to increase each
time. On a loaded server this cause postgres to fail to restart fairly
reliably.
thoughts?
cheers
andrew
On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > > It strikes me that we really need to try reconnecting to the shared memory > here several times, and maybe the backoff need to increase each time. On a > loaded server this cause postgres to fail to restart fairly reliably. At the risk of sounding predictable, +1. Maybe try 5 times, repeating at 1, 2, 4 & 8 seconds? Any longer seems like it will be a genuine failure (so does 8 seconds in fact, but I don't suppose it'll hurt to try). -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Fri, May 1, 2009 at 8:42 AM, Dave Page <dpage@pgadmin.org> wrote: > On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan <andrew@dunslane.net> wrote: >> >> It strikes me that we really need to try reconnecting to the shared memory >> here several times, and maybe the backoff need to increase each time. On a >> loaded server this cause postgres to fail to restart fairly reliably. > > At the risk of sounding predictable, +1. Maybe try 5 times, repeating > at 1, 2, 4 & 8 seconds? Any longer seems like it will be a genuine > failure (so does 8 seconds in fact, but I don't suppose it'll hurt to > try). Do we have any idea why "it may take a short while before it gets dropped from the global namespace"? Is there some demon running which only wakes up periodically? Or any specific reason it takes so long? That might give us a clue exactly how long to sleep for. Is there any way to tell Windows to wake up and do its job? Or to block until its done? -- greg
On Fri, May 1, 2009 at 11:05 AM, Greg Stark <stark@enterprisedb.com> wrote: > Do we have any idea why "it may take a short while before it gets > dropped from the global namespace"? Is there some demon running which > only wakes up periodically? Or any specific reason it takes so long? > That might give us a clue exactly how long to sleep for. I can't find any info in a quick search. I don't see offhand why it would take time - it's just reference counting handles held by different processes. > Is there any way to tell Windows to wake up and do its job? Or to > block until its done? Not that I'm aware of. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page wrote: > On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan <andrew@dunslane.net> wrote: >> It strikes me that we really need to try reconnecting to the shared memory >> here several times, and maybe the backoff need to increase each time. On a >> loaded server this cause postgres to fail to restart fairly reliably. > > At the risk of sounding predictable, +1. Maybe try 5 times, repeating > at 1, 2, 4 & 8 seconds? Any longer seems like it will be a genuine > failure (so does 8 seconds in fact, but I don't suppose it'll hurt to > try). 1+2+4+8 = 15 seconds -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Fri, May 1, 2009 at 4:10 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Dave Page wrote: >> >> On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan <andrew@dunslane.net> >> wrote: >>> >>> It strikes me that we really need to try reconnecting to the shared >>> memory >>> here several times, and maybe the backoff need to increase each time. On >>> a >>> loaded server this cause postgres to fail to restart fairly reliably. >> >> At the risk of sounding predictable, +1. Maybe try 5 times, repeating >> at 1, 2, 4 & 8 seconds? Any longer seems like it will be a genuine >> failure (so does 8 seconds in fact, but I don't suppose it'll hurt to >> try). > > 1+2+4+8 = 15 seconds Err, yes. What's your point? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Heikki Linnakangas wrote: > Dave Page wrote: >> On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan <andrew@dunslane.net> >> wrote: >>> It strikes me that we really need to try reconnecting to the shared >>> memory >>> here several times, and maybe the backoff need to increase each >>> time. On a >>> loaded server this cause postgres to fail to restart fairly reliably. >> >> At the risk of sounding predictable, +1. Maybe try 5 times, repeating >> at 1, 2, 4 & 8 seconds? Any longer seems like it will be a genuine >> failure (so does 8 seconds in fact, but I don't suppose it'll hurt to >> try). > > 1+2+4+8 = 15 seconds > 15 seconds beats the hell out of not restarting at all. cheers andrew
Dave Page wrote: > On Fri, May 1, 2009 at 4:10 PM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: >> Dave Page wrote: >>> On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan <andrew@dunslane.net> >>> wrote: >>>> It strikes me that we really need to try reconnecting to the shared >>>> memory >>>> here several times, and maybe the backoff need to increase each time. On >>>> a >>>> loaded server this cause postgres to fail to restart fairly reliably. >>> At the risk of sounding predictable, +1. Maybe try 5 times, repeating >>> at 1, 2, 4 & 8 seconds? Any longer seems like it will be a genuine >>> failure (so does 8 seconds in fact, but I don't suppose it'll hurt to >>> try). >> 1+2+4+8 = 15 seconds > > Err, yes. What's your point? If 8 seconds already seems like it's a genuine failure, then perhaps retrying at 1, 2 and 4 seconds giving a total delay of 7 seconds is enough. Maybe you meant that 15 s seems like a genuine failure? Well, either way, never mind :-) -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> It strikes me that we really need to try reconnecting to the shared
> memory here several times, and maybe the backoff need to increase each
> time.
Adding a backoff would make the code significantly more complex, with
no gain that I can see. Just loop a few times around the
one-second-sleep-and-retry logic.
I concur with Greg's opinion that the need for a sleep here at all
is pretty fishy, but I doubt anyone really cares enough to find out
exactly what's happening (and it being Windows, there may be no better
solution anyway ...)
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> It strikes me that we really need to try reconnecting to the shared >> memory here several times, and maybe the backoff need to increase each >> time. >> > > Adding a backoff would make the code significantly more complex, with > no gain that I can see. Just loop a few times around the > one-second-sleep-and-retry logic. > > I concur with Greg's opinion that the need for a sleep here at all > is pretty fishy, but I doubt anyone really cares enough to find out > exactly what's happening (and it being Windows, there may be no better > solution anyway ...) > > > We've seen similar things with other Windows file operations, IIRC. What bothers me is that the problem might be precisely because the 1 second sleep between the CloseHandle() call and the CreateFileMapping() call might not be enough due to system load, so repeating the cycle without increasing the sleep period will just repeat the error. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> We've seen similar things with other Windows file operations, IIRC. What
> bothers me is that the problem might be precisely because the 1 second
> sleep between the CloseHandle() call and the CreateFileMapping() call
> might not be enough due to system load, so repeating the cycle without
> increasing the sleep period will just repeat the error.
What system load? This is only called after all the backends are dead.
And surely one CreateFileMapping syscall per second does not materially
contribute to any load that is being caused by something else.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> We've seen similar things with other Windows file operations, IIRC. What >> bothers me is that the problem might be precisely because the 1 second >> sleep between the CloseHandle() call and the CreateFileMapping() call >> might not be enough due to system load, so repeating the cycle without >> increasing the sleep period will just repeat the error. >> > > What system load? This is only called after all the backends are dead. > And surely one CreateFileMapping syscall per second does not materially > contribute to any load that is being caused by something else. > > > I didn't say Postgres was creating the load. In the case where this has been happening for my client, there is an Apache server which can chew up the machine mightily. I don't have any evidence that just repeating the cycle a few times won't work, but neither do you have any that it will, and I don't think the extra code complexity will be terribly great. If it were more than a few extra lines I'd probably agree with you. cheers abdrew
Andrew Dunstan <andrew@dunslane.net> writes: > I am seeing Postgres 8.3.7 running as a service on Windows Server 2003 > repeatedly fail to restart after a backend crash because of the > following code in port/win32_shmem.c: On further review, I see an entirely different explanation for possible failures of that code. It says here: http://msdn.microsoft.com/en-us/library/ms885627.aspx that GetLastError() continues to return the same error code until someone calls SetLastError() to change it. It further says that only a few operating system functions call SetLastError(0) on success, and that it is explicitly documented whenever a function does so. I see no such statement for CreateFileMapping: http://msdn.microsoft.com/en-us/library/aa366537(VS.85).aspx This leads me to conclude that after a successful creation, GetLastError will return whatever the errno previously was, meaning that you cannot reliably distinguish creation from non creation unless you do SetLastError(0) beforehand. Which we don't. Now this would only explain problems if there were some code path through the postmaster that could leave the errno set to ERROR_ALREADY_EXISTS (a/k/a EEXIST) when this code is reached. I'm not sure there is one, and I have even less of a theory as to why system load might make it more probable to happen. Still, this looks like a bug from here, and repeating the create call won't fix it. regards, tom lane
Tom Lane wrote: > > Now this would only explain problems if there were some code path > through the postmaster that could leave the errno set to > ERROR_ALREADY_EXISTS (a/k/a EEXIST) when this code is reached. I'm not > sure there is one, and I have even less of a theory as to why system > load might make it more probable to happen. Still, this looks like a > bug from here, and repeating the create call won't fix it. > > > Oh, I think that this code has such a path. We already know that the code I showed is entered when that error is set. So the solution would be to put SetError(0) before the call to CreateFileMapping(), possibly before both such calls. Maybe we need to look at all the places we call GetLastError(). There are quite a few of them. Good catch, BTW. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > Maybe we need to look at all the places we call GetLastError(). There > are quite a few of them. It would only be an issue with syscalls that have badly designed APIs like this one. Most of the time you know that the function has failed and is supposed to have set the error code. What I'm wondering about right now is whether the sleep-and-retry logic is needed at all, if we get the error code handling straight. A look in the archives says that Magnus added it between these two versions of his draft patch: http://archives.postgresql.org//pgsql-patches/2007-03/msg00250.php http://archives.postgresql.org//pgsql-patches/2007-03/msg00263.php without any indication of why, except that I had complained that some error checks seemed to be missing in the original. I wonder if it was a misguided attempt to fix a stale-error-code problem. regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> I am seeing Postgres 8.3.7 running as a service on Windows Server 2003 >> repeatedly fail to restart after a backend crash because of the >> following code in port/win32_shmem.c: > > On further review, I see an entirely different explanation for possible > failures of that code. > > It says here: > http://msdn.microsoft.com/en-us/library/ms885627.aspx FWIW, this is the Windows CE documentation. The one for win32 is at: http://msdn.microsoft.com/en-us/library/ms679360(VS.85).aspx > that GetLastError() continues to return the same error code until > someone calls SetLastError() to change it. It further says that > only a few operating system functions call SetLastError(0) on success, > and that it is explicitly documented whenever a function does so. > I see no such statement for CreateFileMapping: > http://msdn.microsoft.com/en-us/library/aa366537(VS.85).aspx > > This leads me to conclude that after a successful creation, > GetLastError will return whatever the errno previously was, > meaning that you cannot reliably distinguish creation from non > creation unless you do SetLastError(0) beforehand. Which we don't. > > Now this would only explain problems if there were some code path > through the postmaster that could leave the errno set to > ERROR_ALREADY_EXISTS (a/k/a EEXIST) when this code is reached. I'm not > sure there is one, and I have even less of a theory as to why system > load might make it more probable to happen. Still, this looks like a > bug from here, and repeating the create call won't fix it. The ref page for CreateFileMapping you linked has: "If the object exists before the function call, the function returns a handle to the existing object (with its current size, not the specified size), and GetLastError returns ERROR_ALREADY_EXISTS. " I think that qualifies as it documenting that it's setting the return value, no? That would never work if it isn't set to something other than ERROR_ALREADY_EXISTS (probably zero) when it *didn't* already exist. The quick try would be to stick a SetLastError(0) in there, just to be sure... Could be worth a try? Andrew, just to confirm: you've found a case where this happens *repeatably*? That's what we've failed to do before - it's happened now and then, but never during testing... //Magnus
Andrew Dunstan wrote: > > > Tom Lane wrote: >> >> Now this would only explain problems if there were some code path >> through the postmaster that could leave the errno set to >> ERROR_ALREADY_EXISTS (a/k/a EEXIST) when this code is reached. I'm not >> sure there is one, and I have even less of a theory as to why system >> load might make it more probable to happen. Still, this looks like a >> bug from here, and repeating the create call won't fix it. >> >> >> > > Oh, I think that this code has such a path. We already know that the > code I showed is entered when that error is set. So the solution would > be to put SetError(0) before the call to CreateFileMapping(), possibly > before both such calls. > > Maybe we need to look at all the places we call GetLastError(). There > are quite a few of them. A quick look shows that all of these except the one in pgwin32_get_dynamic_tokeninfo() (which uses a documented way to check the return code in the case of success) are only called after an API function fails, so we should be safe there. //Magnus
Magnus Hagander wrote: > > Andrew, just to confirm: you've found a case where this happens > *repeatably*? That's what we've failed to do before - it's happened now > and then, but never during testing... > > > Well, it happened several times to my client within a matter of hours. I didn't see any successful restarts on the log. Unfortunately, I can't use this system for experimentation - it's doing extremely urgent production work. cheers andrew
Magnus Hagander <magnus@hagander.net> writes: > Tom Lane wrote: >> It says here: >> http://msdn.microsoft.com/en-us/library/ms885627.aspx > FWIW, this is the Windows CE documentation. The one for win32 is at: > http://msdn.microsoft.com/en-us/library/ms679360(VS.85).aspx Sorry, that was the one that came up first in a Google search ... > The ref page for CreateFileMapping you linked has: > "If the object exists before the function call, the function returns a > handle to the existing object (with its current size, not the specified > size), and GetLastError returns ERROR_ALREADY_EXISTS. " > I think that qualifies as it documenting that it's setting the return > value, no? The question is what it does when creating a new object. To be sure that our existing code isn't misled, it'd be necessary for CreateFileMapping to do SetLastError(0) in the successful-creation code path. What I read the GetLastError page to be saying is that most functions do *not* do SetLastError(0) on success, and that it is always documented if they do. > The quick try would be to stick a SetLastError(0) in there, just to be > sure... Could be worth a try? I kinda think we should do that whether or not it can be proven to have anything to do with Andrew's report. It's just like "errno = 0" for Unix --- sometimes you have to do it to be sure of whether a particular function has thrown an error. regards, tom lane
Tom Lane wrote: >> The quick try would be to stick a SetLastError(0) in there, just to be >> sure... Could be worth a try? >> > > I kinda think we should do that whether or not it can be proven to > have anything to do with Andrew's report. It's just like "errno = 0" > for Unix --- sometimes you have to do it to be sure of whether a > particular function has thrown an error. > > > I suspect it has little or nothing to do with it in fact. On my (very lightly loaded) Vista box a crash with exit code 9 seems to result in a consistently problem free restart. I did 200 iterations of the test. Now presumably we sleep for 1 sec between the CloseHandle() call and the CreateFileMapping() call in that code for a reason. We have seen in other cases that Windows can take some time after a call has returned for some operations to actually complete, and I assume we have a similar case here. So, my question is: how do we know that 1 second is enough? Was that a wild guess? I confess I don't have much confidence that just repeating it a few times without increasing the sleep interval will necessarily solve the problem. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Now presumably we sleep for 1 sec between the CloseHandle() call and the
> CreateFileMapping() call in that code for a reason.
I'm not sure. Magnus never did answer my question about why the sleep
and retry was put in at all; it seems not unlikely from here that it
was mere speculation.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Now presumably we sleep for 1 sec between the CloseHandle() call and the >> CreateFileMapping() call in that code for a reason. > > I'm not sure. Magnus never did answer my question about why the sleep > and retry was put in at all; it seems not unlikely from here that it > was mere speculation. It was necessary at the time. The actual 1 second value was completely random - it fixed all the issues on my test VM at the time. I don't recall exactly the details, but I do recall having to run a lot of tests before I managed to provoke an error, and that with the 1 sec thing i could run it for a day of repeated restarts without any errors. //Magnus
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Tom Lane wrote: >>> It says here: >>> http://msdn.microsoft.com/en-us/library/ms885627.aspx > >> FWIW, this is the Windows CE documentation. The one for win32 is at: >> http://msdn.microsoft.com/en-us/library/ms679360(VS.85).aspx > > Sorry, that was the one that came up first in a Google search ... Yeah, it's annoying, but that often happens. One has to be careful to check though - the wince stuff is usually just a subset of the full win32, but sometimes there can be subtle differences. So I recommend always making sure you look at the win32 docs, not wince. >> The quick try would be to stick a SetLastError(0) in there, just to be >> sure... Could be worth a try? > > I kinda think we should do that whether or not it can be proven to > have anything to do with Andrew's report. It's just like "errno = 0" > for Unix --- sometimes you have to do it to be sure of whether a > particular function has thrown an error. Right. Ok, I've applied a patch that does this. Given that it's certainly not in a performance critical path, the overhead shouldn't be noticeable, and it's certainly not wrong to do it :) //Magnus
Magnus Hagander wrote: > Tom Lane wrote: > >> Andrew Dunstan <andrew@dunslane.net> writes: >> >>> Now presumably we sleep for 1 sec between the CloseHandle() call and the >>> CreateFileMapping() call in that code for a reason. >>> >> I'm not sure. Magnus never did answer my question about why the sleep >> and retry was put in at all; it seems not unlikely from here that it >> was mere speculation. >> > > It was necessary at the time. > > The actual 1 second value was completely random - it fixed all the > issues on my test VM at the time. I don't recall exactly the details, > but I do recall having to run a lot of tests before I managed to provoke > an error, and that with the 1 sec thing i could run it for a day of > repeated restarts without any errors. > > > Well, my untested hypothesis is that the actual time required is variable, depending on environmental factors such as machine load. So testing repeatedly where such factors are constant might not be good enough. That's why I suggested some sort of increasing backoff, in an attempt to be adaptive. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Magnus Hagander wrote:
>> The actual 1 second value was completely random - it fixed all the
>> issues on my test VM at the time. I don't recall exactly the details,
>> but I do recall having to run a lot of tests before I managed to provoke
>> an error, and that with the 1 sec thing i could run it for a day of
>> repeated restarts without any errors.
> Well, my untested hypothesis is that the actual time required is
> variable, depending on environmental factors such as machine load.
Seems reasonable.
> So testing repeatedly where such factors are constant might not be good
> enough. That's why I suggested some sort of increasing backoff, in an
> attempt to be adaptive.
I still think there's absolutely no evidence suggesting that a variable
backoff is necessary. Given how little this code is going to be
exercised in the real world, how long will it take till we find out
if you get it wrong? Use a simple retry loop and be done with it.
regards, tom lane
Tom Lane wrote: > I still think there's absolutely no evidence suggesting that a variable > backoff is necessary. Given how little this code is going to be > exercised in the real world, how long will it take till we find out > if you get it wrong? Use a simple retry loop and be done with it. > > > Why should a 1 second delay between CloseHandle() and CreateFileMapping() be enough now when it was not enough 1 second ago? If the event we needed an offset from were outside the loop I'd agree with you. cheers andrew
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Magnus Hagander wrote: >>> The actual 1 second value was completely random - it fixed all the >>> issues on my test VM at the time. I don't recall exactly the details, >>> but I do recall having to run a lot of tests before I managed to provoke >>> an error, and that with the 1 sec thing i could run it for a day of >>> repeated restarts without any errors. > >> Well, my untested hypothesis is that the actual time required is >> variable, depending on environmental factors such as machine load. > > Seems reasonable. > >> So testing repeatedly where such factors are constant might not be good >> enough. That's why I suggested some sort of increasing backoff, in an >> attempt to be adaptive. > > I still think there's absolutely no evidence suggesting that a variable > backoff is necessary. Given how little this code is going to be > exercised in the real world, how long will it take till we find out > if you get it wrong? Use a simple retry loop and be done with it. +1. Let's keep it as simple as possible for now. I doubt it's actually dependent on the *failed* call. Andrew, you want to write up a patch or do you want me to do it? //Magnus
Magnus Hagander wrote: > > Andrew, you want to write up a patch or do you want me to do it? > > > Go for it. cheers andrew
Magnus Hagander wrote: > > Andrew, you want to write up a patch or do you want me to do it? This is going to be backpatched, I assume? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> This is going to be backpatched, I assume?
Yeah, back to 8.2 I suppose.
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> I still think there's absolutely no evidence suggesting that a variable
>> backoff is necessary. Given how little this code is going to be
>> exercised in the real world, how long will it take till we find out
>> if you get it wrong? Use a simple retry loop and be done with it.
> +1. Let's keep it as simple as possible for now. I doubt it's actually
> dependent on the *failed* call.
Exactly. Presumably we're waiting for some system bookkeeping to
finish. Maybe it will take more than 1 second, but we're not going
to be slowing it noticeably by trying once a second.
regards, tom lane
Andrew Dunstan wrote:
>
>
> Magnus Hagander wrote:
>>
>> Andrew, you want to write up a patch or do you want me to do it?
>>
>>
>>
>
> Go for it.
How does this look?
Passes my tests, but I can't really reproduce the requirement to retry,
so I haven't been able to test that part :(
//Magnus
*** a/src/backend/port/win32_shmem.c
--- b/src/backend/port/win32_shmem.c
***************
*** 123,128 **** PGSharedMemoryCreate(Size size, bool makePrivate, int port)
--- 123,129 ----
HANDLE hmap,
hmap2;
char *szShareMem;
+ int i;
/* Room for a header? */
Assert(size > MAXALIGN(sizeof(PGShmemHeader)));
***************
*** 131,184 **** PGSharedMemoryCreate(Size size, bool makePrivate, int port)
UsedShmemSegAddr = NULL;
- /* In case CreateFileMapping() doesn't set the error code to 0 on success */
- SetLastError(0);
-
- hmap = CreateFileMapping((HANDLE) 0xFFFFFFFF, /* 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).",
- (unsigned long) 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
- * sleeping for a second.
- */
- CloseHandle(hmap); /* Close the old handle, since we got a valid
- * one to the previous segment. */
-
- Sleep(1000);
-
/* In case CreateFileMapping() doesn't set the error code to 0 on success */
SetLastError(0);
! hmap = CreateFileMapping((HANDLE) 0xFFFFFFFF, NULL, PAGE_READWRITE, 0L, (DWORD) size, szShareMem);
if (!hmap)
ereport(FATAL,
(errmsg("could not create shared memory segment: %lu", GetLastError()),
errdetail("Failed system call was CreateFileMapping(size=%lu, name=%s).",
(unsigned long) 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.")));
}
free(szShareMem);
/*
--- 132,184 ----
UsedShmemSegAddr = NULL;
/*
! * When recycling a shared memory segment, it may take a short while
! * before it gets dropped from the global namespace. So re-try after
! * sleeping for a second, and continue retrying 10 times.
! * (both the 1 second time and the 10 retries are completely arbitrary)
*/
! for (i = 0; i < 10; i++)
{
/* In case CreateFileMapping() doesn't set the error code to 0 on success */
SetLastError(0);
! hmap = CreateFileMapping((HANDLE) 0xFFFFFFFF, /* 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).",
(unsigned long) size, szShareMem)));
+ /*
+ * If the segment already existed, CreateFileMapping() will return a
+ * handle to the existing one.
+ */
if (GetLastError() == ERROR_ALREADY_EXISTS)
! {
! CloseHandle(hmap); /* Close the old handle, since we got a valid
! * one to the previous segment. */
! Sleep(1000);
! continue;
! }
! break;
}
+ /*
+ * If the last call in the loop still returned ERROR_ALREADY_EXISTS, this shared memory
+ * segment exists and we assume it belongs to somebody else.
+ */
+ 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.")));
+
free(szShareMem);
/*
Magnus Hagander wrote: > How does this look? > > Passes my tests, but I can't really reproduce the requirement to retry, > so I haven't been able to test that part :( I'm disappointed :-( I thought this thread (without reading it too deeply) was about fixing the problem that backends sometimes fail to connect to shmem, on a system that's been running for a while. But I see that it's about recycling the segment after a crash (and/or restart?), so it has no relationship to the other case at all :-( I can't comment on the patch itself. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes: > I'm disappointed :-( I thought this thread (without reading it too > deeply) was about fixing the problem that backends sometimes fail to > connect to shmem, on a system that's been running for a while. Nobody knows yet what's wrong there or how to fix it. This thread is about Andrew's complaint here: http://archives.postgresql.org//pgsql-hackers/2009-05/msg00003.php which seems relatively straightforward to fix, or at least reduce the probability of trouble to the vanishing point. regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
> Passes my tests, but I can't really reproduce the requirement to retry,
> so I haven't been able to test that part :(
The patch looks sane to me. If you want to test, perhaps reducing the
sleep to 1 msec or so would reproduce the need to go around the loop
more than once. (Don't forget to put the machine under additional
load, too.)
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Passes my tests, but I can't really reproduce the requirement to retry, >> so I haven't been able to test that part :( > > The patch looks sane to me. If you want to test, perhaps reducing the > sleep to 1 msec or so would reproduce the need to go around the loop > more than once. (Don't forget to put the machine under additional > load, too.) I've applied this to HEAD and 8.3 so we can get some buildfarm testing on it as well. Andrew, any chance you can get 8.3-tip tested with your client? Or at least in your own reproducable-environment? I didn't backpatch to 8.2, because the code is completely different there. We should probably consider doing it once we know if this fixes the actual issue, but I don't want to spend the effort on backporting it until we know it works. //Magnus