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

Hello,

> This patch enables building pgbench to use ppoll() instead of select()
> to allow for more than (FD_SETSIZE - 10) connections. As implemented,
> when using ppoll(), the only connection limitation is system resources.

I'm fine with allowing more clients through ppoll, as large
multi/many/whatever-core hardware becomes more common and is expected to
grow.

However, I'm at odds with the how and the ppoll/select compatibility layer
offered by the patch, and the implied maintenance cost just to understand
what is happening while reading the code cluttered with possibly empty
macros and ifdef stuff. There are many ifdef, many strange/astute macros,
significant dissymetry in the code, which reduce maintainability
noticeably.

I think that it should abstract cleanly the two implementations into a set
of functions with the same signatures, even if some arguments are unsused,
and use these functions without ifdef stuff later. Maybe some macros too,
ok, but not too much.

"SCKTWTMTHD"... WTH? I do not want to know what it means. I'm sure a
better name (if something without vowels can be a name of anything in
English...) can be found. Names must be chosen with great care.

MAXCLIENTS: could be set to -1 when there is no limit, and tested to avoid
one ifdef.

I must admit that I do not see the benefit of "{ do { ... } while(0); }"
compared to "{ ... }". Maybe it has to do with compiler warnings.

> The patch has been implemented to introduce a minimal of #ifdef/#ifndef
> clutter in the code.

I think that it could be much more minimal than that. I would aim at
having just one.

#ifdef USE_PPOLL
specific functions definitions
some macros
#else /* SELECT */
idem for select
#endif

Ok, it may not be the best solution everywhere, but it should be
significantly reduce compared to the current state.

ISTM that ifdef which breaks the code structure should be avoided, eg

#ifdef ...
if (...)
#else
if (...)
#endif
{
// condition was true...

It is unclear to me why the input_mask is declared in the threadRun
function (so is thread specific) but pfds is in each thread struct (so is
also thread specific, but differently). The same logic should be used for
both, which ever is best. Not sure that "pfds" is the right name. If the
two variables means the same thing, they should have the same name,
although possibly different types.

> $ pgbench -j 3000 -c 1500

Probably you intended 1500 threads for 3000 clients.

--
Fabien.