v16-0006-Bump-protocol-version-to-3.2.patch

From 5988f206d139808a8c3d191de252f36fb9d24cf6 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <postgres@jeltef.nl>
Date: Wed, 14 Aug 2024 18:25:16 +0200
Subject: [PATCH v16 6/8] Bump protocol version to 3.2

In preparation of new additions to the protocol in a follow up commit
this bumps the minor protocol version number. Instead of bumping the
version number to 3.1, which would be the next minor version, we skip
that one and bump straight to 3.2. The reason for this is that many
PgBouncer releases have, due to an off-by-one bug, reported 3.1 as
supported[1]. These versions would interpret 3.1 as equivalent to 3.0. So
if we would now add extra messages to the 3.1 protocol, clients would
succeed to connect to PgBouncer, but would then cause connection
failures when sending such new messages.

So instead of bumping to 3.1, we bump the protocol version to 3.2, for
which these buggy PgBouncer releases will correctly close the connection
at the startup packet. It's a bit strange to skip a version number due
to a bug in a third-party application, but PgBouncer is used widely
enough that it seems worth it to not cause user confusion when
connecting to recent versions of it. Especially since skipping a single
minor version number in the protocol versioning doesn't really cost us
anything. So, while this is not the most theoretically sound decission,
it is the most pragmatic one.

[1]: https://github.com/pgbouncer/pgbouncer/pull/1007
---
 doc/src/sgml/libpq.sgml                       |   8 +-
 src/include/libpq/pqcomm.h                    |   3 +-
 src/interfaces/libpq/fe-connect.c             |   9 ++
 src/interfaces/libpq/fe-protocol3.c           |   8 +-
 .../modules/libpq_pipeline/libpq_pipeline.c   | 110 +++++++++++++++---
 5 files changed, 117 insertions(+), 21 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 385ddb87c65..f3bf5bcf3d3 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2360,7 +2360,8 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 
        <para>
         The current supported values are
-        <literal>3.0</literal>
+        <literal>3.0</literal>,
+        <literal>3.2</literal>,
         and <literal>latest</literal>. The <literal>latest</literal> value is
         equivalent to the latest protocol version that is supported by the used
         libpq version, which currently is <literal>3.2</literal>, but this will
@@ -2387,10 +2388,11 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 
        <para>
         The current supported values are
-        <literal>3.0</literal>
+        <literal>3.0</literal>,
+        <literal>3.2</literal>,
         and <literal>latest</literal>. The <literal>latest</literal> value is
         equivalent to the latest protocol version that is supported by the used
-        libpq version, which currently is <literal>3.0</literal>, but this will
+        libpq version, which currently is <literal>3.2</literal>, but this will
         change in future libpq releases.
        </para>
       </listitem>
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 6925f10602b..b93bc14f109 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -91,11 +91,10 @@ is_unixsock_path(const char *path)
 
 /*
  * The earliest and latest frontend/backend protocol version supported.
- * (Only protocol version 3 is currently supported)
  */
 
 #define PG_PROTOCOL_EARLIEST	PG_PROTOCOL(3,0)
-#define PG_PROTOCOL_LATEST		PG_PROTOCOL(3,0)
+#define PG_PROTOCOL_LATEST		PG_PROTOCOL(3,2)
 
 typedef uint32 ProtocolVersion; /* FE/BE protocol version number */
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d0844a03396..8d5f46870a1 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7880,6 +7880,15 @@ pqParseProtocolVersion(const char *value, ProtocolVersion *result, PGconn *conn,
 		return true;
 	}
 
+	if (strcmp(value, "3.1") == 0)
+	{
+		conn->status = CONNECTION_BAD;
+		libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
+								context,
+								value);
+		return false;
+	}
+
 	major = strtol(value, &end, 10);
 	if (*end != '.')
 	{
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index fddab05c6e5..12d6c1527db 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1427,7 +1427,13 @@ pqGetNegotiateProtocolVersion3(PGconn *conn)
 		libpq_append_conn_error(conn, "received invalid protocol negotiation message: server requests downgrade to pre-3.0 protocol version");
 		goto failure;
 	}
-	
+
+	if (their_version == PG_PROTOCOL(3, 1))
+	{
+		libpq_append_conn_error(conn, "received invalid protocol negotiation message: server requests downgrade to 3.1 protocol version");
+		goto failure;
+	}
+
 	if (num < 0)
 	{
 		libpq_append_conn_error(conn, "received invalid protocol negotiation message: server reported negative number of unsupported parameters");
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 5b446c37865..2eecdf21f5e 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -198,24 +198,15 @@ send_cancellable_query_impl(int line, PGconn *conn, PGconn *monitorConn)
 }
 
 /*
- * Create a new connection with the same conninfo as the given one.
+ * Fills keywords and vals, with the same options as the ones in the opts
+ * linked-list. Returns the length of the filled in list.
  */
-static PGconn *
-copy_connection(PGconn *conn)
+static
+int
+copy_connection_options(PQconninfoOption *opts, const char **keywords, const char **vals)
 {
-	PGconn	   *copyConn;
-	PQconninfoOption *opts = PQconninfo(conn);
-	const char **keywords;
-	const char **vals;
-	int			nopts = 1;
 	int			i = 0;
 
-	for (PQconninfoOption *opt = opts; opt->keyword != NULL; ++opt)
-		nopts++;
-
-	keywords = pg_malloc(sizeof(char *) * nopts);
-	vals = pg_malloc(sizeof(char *) * nopts);
-
 	for (PQconninfoOption *opt = opts; opt->keyword != NULL; ++opt)
 	{
 		if (opt->val)
@@ -225,8 +216,28 @@ copy_connection(PGconn *conn)
 			i++;
 		}
 	}
-	keywords[i] = vals[i] = NULL;
+	return i;
+}
+
+/*
+ * Create a new connection with the same conninfo as the given one.
+ */
+static PGconn *
+copy_connection(PGconn *conn)
+{
+	const char **keywords;
+	const char **vals;
+	PGconn	   *copyConn;
+	PQconninfoOption *opts = PQconninfo(conn);
+	int			nopts = 1;		/* 1 for the NULL terminator */
 
+	for (PQconninfoOption *opt = opts; opt->keyword != NULL; ++opt)
+		nopts++;
+
+	keywords = pg_malloc0(sizeof(char *) * nopts);
+	vals = pg_malloc0(sizeof(char *) * nopts);
+
+	copy_connection_options(opts, keywords, vals);
 	copyConn = PQconnectdbParams(keywords, vals, false);
 
 	if (PQstatus(copyConn) != CONNECTION_OK)
@@ -1406,6 +1417,72 @@ test_prepared(PGconn *conn)
 	fprintf(stderr, "ok\n");
 }
 
+static void
+test_protocol_version(PGconn *conn)
+{
+	const char **keywords;
+	const char **vals;
+	int			nopts = 2;		/* NULL terminator + max_protocol_version */
+	PQconninfoOption *opts = PQconninfo(conn);
+	int			protocol_version;
+	int			max_protocol_version_index;
+
+	for (PQconninfoOption *opt = opts; opt->keyword != NULL; ++opt)
+		nopts++;
+
+	keywords = pg_malloc0(sizeof(char *) * nopts);
+	vals = pg_malloc0(sizeof(char *) * nopts);
+
+	max_protocol_version_index = copy_connection_options(opts, keywords, vals);
+
+	keywords[max_protocol_version_index] = "max_protocol_version";
+	vals[max_protocol_version_index] = "3.0";
+
+	conn = PQconnectdbParams(keywords, vals, false);
+
+	if (PQstatus(conn) != CONNECTION_OK)
+		pg_fatal("Connection to database failed: %s",
+				 PQerrorMessage(conn));
+
+	protocol_version = PQfullProtocolVersion(conn);
+	if (protocol_version != 30000)
+		pg_fatal("expected 30000, got %d", protocol_version);
+
+	PQfinish(conn);
+
+	vals[max_protocol_version_index] = "3.1";
+	conn = PQconnectdbParams(keywords, vals, false);
+
+	if (PQstatus(conn) != CONNECTION_BAD)
+		pg_fatal("Connecting with max_protocol_version 3.1 should have failed.");
+
+	PQfinish(conn);
+
+	vals[max_protocol_version_index] = "3.2";
+	conn = PQconnectdbParams(keywords, vals, false);
+
+	if (PQstatus(conn) != CONNECTION_OK)
+		pg_fatal("Connection to database failed: %s",
+				 PQerrorMessage(conn));
+
+	protocol_version = PQfullProtocolVersion(conn);
+	if (protocol_version != 30002)
+		pg_fatal("expected 30002, got %d", protocol_version);
+	PQfinish(conn);
+
+	vals[max_protocol_version_index] = "latest";
+	conn = PQconnectdbParams(keywords, vals, false);
+
+	if (PQstatus(conn) != CONNECTION_OK)
+		pg_fatal("Connection to database failed: %s",
+				 PQerrorMessage(conn));
+
+	protocol_version = PQfullProtocolVersion(conn);
+	if (protocol_version != 30002)
+		pg_fatal("expected 30002, got %d", protocol_version);
+	PQfinish(conn);
+}
+
 /* Notice processor: print notices, and count how many we got */
 static void
 notice_processor(void *arg, const char *message)
@@ -2154,6 +2231,7 @@ print_test_list(void)
 	printf("pipeline_idle\n");
 	printf("pipelined_insert\n");
 	printf("prepared\n");
+	printf("protocol_version\n");
 	printf("simple_pipeline\n");
 	printf("singlerow\n");
 	printf("transaction\n");
@@ -2273,6 +2351,8 @@ main(int argc, char **argv)
 		test_pipelined_insert(conn, numrows);
 	else if (strcmp(testname, "prepared") == 0)
 		test_prepared(conn);
+	else if (strcmp(testname, "protocol_version") == 0)
+		test_protocol_version(conn);
 	else if (strcmp(testname, "simple_pipeline") == 0)
 		test_simple_pipeline(conn);
 	else if (strcmp(testname, "singlerow") == 0)
-- 
2.34.1