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

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] > 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 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