Version reporting in pgbench - Mailing list pgsql-hackers

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

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...");