Re: [PATCHES] [HACKERS] Possible explanation for Win32 stats regression test

[[EMAIL PROTECTED]]

Is anyone working on this?

Tom Lane wrote:
 korry [EMAIL PROTECTED] writes:
  The problem is that, each time you go through
  pgwin32_waitforsinglesocket(), you tie the *same* kernel object
  (waitevent is static) to each socket.
 
  The fix is pretty simple - just call WSAEventSelect( s, waitevent, 0 )
  after WaitForMultipleObjectsEx() returns.  That disassociates the socket
  from the Event (it will get re-associated the next time
  pgwin32_waitforsingleselect() is called.  
 
 Hmm.  Presumably we don't do this a whole lot (use multiple sockets) or
 we'd have noticed before.  Perhaps better would be to keep an additional
 static variable saying which socket the event is currently associated
 to, and only issue the extra WSAEventSelect calls if we need to change
 it.  Or is WSAEventSelect fast enough that it doesn't matter?
 



Here's a simple patch that fixes the problem (I haven't explored the performance of this patch compared to Tom's suggestion).

 -- Korry


Index: src/backend/port/win32/socket.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/socket.c,v
retrieving revision 1.11
diff -w -c -r1.11 socket.c
*** src/backend/port/win32/socket.c	5 Mar 2006 15:58:35 -	1.11
--- src/backend/port/win32/socket.c	29 Jul 2006 12:13:19 -
***
*** 132,137 
--- 132,154 
  	events[1] = waitevent;
  	r = WaitForMultipleObjectsEx(2, events, FALSE, INFINITE, TRUE);
  
+ 
+ 	/*
+ 	 * NOTE: we must disassociate this socket from waitevent - if we don't, then 
+ 	 *   we may accidentally fire waitevent at some point in the future if,
+ 	 *		 for example, the socket is closed.  That normally would not be a 
+ 	 *		 problem, but if you ever have two (or more) sockets in a single 
+ 	 *		 backend, they *ALL* share the same waitevent. So, if you pass through
+ 	 *		 this function for socket1 and socket2, a close on EITHER socket will
+ 	 *		 trigger an FD_CLOSE event, regardless of whether you're waiting for
+ 	 *		 socket1 or socket2.  That means that if you are waiting for socket1
+ 	 *		 and socket2 gets some interesting traffic (an FD_CLOSE or FD_READ
+ 	 *		 event for example), the above call to WaitForMultipleObjectsEx() 
+ 	 *		 will return even though nothing actually happened to socket1. Nasty...
+ 	 */
+ 
+ 	WSAEventSelect(s, waitevent, 0 );
+ 
  	if (r == WAIT_OBJECT_0 || r == WAIT_IO_COMPLETION)
  	{
  		pgwin32_dispatch_queued_signals();

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly

Re: [PATCHES] [HACKERS] Possible explanation for Win32 stats regression

[Andrew Dunstan]

heh. I was just doing it the way Tom suggested - see attached. With a 
little more trouble we could also keep track if the listened for events 
and sometimes save ourselves a second call to WSAEventSelect, but I'm 
not sure it's worth it.


cheers

andrew


[EMAIL PROTECTED] wrote:

Is anyone working on this?

Tom Lane wrote:
 korry [EMAIL PROTECTED] mailto:[EMAIL PROTECTED] writes:
  The problem is that, each time you go through
  pgwin32_waitforsinglesocket(), you tie the *same* kernel object
  (waitevent is static) to each socket.
 
  The fix is pretty simple - just call WSAEventSelect( s, waitevent, 0 )

  after WaitForMultipleObjectsEx() returns.  That disassociates the socket
  from the Event (it will get re-associated the next time
  pgwin32_waitforsingleselect() is called.  
 
 Hmm.  Presumably we don't do this a whole lot (use multiple sockets) or

 we'd have noticed before.  Perhaps better would be to keep an additional
 static variable saying which socket the event is currently associated
 to, and only issue the extra WSAEventSelect calls if we need to change
 it.  Or is WSAEventSelect fast enough that it doesn't matter?
 



Here's a simple patch that fixes the problem (I haven't explored the 
performance of this patch compared to Tom's suggestion).





Index: backend/port/win32/socket.c
===
RCS file: /cvsroot/pgsql/src/backend/port/win32/socket.c,v
retrieving revision 1.11
diff -c -r1.11 socket.c
*** backend/port/win32/socket.c	5 Mar 2006 15:58:35 -	1.11
--- backend/port/win32/socket.c	29 Jul 2006 14:19:23 -
***
*** 106,111 
--- 106,112 
  pgwin32_waitforsinglesocket(SOCKET s, int what)
  {
  	static HANDLE waitevent = INVALID_HANDLE_VALUE;
+ 	static SOCKET current_socket = -1;
  	HANDLE		events[2];
  	int			r;
  
***
*** 121,126 
--- 122,136 
  		ereport(ERROR,
  (errmsg_internal(Failed to reset socket waiting event: %i, (int) GetLastError(;
  
+ 	/*
+ 	 * make sure we don't multiplex this with a different socket 
+ 	 * from a previous call
+ 	 */
+ 
+ 	if (current_socket != s  current_socket != -1)
+ 		WSAEventSelect(current_socket, waitevent, 0);
+ 
+ 	current_socket = s;
  
  	if (WSAEventSelect(s, waitevent, what) == SOCKET_ERROR)
  	{

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [PATCHES] [HACKERS] Possible explanation for Win32 stats regression

[Andrew Dunstan]

[EMAIL PROTECTED] wrote:
heh. I was just doing it the way Tom suggested - see attached. With a 
little more trouble we could also keep track if the listened for events 
and sometimes save ourselves a second call to WSAEventSelect, but I'm 
not sure it's worth it.



It all depends on the overhead of WSAEventSelect().  I'm sure your version would run 
faster, but I just don't know if slower would be measurable.
  


BTW:  I would suggest changing your comment to:

   /*
* make sure we don't multiplex this kernel event object with a 
different socket

* from a previous call
*/

Thanks for tackling this problem too.



Ok. Applied to HEAD and 8.1 and 8.0 branches.

cheers

andrew

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
  subscribe-nomail command to [EMAIL PROTECTED] so that your
  message can get through to the mailing list cleanly