From: Rady, Doug <radydoug@amazon.com>
To: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: Re: PATCH: pgbench - option to build using ppoll() for larger connection counts
Date: Fri, 26 Jan 2018 22:27:47 +0000
Message-ID: <81936381-841A-459D-9FE5-18659680355C@amazon.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1801252339520.26762@lancre>
References: <23D017C9-81B7-484D-8490-FD94DEC4DF59@amazon.com>
<alpine.DEB.2.20.1801240847010.2307@lancre>
<16744C85-E44E-4354-9E3D-45A7C9BDB19B@amazon.com>
<alpine.DEB.2.20.1801252339520.26762@lancre>
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
Attachments:
[application/octet-stream] pgbench11-ppoll-v9.patch (10.3K, ../81936381-841A-459D-9FE5-18659680355C@amazon.com/2-pgbench11-ppoll-v9.patch)
download | inline diff:diff --git a/configure.in b/configure.inindex 4d26034579..7b2b531675 100644--- a/configure.in+++ b/configure.in@@ -1425,7 +1425,7 @@ PGAC_FUNC_WCSTOMBS_L
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
-AC_CHECK_FUNCS([cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate pstat pthread_is_threaded_np readlink setproctitle setsid shm_open symlink sync_file_range utime utimes wcstombs_l])+AC_CHECK_FUNCS([cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setsid shm_open symlink sync_file_range utime utimes wcstombs_l])
AC_REPLACE_FUNCS(fseeko)
case $host_os in
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.cindex 31ea6ca06e..2ec8d09561 100644--- a/src/bin/pgbench/pgbench.c+++ b/src/bin/pgbench/pgbench.c@@ -44,7 +44,14 @@
#include <signal.h>
#include <time.h>
#include <sys/time.h>
-#ifdef HAVE_SYS_SELECT_H+#ifdef USE_SELECT /* force use of select(2)? */+#undef HAVE_PPOLL+#endif+#ifdef HAVE_PPOLL+#include <poll.h>+#elif defined(HAVE_SYS_SELECT_H)+#undef USE_SELECT+#define USE_SELECT
#include <sys/select.h>
#endif
@@ -82,13 +89,19 @@ static int pthread_join(pthread_t th, void **thread_return);
/********************************************************************
* some configurable parameters */
--/* max number of clients allowed */+#ifdef USE_SELECT /* using select(2) */+#define SOCKET_WAIT_METHOD "select"+typedef fd_set socket_set;
#ifdef FD_SETSIZE
-#define MAXCLIENTS (FD_SETSIZE - 10)+#define MAXCLIENTS (FD_SETSIZE - 10) /* system limited max number of clients allowed */
#else
-#define MAXCLIENTS 1024+#define MAXCLIENTS 1024 /* max number of clients allowed */
#endif
+#else /* using ppoll(2) */+#define SOCKET_WAIT_METHOD "ppoll"+typedef struct pollfd socket_set;+#define MAXCLIENTS -1 /* unlimited number of clients */+#endif /* USE_SELECT */
#define DEFAULT_INIT_STEPS "dtgvp" /* default -I setting */
@@ -502,6 +515,13 @@ static void pgbench_error(const char *fmt,...) pg_attribute_printf(1, 2);
static void addScript(ParsedScript script);
static void *threadRun(void *arg);
static void setalarm(int seconds);
+static socket_set *alloc_socket_set(int count);+static bool error_on_socket(socket_set *sa, int idx, PGconn *con);+static void free_socket_set(socket_set *sa);+static bool ignore_socket(socket_set *sa, int idx, PGconn *con);+static void clear_socket_set(socket_set *sa, int count);+static void set_socket(socket_set *sa, int fd, int idx);+static int wait_on_socket_set(socket_set *sa, int nstate, int maxsock, int64 usec);
/* callback functions for our flex lexer */
@@ -1071,6 +1091,7 @@ doConnect(void)
!have_password)
{
PQfinish(conn);
+ conn = NULL;
simple_prompt("Password: ", password, sizeof(password), false);
have_password = true;
new_pass = true;
@@ -2704,6 +2725,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
if (INSTR_TIME_IS_ZERO(now))
INSTR_TIME_SET_CURRENT(now);
start = now;
+
if ((st->con = doConnect()) == NULL)
{
fprintf(stderr, "client %d aborted while establishing connection\n",
@@ -4507,7 +4529,7 @@ main(int argc, char **argv)
case 'c':
benchmarking_option_set = true;
nclients = atoi(optarg);
- if (nclients <= 0 || nclients > MAXCLIENTS)+ if (nclients <= 0 || (MAXCLIENTS != -1 && nclients > MAXCLIENTS))
{
fprintf(stderr, "invalid number of clients: \"%s\"\n",
optarg);
@@ -5189,6 +5211,7 @@ threadRun(void *arg)
int64 next_report = last_report + (int64) progress * 1000000;
StatsData last,
aggs;
+ socket_set *sockets = alloc_socket_set(nstate);
/*
* Initialize throttling rate target for all of the thread's clients. It
@@ -5232,6 +5255,7 @@ threadRun(void *arg)
{
if ((state[i].con = doConnect()) == NULL)
goto done;
+ set_socket(sockets, PQsocket(state[i].con), i);
}
}
@@ -5248,13 +5272,13 @@ threadRun(void *arg)
/* loop till all clients have terminated */
while (remains > 0)
{
- fd_set input_mask;
int maxsock; /* max socket number to be waited for */
int64 min_usec;
int64 now_usec = 0; /* set this only if needed */
/* identify which client sockets should be checked for input */
- FD_ZERO(&input_mask);+ clear_socket_set(sockets, nstate);+
maxsock = -1;
min_usec = PG_INT64_MAX;
for (i = 0; i < nstate; i++)
@@ -5304,7 +5328,7 @@ threadRun(void *arg)
goto done;
}
- FD_SET(sock, &input_mask);+ set_socket(sockets, sock, i);
if (maxsock < sock)
maxsock = sock;
}
@@ -5341,7 +5365,7 @@ threadRun(void *arg)
/*
* If no clients are ready to execute actions, sleep until we receive
* data from the server, or a nap-time specified in the script ends,
- * or it's time to print a progress report. Update input_mask to show+ * or it's time to print a progress report. Update sockets to show
* which client(s) received data.
*/
if (min_usec > 0)
@@ -5352,11 +5376,7 @@ threadRun(void *arg)
{
if (maxsock != -1)
{
- struct timeval timeout;-- timeout.tv_sec = min_usec / 1000000;- timeout.tv_usec = min_usec % 1000000;- nsocks = select(maxsock + 1, &input_mask, NULL, NULL, &timeout);+ nsocks = wait_on_socket_set(sockets, nstate, maxsock, min_usec);
}
else /* nothing active, simple sleep */
{
@@ -5365,7 +5385,7 @@ threadRun(void *arg)
}
else /* no explicit delay, select without timeout */
{
- nsocks = select(maxsock + 1, &input_mask, NULL, NULL, NULL);+ nsocks = wait_on_socket_set(sockets, nstate, maxsock, 0);
}
if (nsocks < 0)
@@ -5376,7 +5396,7 @@ threadRun(void *arg)
continue;
}
/* must be something wrong */
- fprintf(stderr, "select() failed: %s\n", strerror(errno));+ fprintf(stderr, "%s() failed: %s\n", SOCKET_WAIT_METHOD, strerror(errno));
goto done;
}
}
@@ -5385,7 +5405,7 @@ threadRun(void *arg)
/* min_usec == 0, i.e. something needs to be executed */
/* If we didn't call select(), don't try to read any data */
- FD_ZERO(&input_mask);+ clear_socket_set(sockets, nstate);
}
/* ok, advance the state machine of each connection */
@@ -5396,16 +5416,11 @@ threadRun(void *arg)
if (st->state == CSTATE_WAIT_RESULT)
{
/* don't call doCustom unless data is available */
- int sock = PQsocket(st->con);- if (sock < 0)- {- fprintf(stderr, "invalid socket: %s",- PQerrorMessage(st->con));+ if (error_on_socket(sockets, i, st->con))
goto done;
- }- if (!FD_ISSET(sock, &input_mask))+ if (ignore_socket(sockets, i, st->con))
continue;
}
else if (st->state == CSTATE_FINISHED ||
@@ -5543,9 +5558,143 @@ done:
fclose(thread->logfile);
thread->logfile = NULL;
}
+ free_socket_set(sockets);+ sockets = NULL;
return NULL;
}
+#ifdef USE_SELECT /* select(2) based socket polling */+static socket_set *+alloc_socket_set(int count)+{+ return (socket_set *) pg_malloc0(sizeof(socket_set) * MAXCLIENTS);+}++static void+free_socket_set(socket_set *sa)+{+ pg_free(sa);+}++static bool+error_on_socket(socket_set *sa, int idx, PGconn *con)+{+ if (PQsocket(con) >= 0) return false;+ fprintf(stderr, "invalid socket: %s", PQerrorMessage(con));+ return true;+}++static bool+ignore_socket(socket_set *sa, int idx, PGconn *con)+{+ if (FD_ISSET(PQsocket(con), sa))+ return false;+ else+ return true;+}++static void+clear_socket_set(socket_set *sa, int count)+{+ FD_ZERO(sa);+}++static void+set_socket(socket_set *sa, int fd, int idx)+{+ FD_SET(fd, sa);+}++static int+wait_on_socket_set(socket_set *sa, int nstate, int maxsock, int64 usec)+{+ struct timeval timeout;++ if (usec)+ {+ timeout.tv_sec = usec / 1000000;+ timeout.tv_usec = usec % 1000000;+ } else {+ timeout.tv_sec = timeout.tv_usec = 0;+ }++ return select(maxsock + 1, sa, NULL, NULL, &timeout);+}+#else /* ppoll(2) based socket polling */+/* ppoll() will block until timeout or one of POLL_EVENTS occurs. */+#define POLL_EVENTS (POLLRDHUP|POLLIN|POLLPRI)+/* ppoll() events returned that we do not want/expect to see. */+#define POLL_UNWANTED (POLLRDHUP|POLLERR|POLLHUP|POLLNVAL)++static socket_set *+alloc_socket_set(int count)+{+ return (socket_set *) pg_malloc0(sizeof(socket_set) * count);+}++static void+free_socket_set(socket_set *sa)+{+ pg_free(sa);+}++static bool+error_on_socket(socket_set *sa, int idx, PGconn *con)+{+ /*+ * No error if socket not used or non-error status from PQsocket() and none+ * of the unwanted ppoll() return events.+ */+ if (sa[idx].fd == -1 || (PQsocket(con) >= 0 && !(sa[idx].revents & POLL_UNWANTED)))+ return false;+ fprintf(stderr, "invalid socket: %s", PQerrorMessage(con));+ if (debug)+ fprintf(stderr, "ppoll() fail - errno: %d, socket: %d, events: %x\n",+ errno, sa[idx].fd, (sa[idx].revents & POLL_UNWANTED));+ return true;+}++static bool+ignore_socket(socket_set *sa, int idx, PGconn *con)+{+ if (sa[idx].fd != -1 && !sa[idx].revents)+ return true;+ else+ return false;+}++static void+clear_socket_set(socket_set *sa, int count)+{+ int i = 0;+ for (i = 0; i < count; i++)+ set_socket(sa, -1, i);+}++static void+set_socket(socket_set *sa, int fd, int idx)+{+ sa[idx].fd = fd;+ sa[idx].events = POLL_EVENTS;+ sa[idx].revents = 0;+}++static int+wait_on_socket_set(socket_set *sa, int nstate, int maxsock, int64 usec)+{+ struct timespec timeout;++ if (usec)+ {+ timeout.tv_sec = usec / 1000000;+ timeout.tv_nsec = usec % 1000000000;+ } else {+ timeout.tv_sec = timeout.tv_nsec = 0;+ }+ return ppoll(sa, nstate, &timeout, NULL);+}+#endif /* USE_SELECT */+
/*
* Support for duration option: set timer_exceeded after so many seconds.
*/
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.inindex f98f773ff0..63207e13fd 100644--- a/src/include/pg_config.h.in+++ b/src/include/pg_config.h.in@@ -409,6 +409,9 @@
/* Define to 1 if the assembler supports PPC's LWARX mutex hint bit. */
#undef HAVE_PPC_LWARX_MUTEX_HINT
+/* Define to 1 if you have the `ppoll' function. */+#undef HAVE_PPOLL+
/* Define to 1 if you have the `pstat' function. */
#undef HAVE_PSTAT
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: radydoug@amazon.com, coelho@cri.ensmp.fr, robertmhaas@gmail.com
Subject: Re: PATCH: pgbench - option to build using ppoll() for larger connection counts
In-Reply-To: <81936381-841A-459D-9FE5-18659680355C@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox