Re: [HACKERS] windows shared memory error

2009-05-05 Thread Magnus Hagander
Tom Lane wrote:
 Magnus Hagander mag...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Magnus Hagander
Tom Lane wrote:
 Andrew Dunstan and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Magnus Hagander
Tom Lane wrote:
 Magnus Hagander mag...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Andrew Dunstan



Magnus Hagander wrote:

Tom Lane wrote:
  

Andrew Dunstan and...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Tom Lane
Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Andrew Dunstan



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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Magnus Hagander
Tom Lane wrote:
 Andrew Dunstan and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Alvaro Herrera
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 Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes:
 This is going to be backpatched, I assume?

Yeah, back to 8.2 I suppose.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Tom Lane
Magnus Hagander mag...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Andrew Dunstan



Magnus Hagander wrote:


Andrew, you want to write up a patch or do you want me to do it?


  


Go for it.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Magnus Hagander
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) 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).,
- 		   (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) 0x, 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) 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).,
  			   (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);
  
  	/*

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Alvaro Herrera
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 Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Tom Lane
Alvaro Herrera alvhe...@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/msg3.php
which seems relatively straightforward to fix, or at least reduce
the probability of trouble to the vanishing point.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-04 Thread Tom Lane
Magnus Hagander mag...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-03 Thread Magnus Hagander
Tom Lane wrote:
 Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-03 Thread Magnus Hagander
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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-03 Thread Andrew Dunstan



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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-03 Thread Tom Lane
Magnus Hagander mag...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-03 Thread Andrew Dunstan



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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-03 Thread Tom Lane
Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-02 Thread Andrew Dunstan



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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-02 Thread Tom Lane
Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] windows shared memory error

2009-05-01 Thread Andrew Dunstan


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 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);

   hmap = CreateFileMapping((HANDLE) 0x, 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.)));

   }


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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Dave Page
On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Greg Stark
On Fri, May 1, 2009 at 8:42 AM, Dave Page dp...@pgadmin.org wrote:
 On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Dave Page
On Fri, May 1, 2009 at 11:05 AM, Greg Stark st...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Heikki Linnakangas

Dave Page wrote:

On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan and...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Dave Page
On Fri, May 1, 2009 at 4:10 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 Dave Page wrote:

 On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Andrew Dunstan



Heikki Linnakangas wrote:

Dave Page wrote:
On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan and...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Heikki Linnakangas

Dave Page wrote:

On Fri, May 1, 2009 at 4:10 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:

Dave Page wrote:

On Fri, May 1, 2009 at 12:59 AM, Andrew Dunstan and...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Tom Lane
Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Andrew Dunstan



Tom Lane wrote:

Andrew Dunstan and...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Tom Lane
Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Andrew Dunstan



Tom Lane wrote:

Andrew Dunstan and...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] windows shared memory error

2009-05-01 Thread Tom Lane
Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers