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.

On 1/24/18, 00:00, "Fabien COELHO" <coelho@cri.ensmp.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

ISTM that the v7 patch version you sent is identical to v6.

--
Fabien.

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

On 1/24/18, 00:00, "Fabien COELHO" <coelho@cri.ensmp.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

Hello Doug,

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.

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.

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.

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

--
Fabien.

On 1/25/18, 14:46, "Fabien COELHO" <coelho@cri.ensmp.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

Hello Doug,

Patch applies, compiles, tests ok.

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

USE_SELECT could mean something somewhere. Maybe use something more
specific like PGBENCH_USE_SELECT? Having this macro available simplifies
testing.

I'm not sure why you do the following trick, could you explain?
   +#undef USE_SELECT
   +#define USE_SELECT

In the select implementation you do:

return (socket_set *) pg_malloc0(sizeof(socket_set) * MAXCLIENTS);

but ISTM that socket_set is already an fd_set which represents a set of
clients, so allocating a number of it is needed. The initial
implementation just does "fs_set input_mask", whetever the number of
clients, and it works fine.

--
Fabien.

On 1/26/18, 15:00, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:
Hello Doug,

Hello Fabien,

Patch applies, compiles, tests ok.

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

USE_SELECT could mean something somewhere. Maybe use something more
specific like PGBENCH_USE_SELECT? Having this macro available simplifies
testing.

Changed to PGBENCH_USE_SELECT

    I'm not sure why you do the following trick, could you explain?
       +#undef USE_SELECT
       +#define USE_SELECT

This was due to compiler complaint about USE_SELECT being redefined.
Have replaced that "trick" with a new #define POLL_USING_SELECT which is used elsewhere in pgbench instead.

In the select implementation you do:

return (socket_set *) pg_malloc0(sizeof(socket_set) * MAXCLIENTS);

but ISTM that socket_set is already an fd_set which represents a set of
clients, so allocating a number of it is needed. The initial
implementation just does "fs_set input_mask", whetever the number of
clients, and it works fine.

Ugh. Yes, for socket() only one (1) fd_set is needed.
Fixed.

--
Fabien.

Thank you, again!!
doug

Hello Doug,

I'm not sure why you do the following trick, could you explain?
+#undef USE_SELECT
+#define USE_SELECT

This was due to compiler complaint about USE_SELECT being redefined.
Have replaced that "trick" with a new #define POLL_USING_SELECT which is used elsewhere in pgbench instead.

Ok, why not.

Another option to avoid the warning and a new name could have been to
"#ifndef X #define X #endif /* !X */"

Patch applies cleanly, compiles cleanly for both options, local & global
"make check" ok.

Switched to "Ready".

--
Fabien.

On 2018-01-28 23:02:57 +0000, Rady, Doug wrote:

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 31ea6ca06e..689f15a772 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -44,7 +44,13 @@
#include <signal.h>
#include <time.h>
#include <sys/time.h>
-#ifdef HAVE_SYS_SELECT_H
+#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
+#undef HAVE_PPOLL
+#endif
+#ifdef HAVE_PPOLL
+#include <poll.h>
+#elif defined(HAVE_SYS_SELECT_H)
+#define POLL_USING_SELECT

(random thing noticed while going through patches)

It strikes me as a bad idea to undefine configure selected
symbols. Postgres header might rely on them. It also strikes me as
entirely unnecessary here.

- Andres

-#ifdef HAVE_SYS_SELECT_H
+#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
+#undef HAVE_PPOLL
+#endif
+#ifdef HAVE_PPOLL
+#include <poll.h>
+#elif defined(HAVE_SYS_SELECT_H)
+#define POLL_USING_SELECT

(random thing noticed while going through patches)

It strikes me as a bad idea to undefine configure selected
symbols. Postgres header might rely on them. It also strikes me as
entirely unnecessary here.

Yes, I though about this one but let it pass. Indeed, it would be
sufficient to not load "poll.h" when select is forced, without undefining
the configure setting.

--
Fabien.

On 2018-03-01 11:30:39 +0100, Fabien COELHO wrote:

-#ifdef HAVE_SYS_SELECT_H
+#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
+#undef HAVE_PPOLL
+#endif
+#ifdef HAVE_PPOLL
+#include <poll.h>
+#elif defined(HAVE_SYS_SELECT_H)
+#define POLL_USING_SELECT

(random thing noticed while going through patches)

It strikes me as a bad idea to undefine configure selected
symbols. Postgres header might rely on them. It also strikes me as
entirely unnecessary here.

Yes, I though about this one but let it pass. Indeed, it would be sufficient
to not load "poll.h" when select is forced, without undefining the configure
setting.

I've marked the CF entry waiting on author.

Greetings,

Andres Freund

Updated the patch to not do the #undef

pgbench11-ppoll-v11.patch attached.

Thanks,
doug

On 3/3/18, 16:14, "Andres Freund" <andres@anarazel.de> wrote:

On 2018-03-01 11:30:39 +0100, Fabien COELHO wrote:

-#ifdef HAVE_SYS_SELECT_H
+#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
+#undef HAVE_PPOLL
+#endif
+#ifdef HAVE_PPOLL
+#include <poll.h>
+#elif defined(HAVE_SYS_SELECT_H)
+#define POLL_USING_SELECT

(random thing noticed while going through patches)

It strikes me as a bad idea to undefine configure selected
symbols. Postgres header might rely on them. It also strikes me as
entirely unnecessary here.

Yes, I though about this one but let it pass. Indeed, it would be sufficient
to not load "poll.h" when select is forced, without undefining the configure
setting.

I've marked the CF entry waiting on author.

Greetings,

Andres Freund

Hello Doug,

Updated the patch to not do the #undef
pgbench11-ppoll-v11.patch attached.

Patch applies. Do not forget to regenerate configure to test...

I've compiled and run with both ppoll & select options without issues.

Two quite minor style comment (at least 2 instances):

if (cond) return false; else return true;

ISTM that the simpler the better:

return !cond;

Also ISTM that the following does not comply with pg C style expectations
(idem, 2 instances):

} else {

--
Fabien.

On 3/25/18, 04:00, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:

Hello Fabien,

Hello Doug,

Updated the patch to not do the #undef
pgbench11-ppoll-v11.patch attached.

Patch applies. Do not forget to regenerate configure to test...

I've compiled and run with both ppoll & select options without issues.

Two quite minor style comment (at least 2 instances):

if (cond) return false; else return true;

ISTM that the simpler the better:

return !cond;
Fixed.

Also ISTM that the following does not comply with pg C style expectations
(idem, 2 instances):

} else {
Fixed.

Also fixed issue with 'timeout' not being passed as NULL when no timeout time.

--
Fabien.

Thanks!
doug

Hello Doug,

I've compiled and run with both ppoll & select options without issues.
Two quite minor style comment (at least 2 instances):

Fixed. Fixed. Also fixed issue with 'timeout' not being passed as NULL
when no timeout time.

v12 compiled and tested with both ppoll & select (by forcing). All seems
ok to me.

Switched back to "ready".

--
Fabien.

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

Andres Freund <andres@anarazel.de> writes:

I'm still not particularly happy with this.

I'm a bit confused as to what the point is. It seems unlikely that one
pgbench process can effectively drive enough backends for select's
limitations to really be an issue.

regards, tom lane

On 2018-04-06 17:49:19 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I'm still not particularly happy with this.

I'm a bit confused as to what the point is. It seems unlikely that one
pgbench process can effectively drive enough backends for select's
limitations to really be an issue.

It's not that crazy to use more than 1024 connections (common FD_SETSIZE
valu), especially over a higher latency connection.

As I wrote, I think using a poll API that doesn't require looping over
all sockets, even if they didn't get an event, would be a better plan.

- Andres