Re: PATCH: pgbench - option to build using ppoll() for largerconnection 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