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

Is anyone working on this?

Tom Lane wrote:
> korry <korry@appx.com> 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

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


korryd@enterprisedb.com wrote:
>> Is anyone working on this?
>>
>> Tom Lane wrote:
>> > korry <korry@appx.com <mailto:korry@appx.com>> 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 -0000    1.11
--- backend/port/win32/socket.c    29 Jul 2006 14:19:23 -0000
***************
*** 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)
      {

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.

    -- Korry

----   Korry Douglas    korryd@enterprisedb.com   EnterpriseDB      http://www.enterprisedb.com

korryd@enterprisedb.com 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