Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

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