Re: Hang in pldebugger after git commit : 98a64d0

On Thu, Dec 15, 2016 at 1:18 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Dec 14, 2016 at 5:35 AM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)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