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

Hello Doug,

> This version of the patch attempts to address the feedback for the
> previous submission on 28-Nov-2017

Please avoid recreating a thread, but rather respond to the previous one,
I missed this post.

The overall function-based implementation with limited ifdefs seems
readable and maintainable. I think that it rather improves the code in
places by hiding details.

Patch applies with one warning:

pgbench11-ppoll-v6.patch:141: trailing whitespace.
set_socket(sockets, PQsocket(state[i].con), i);<SPACE>
warning: 1 line adds whitespace errors.

The patch implies to run "autoconf" to regenerate the configure script.

Compilation with "ppoll" ok, globale & local make check ok. I do not have
hardware which allows to run with over 1024 clients, so I cannot say that
I tested the case.

Compilation without "ppoll" gets a warning:

pgbench.c:5645:1: warning: ‘clear_socket’ defined but not used...
clear_socket(socket_array *sa, int fd, int idx)

The "clear_socket" function is not used in this case. I'd suggest to
remove it from the prototypes, remove or comment the unused implementation
in the select section, and keep the one in the ppoll section. Or use it if
it is needed. Or inline it in the "init_socket_array" function where it is
used just once.

I'm not sure of the name "socket_array", because it is an array only in
the ppoll case. Maybe "sockets_t" or "socket_set"? Or something else?

Maybe "init_socket_array" should be named "clear_...".

I would suggest not to include sys/select.h when using ppoll, as it is a useless
include this case. I.e. move includes in the ifdef USE_PPOLL section?

Please do not remove comments, eg:

- /* identify which client sockets should be checked for input */

On #endif or #else, especially large scope ones, I would have a comment to
say about what it is, eg at the minimum:
#else /* select(2) implementation */
#endif /* USE_PPOLL */

On:
+#if defined(USE_PPOLL)
+#ifdef POLLRDHUP

Use just one "ifdef" style?

There should be a comment about what this sections are providing, eg:
/* ppoll(2) implementation for "socket_array" functions */

There should be an empty line and possibly a comment explaining why
POLLRDHUP may not be there and/or why this define is necessary.

With select you always initialize the timeout, but not with ppoll.
Use a common approach in the implementations?

The "finishCon" function addition seems totally unrelated to this patch.
Although I agree that this function improves the code, it is refactoring
and does not really belong here.

--
Fabien.