Version reporting in pgbench
I see that commit 547f04e73 caused pgbench to start printing its
version number. I think that's a good idea in general, but it
appears to me that next to no thought went into the details
(as perhaps evidenced by the fact that the commit message doesn't
even mention it). I've got two beefs with how it was done:
* The output only mentions pgbench's own version, which would be
highly misleading if the server being used is of a different
version. I should think that in most cases the server's version
is more important than pgbench's.
* We have a convention for how client programs should print their
versions, and this ain't it. (Specifically, you should print the
PG_VERSION string not make up your own.)
What I think should have been done instead is to steal psql's
battle-tested logic for printing its startup version banner,
more or less as attached.
One point here is that printing the server version requires
access to a connection, which printResults() hasn't got
because we already closed all the connections by that point.
I solved that by printing the banner during the initial
connection that gets the scale factor, does vacuuming, etc.
If you're dead set on not printing the version till the end,
that could be made to happen; but it's not clear to me that
this way is any worse, and it's certainly easier.
Thoughts?
regards, tom lane
Attachments:
pgbench-version-banner-1.patchtext/x-diff; charset=us-ascii; name=pgbench-version-banner-1.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d7479925cb..78b5ac612b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -63,6 +63,7 @@
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "pgbench.h"
@@ -5493,6 +5494,37 @@ printSimpleStats(const char *prefix, SimpleStats *ss)
}
}
+/* print version banner */
+static void
+printVersion(PGconn *con)
+{
+ int server_ver = PQserverVersion(con);
+ int client_ver = PG_VERSION_NUM;
+
+ if (server_ver != client_ver)
+ {
+ const char *server_version;
+ char sverbuf[32];
+
+ /* Try to get full text form, might include "devel" etc */
+ server_version = PQparameterStatus(con, "server_version");
+ /* Otherwise fall back on server_ver */
+ if (!server_version)
+ {
+ formatPGVersionNumber(server_ver, true,
+ sverbuf, sizeof(sverbuf));
+ server_version = sverbuf;
+ }
+
+ printf(_("%s (%s, server %s)\n"),
+ "pgbench", PG_VERSION, server_version);
+ }
+ /* For version match, only print pgbench version */
+ else
+ printf("%s (%s)\n", "pgbench", PG_VERSION);
+ fflush(stdout);
+}
+
/* print out results */
static void
printResults(StatsData *total,
@@ -5506,7 +5538,6 @@ printResults(StatsData *total,
double bench_duration = PG_TIME_GET_DOUBLE(total_duration);
double tps = ntx / bench_duration;
- printf("pgbench (PostgreSQL) %d.%d\n", PG_VERSION_NUM / 10000, PG_VERSION_NUM % 100);
/* Report test parameters. */
printf("transaction type: %s\n",
num_scripts == 1 ? sql_script[0].desc : "multiple scripts");
@@ -6386,6 +6417,9 @@ main(int argc, char **argv)
exit(1);
}
+ /* report pgbench and server versions */
+ printVersion(con);
+
if (!is_no_vacuum)
{
fprintf(stderr, "starting vacuum...");
Hello Tom,
One point here is that printing the server version requires
access to a connection, which printResults() hasn't got
because we already closed all the connections by that point.
I solved that by printing the banner during the initial
connection that gets the scale factor, does vacuuming, etc.
Ok.
If you're dead set on not printing the version till the end,
that could be made to happen; but it's not clear to me that
this way is any worse, and it's certainly easier.
pgbench (14beta1 dev 2021-06-12 08:10:44, server 13.3 (Ubuntu 13.3-1.pgdg20.04+1))
Why not move the printVersion call right after the connection is created,
at line 6374?
Otherwise it works for me.
--
Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
Why not move the printVersion call right after the connection is created,
at line 6374?
I started with that, and one of the 001_pgbench_with_server.pl
tests fell over --- it was expecting no stdout output before a
"Perhaps you need to do initialization" failure. If you don't
mind changing that, I agree that printing immediately after
the connection is made is a bit less astonishing.
regards, tom lane
Hello Tom,
Why not move the printVersion call right after the connection is
created, at line 6374?I started with that, and one of the 001_pgbench_with_server.pl
tests fell over --- it was expecting no stdout output before a
"Perhaps you need to do initialization" failure. If you don't
mind changing that,
Why would I mind?
I agree that printing immediately after the connection is made is a bit
less astonishing.
Ok, so let's just update the test? Attached a proposal with the version
moved.
Note that if no connections are available, then you do not get the
version, which may be a little bit strange. Attached v3 prints out the
local version in that case. Not sure whether it is worth the effort.
--
Fabien.
Attachments:
pgbench-version-banner-2.patchtext/x-diff; name=pgbench-version-banner-2.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d7479925cb..e61055b6b7 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -63,6 +63,7 @@
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "pgbench.h"
@@ -5493,6 +5494,37 @@ printSimpleStats(const char *prefix, SimpleStats *ss)
}
}
+/* print version banner */
+static void
+printVersion(PGconn *con)
+{
+ int server_ver = PQserverVersion(con);
+ int client_ver = PG_VERSION_NUM;
+
+ if (server_ver != client_ver)
+ {
+ const char *server_version;
+ char sverbuf[32];
+
+ /* Try to get full text form, might include "devel" etc */
+ server_version = PQparameterStatus(con, "server_version");
+ /* Otherwise fall back on server_ver */
+ if (!server_version)
+ {
+ formatPGVersionNumber(server_ver, true,
+ sverbuf, sizeof(sverbuf));
+ server_version = sverbuf;
+ }
+
+ printf(_("%s (%s, server %s)\n"),
+ "pgbench", PG_VERSION, server_version);
+ }
+ /* For version match, only print pgbench version */
+ else
+ printf("%s (%s)\n", "pgbench", PG_VERSION);
+ fflush(stdout);
+}
+
/* print out results */
static void
printResults(StatsData *total,
@@ -5506,7 +5538,6 @@ printResults(StatsData *total,
double bench_duration = PG_TIME_GET_DOUBLE(total_duration);
double tps = ntx / bench_duration;
- printf("pgbench (PostgreSQL) %d.%d\n", PG_VERSION_NUM / 10000, PG_VERSION_NUM % 100);
/* Report test parameters. */
printf("transaction type: %s\n",
num_scripts == 1 ? sql_script[0].desc : "multiple scripts");
@@ -6334,6 +6365,9 @@ main(int argc, char **argv)
if (con == NULL)
exit(1);
+ /* report pgbench and server versions */
+ printVersion(con);
+
pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
PQhost(con), PQport(con), nclients,
duration <= 0 ? "nxacts" : "duration",
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 55b3c3f6fd..49fe48093c 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -100,7 +100,7 @@ pgbench(
'no such database');
pgbench(
- '-S -t 1', 1, [qr{^$}],
+ '-S -t 1', 1, [qr{^pgbench \([^\n]+\)$}],
[qr{Perhaps you need to do initialization}],
'run without init');
pgbench-version-banner-3.patchtext/x-diff; name=pgbench-version-banner-3.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d7479925cb..20910e5ad1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -63,6 +63,7 @@
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "pgbench.h"
@@ -5493,6 +5494,37 @@ printSimpleStats(const char *prefix, SimpleStats *ss)
}
}
+/* print version banner */
+static void
+printVersion(PGconn *con)
+{
+ int server_ver = PQserverVersion(con);
+ int client_ver = PG_VERSION_NUM;
+
+ if (server_ver != client_ver)
+ {
+ const char *server_version;
+ char sverbuf[32];
+
+ /* Try to get full text form, might include "devel" etc */
+ server_version = PQparameterStatus(con, "server_version");
+ /* Otherwise fall back on server_ver */
+ if (!server_version)
+ {
+ formatPGVersionNumber(server_ver, true,
+ sverbuf, sizeof(sverbuf));
+ server_version = sverbuf;
+ }
+
+ printf(_("%s (%s, server %s)\n"),
+ "pgbench", PG_VERSION, server_version);
+ }
+ /* For version match, only print pgbench version */
+ else
+ printf("%s (%s)\n", "pgbench", PG_VERSION);
+ fflush(stdout);
+}
+
/* print out results */
static void
printResults(StatsData *total,
@@ -5506,7 +5538,6 @@ printResults(StatsData *total,
double bench_duration = PG_TIME_GET_DOUBLE(total_duration);
double tps = ntx / bench_duration;
- printf("pgbench (PostgreSQL) %d.%d\n", PG_VERSION_NUM / 10000, PG_VERSION_NUM % 100);
/* Report test parameters. */
printf("transaction type: %s\n",
num_scripts == 1 ? sql_script[0].desc : "multiple scripts");
@@ -6332,7 +6363,13 @@ main(int argc, char **argv)
/* opening connection... */
con = doConnect();
if (con == NULL)
+ {
+ printf("%s (%s)\n", "pgbench", PG_VERSION);
exit(1);
+ }
+
+ /* report pgbench and server versions */
+ printVersion(con);
pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
PQhost(con), PQport(con), nclients,
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 55b3c3f6fd..6a40e4b7d1 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -92,7 +92,7 @@ pgbench(
pgbench(
'no-such-database',
1,
- [qr{^$}],
+ [qr{^pgbench \([^\n]*\)$}],
[
qr{connection to server .* failed},
qr{FATAL: database "no-such-database" does not exist}
@@ -100,7 +100,7 @@ pgbench(
'no such database');
pgbench(
- '-S -t 1', 1, [qr{^$}],
+ '-S -t 1', 1, [qr{^pgbench \([^\n]+\)$}],
[qr{Perhaps you need to do initialization}],
'run without init');
Fabien COELHO <coelho@cri.ensmp.fr> writes:
Note that if no connections are available, then you do not get the
version, which may be a little bit strange. Attached v3 prints out the
local version in that case. Not sure whether it is worth the effort.
I'm inclined to think that the purpose of that output is mostly
to report the server version, so not printing it if we fail to
connect isn't very surprising. Certainly that's how psql has
acted for decades.
regards, tom lane
Note that if no connections are available, then you do not get the
version, which may be a little bit strange. Attached v3 prints out the
local version in that case. Not sure whether it is worth the effort.I'm inclined to think that the purpose of that output is mostly
to report the server version, so not printing it if we fail to
connect isn't very surprising. Certainly that's how psql has
acted for decades.
I'm fine with having a uniform behavior over pg commands.
Thanks for the improvement!
--
Fabien.