Re: Version reporting in pgbench

2021-06-18 Thread Fabien COELHO




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.




Re: Version reporting in pgbench

2021-06-18 Thread Tom Lane
Fabien COELHO  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




Re: Version reporting in pgbench

2021-06-18 Thread Fabien COELHO


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.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 / 1, 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');
 
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 / 1, 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 

Re: Version reporting in pgbench

2021-06-18 Thread Tom Lane
Fabien COELHO  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




Re: Version reporting in pgbench

2021-06-18 Thread Fabien COELHO



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.