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

On 1/24/18, 00:00, "Fabien COELHO" <coelho(at)cri(dot)ensmp(dot)fr> wrote:

Hello Doug,

Hello Fabien,

> 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.

Got it.

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.

Fixed.

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

Yes. I should have included this in the description.

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.

"clear_socket" removed. Functionality inlined into "init_socket_array"

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?

Changed "socket_array" to "socket_set"

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

Renamed "init_socket_array" to "clear_socket_set"

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?

Done. Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.

Please do not remove comments, eg:

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

Restored.

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.

Removed unneeded #ifdef around POLLRDHUP

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

Fixed init & passing of timeout.

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.

Removed.

--
Fabien.

Thank you!!
doug