Hello David,
This patch adds the concept of "multiconnect" to pgbench (better
terminology welcome).
Good. I was thinking of adding such capability, possibly for handling
connection errors and reconnecting…
The basic idea here is to allow connections made with pgbench to use
different auth values or connect to multiple databases. We implement
this using a user-provided PGSERVICEFILE and choosing a PGSERVICE from
this based on a number of strategies. (Currently the only supported
strategies are round robin or random.)
I was thinking of providing a allowing a list of conninfo strings with
repeated options, eg --conninfo "foo" --conninfo "bla"…
Your approach using PGSERVICEFILE also make sense!
Maybe it could be simplified, the code base reduced, and provide more
benefits, by mixing both ideas.
In particular, pgbench parses the file but then it will be read also by
libpq, yuk yuk.
Also, I do not like that PGSERVICE is overriden by pgbench, while other
options are passed with the parameters approach in doConnect. It would
make proce sense to add a "service" field to the parameters for
consistency, if this approach was to be pursued.
On reflexion, I'd suggest to use the --conninfo (or some other name)
approach, eg "pgbench --conninfo='service=s1' --conninfo='service=s2'" and
users just have to set PGSERVICEFILE env themselves, which I think is
better than pgbench overriding env variables behind their back.
This allow to have a service file with more connections and just tell
pgbench which ones to use, which is the expected way to use this feature.
This drops file parsing.
I can only see benefit to this simplified approach.
What do you think?
About the patch:
There are warnings about trailing whitespaces when applying the patch, and
there are some tabbing issues in the file.
I would not consume "-g" option unless there is some logical link with the
feature. I'd be okay with "-m" if it is still needed. I would suggest to
use it for the choice strategy?
stringinfo: We already have PQExpBuffer imported, could we use that
instead? Having two set of struct/functions which do the same in the same
source file does not look like a good idea. If we do not parse the file,
nothing is needed, which is a relief.
Attached is my work-in-progress start at adding conninfo, that would need
to be improved with strategies.
--
Fabien.
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 0c60077e1f..d1390e83e5 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -840,6 +840,16 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
<variablelist>
+ <varlistentry>
+ <term><option>--conninfo=</option><replaceable>conninfo</replaceable></term>
+ <listitem>
+ <para>
+ Add a <replaceable>conninfo</replaceable> connection information string
+ to the pool of possible connection strings.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-h</option> <replaceable>hostname</replaceable></term>
<term><option>--host=</option><replaceable>hostname</replaceable></term>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4aeccd93af..ad71a568db 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -276,13 +276,36 @@ bool is_connect; /* establish connection for each transaction */
bool report_per_command; /* report per-command latencies */
int main_pid; /* main process id used in log filename */
+char *logfile_prefix = NULL;
+
+/* main connection definition */
const char *pghost = NULL;
const char *pgport = NULL;
const char *username = NULL;
const char *dbName = NULL;
-char *logfile_prefix = NULL;
const char *progname;
+/* connection info list */
+typedef struct conninfo_t
+{
+ const char *conninfo;
+ int errors;
+} conninfo_t;
+
+#define MAX_CONNINFO 8
+int n_conninfo = 0;
+conninfo_t conninfos[MAX_CONNINFO];
+
+static void
+push_conninfo(const char *ci)
+{
+ // FIXME nicer error
+ Assert(n_conninfo < MAX_CONNINFO);
+ conninfos[n_conninfo].conninfo = ci;
+ conninfos[n_conninfo].errors = 0;
+ n_conninfo++;
+}
+
#define WSEP '@' /* weight separator */
volatile bool timer_exceeded = false; /* flag from signal handler */
@@ -751,6 +774,7 @@ usage(void)
" -U, --username=USERNAME connect as specified database user\n"
" -V, --version output version information, then exit\n"
" -?, --help show this help, then exit\n"
+ " --conninfo=CONNINFO add a database server conninfo\n"
"\n"
"Report bugs to <%s>.\n"
"%s home page: <%s>\n",
@@ -1343,9 +1367,44 @@ tryExecuteStatement(PGconn *con, const char *sql)
PQclear(res);
}
+/* set up a connection to the backend with a provided conninfo */
+static PGconn *
+doConnectCI(bool change)
+{
+ static int nci = 0;
+ PGconn *conn;
+ conninfo_t *ci = &conninfos[nci];
+
+ conn = PQconnectdb(ci->conninfo);
+
+ if (!conn)
+ {
+ ci->errors++;
+ pg_log_error("connection to database \"%s\" failed", ci->conninfo);
+ /* try another one next time */
+ nci = (nci + 1) % n_conninfo;
+ return NULL;
+ }
+
+ if (PQstatus(conn) == CONNECTION_BAD)
+ {
+ ci->errors++;
+ pg_log_error("%s", PQerrorMessage(conn));
+ PQfinish(conn);
+ /* try another one next time */
+ nci = (nci + 1) % n_conninfo;
+ return NULL;
+ }
+
+ if (change)
+ nci = (nci + 1) % n_conninfo;
+
+ return conn;
+}
+
/* set up a connection to the backend */
static PGconn *
-doConnect(void)
+doConnectParams(void)
{
PGconn *conn;
bool new_pass;
@@ -1408,6 +1467,13 @@ doConnect(void)
return conn;
}
+/* create a connection with one of the above methods */
+static PGconn *
+doConnect(bool change)
+{
+ return n_conninfo > 0 ? doConnectCI(change) : doConnectParams();
+}
+
/* qsort comparator for Variable array */
static int
compareVariableNames(const void *v1, const void *v2)
@@ -3172,7 +3238,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
{
pg_time_usec_t start = now;
- if ((st->con = doConnect()) == NULL)
+ if ((st->con = doConnect(true)) == NULL)
{
pg_log_error("client %d aborted while establishing connection", st->id);
st->state = CSTATE_ABORTED;
@@ -4407,7 +4473,7 @@ runInitSteps(const char *initialize_steps)
initPQExpBuffer(&stats);
- if ((con = doConnect()) == NULL)
+ if ((con = doConnect(false)) == NULL)
exit(1);
setup_cancel_handler(NULL);
@@ -5769,6 +5835,7 @@ main(int argc, char **argv)
{"show-script", required_argument, NULL, 10},
{"partitions", required_argument, NULL, 11},
{"partition-method", required_argument, NULL, 12},
+ {"conninfo", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -6137,6 +6204,9 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 13:
+ push_conninfo(pg_strdup(optarg));
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -6363,7 +6433,7 @@ main(int argc, char **argv)
}
/* opening connection... */
- con = doConnect();
+ con = doConnect(false);
if (con == NULL)
exit(1);
@@ -6606,7 +6676,7 @@ threadRun(void *arg)
/* make connections to the database before starting */
for (int i = 0; i < nstate; i++)
{
- if ((state[i].con = doConnect()) == NULL)
+ if ((state[i].con = doConnect(true)) == NULL)
{
/*
* On connection failure, we meet the barrier here in place of
