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

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

Hello Doug,

Hello Fabien,

> This time with the revised patch file: pgbench11-ppoll-v8.patch

Patch applies cleanly. Compiles cleanly and runs fine in both ppoll &
select cases.

I'm okay with having a preferred ppoll implementation because of its improved
capability.

A few minor additional comments/suggestions:

Cpp has an #elif that could be used to manage the ppoll/select alternative.
It is already used elsewhere in the file. Or not.

I must admit that I'm not fond of the alloc_socket_set trick with MAXCLIENTS,
especially without any comment. I'd suggest to just have two distinct functions
in their corresponding sections.

Made a distinct function for each section.

I would add a comment that free_socket_set code is common to both
versions, and maybe consider moving it afterwards. Or maybe just duplicate
if in each section for homogeneity.

Duplicated.

It looks like error_on_socket and ignore_socket should return a boolean instead
of an int. Also, maybe simplify the implementation of the later by avoiding
the ?: expression.

ISTM that the error_on_socket function in the ppoll case deserves some
comments, especially on the condition.

Added comment. Changed output to be more compatible with socket() error check.
Extra ppoll() specific error info only output when debug flag set ... not sure if this is OK or should just remove the extra info.


> [...] Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.

I'm okay with that. I'm wondering whether there should be a way to force
using one or the other when both are available. Not sure.

Added option to force use of select(2) via: -DUSE_SELECT

--
Fabien.

Thank you!!
doug