Hi,
I'm still not particularly happy with this. Checking whether I can
polish it up.
a) the new function names are partially non-descriptive and their
meaning is undocumented. As an extreme example:
- if (!FD_ISSET(sock, &input_mask))
+ if (ignore_socket(sockets, i, st->con))
continue;
reading the new code it's entirely unclear what that could mean. Are
you marking the socket as ignored? What does ignored even mean?
There's not a single comment on what the new functions mean. It's not
that bad if there's no docs on what FD_ISSET implies, because that's a
well known and documented interface. But introducing an abstraction
without any comments on it?
b) Does this actually improve the situation all that much? We still loop
over every socket:
/* ok, advance the state machine of each connection */
for (i = 0; i < nstate; i++)
{
CState *st = &state[i];
if (st->state == CSTATE_WAIT_RESULT)
{
/* don't call doCustom unless data is available
*/
if (error_on_socket(sockets, i, st->con))
goto done;
if (ignore_socket(sockets, i, st->con))
continue;
}
else if (st->state == CSTATE_FINISHED ||
st->state == CSTATE_ABORTED)
{
/* this client is done, no need to consider it
anymore */
continue;
}
doCustom(thread, st, &aggs);
/* If doCustom changed client to finished state, reduce
remains */
if (st->state == CSTATE_FINISHED || st->state ==
CSTATE_ABORTED)
remains--;
}
if the goal is to make this more scalable, wouldn't this require
using a proper polling mechanism that supports signalling the
sockets with relevant changes, rather than busy-looping through every
socket if there's just a single change?
I kinda wonder if the proper fix wouldn't be to have one patch making
WaitEventSets usable from frontend code, and then make this code use
them. Not a small project though.
Greetings,
Andres Freund
