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