On Thu, Dec 15, 2016 at 1:18 AM, Robert Haas <robertmh...@gmail.com> wrote: > On Wed, Dec 14, 2016 at 5:35 AM, Ashutosh Sharma <ashu.coe...@gmail.com> > wrote: >>> I have just read the patch, and hardcoding the array position for a >>> socket event to 0 assumes that the socket event will be *always* the >>> first one in the list. but that cannot be true all the time, any code >>> that does not register the socket event as the first one in the list >>> will likely break. >> >> I think your point is very valid and even i had similar thought in my >> mind when implementing it but as i mentioned upthread that's how it is >> being used in the current code base. Please check a function call to >> ModifyWaitEvent() in secure_read(). > > That's kind of irrelevant. A WaitEventSet is a general-purpose > primitive, and it needs to work in all situations, current and future, > where a reasonable developer would expect it to work. Sure, pq_init() > puts the socket in FeBeWaitSet at position 0. However, in > WaitLatchOrSocket, the socket event, if required, is added after the > latch and postmaster-death events. And new code written using > CreateWaitEventSet() / AddWaitEventToSet() in the future could put a > socket at any offset in the event array, or could add multiple > sockets. So Michael is right: you can't just stick a hack into > WaitEventSetWait that assumes the socket is at offset 0 even if that > happens to fix the problem we are facing right at this moment. > > (I am not sure whether Michael's proposed placement is correct, but > I'm almost 100% sure the placement in the submitted patch is not.)
What would seem like the good place is really WaitEventAdjustWin32() that gets called in ModifyWaitEvent() by secure_read(). And WaitEventAdjustWin32() is the place where WSAEventSelect() gets called to adjust the flags FD_READ and FD_WRITE depending on the events WL_SOCKET_READABLE or WL_SOCKET_WRITEABLE. Amit, I have a question here. As far as I can read, secure_read() does set WL_SOCKET_READABLE as an event to wait for, so FD_READ is correctly being set by WSAEventSelect(). What the patch proposed is doing is making sure that *only* WL_SOCKET_READABLE is set in the event flags. Why is that necessary? I can buy that this addresses the problem as this has been visibly tested, but I am unclear about why that's the correct thing to do. The patch clearly needs comments to clarify its position. Actually, I think that what is surprising in the solution proposed is actually that the event flags are reset *after* the while loop in WaitEventSetWait(). I could understand something happening inside the loop if WSAEnumNetworkEvents() updates things on the fly though... -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
