Subject: [pgjdbc/pgjdbc] PR #3592: Handle protocol 3.2 and wider cancel keys.

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 11 Apr 2025 19:40:44 +0000

We also now have to handle the protocolNegotiationMessage

As of https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=285613c60a7aff5daaf281c67002483b0d26e715 the protocol version has been bumped to 3.2. This will take effect in version 18.

The only protocol change is wider cancel keys. Previously cancel keys were 4 bytes wide now they can have a maximum of 256 bytes but currently the server sends 32 byte wide cancel keys.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=a460251f0a1ac987f0225203ff9593704da0b1a9

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 07:09:16 +0000

(on pgjdbc/src/main/java/org/postgresql/core/ConnectionFactory.java)

Should we restrict the supported protocol versions only? If a newer protocol adds something in 3.7, the old existing drivers won't support 3.7 features anyway.

It looks it would be nice if the driver could reject unsupported values, so the users know the value they configure is ignored or unsupported.

WDYT?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 07:17:43 +0000

(on pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java)

Have you considered nullable array instead so we don't allocate the array?

```suggestion
  private byte @Nullable[] cancelKey;
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 07:28:40 +0000

(on pgjdbc/src/main/java/org/postgresql/core/PGStream.java)

WDYT of creating `class ProtocolVersion { int major; int minor; }`? (or `short major, minor`)

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 07:29:27 +0000

(on pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java)

The return type of `ProtocolVersion` would be easier to reason here

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 07:39:07 +0000

(on pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java)

```suggestion
          byte[] ckey;
```

The allocation here is not needed. `pgStream.receive(..)` below would reallocate

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 08:02:55 +0000

`ProtocolVersion.v3_0.equals(getProtocolVersion...)` could be helpful here

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 08:04:42 +0000

Please include the failure detains into the message. Something like "cancel key should be 4 bytes only for protocol version 3.0. Got .. from the backend" would probably be easier to understand for those who get the exception in their systems.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 08:06:12 +0000

WDYT of one of the following so it could return `ProtocolVersion` instance rather multiple minor/major getters?

`getProtocolVersionInfo()`

* Emphasizes that this is more than just a number now.
* Common naming pattern when moving from primitive to object.

`getProtocolVersionObject()`

* Straightforward, explicitly communicates it's an object.

`getProtocolVersionDetails()`

* Suggests there might be more metadata than just a version number.

`getProtocolVersionValue()`

* Implies it's the full value, which works if the class holds major/minor etc.

`getFullProtocolVersion()`

* Indicates a more complete version compared to the basic int.

`getProtocolVersionEx()` (or `getProtocolVersionExtended()`)

* Simple, adds the "extended" idea, though a bit more API-looking.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 08:10:11 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java:954)

Does the test differ from `closeInProgressStatement`? (looks like the only difference is 3.2 in the setup, however, I can't tell if the rest is the same or not)
If the core is the same, could you factor it to a say `private` method which you call from both of the tests?

It is hard to review and maintain code with large amount of duplicate logic.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 14 Apr 2025 19:31:31 +0000

Probably, will fix

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 15 Apr 2025 18:26:10 +0000

null is the default value. Let's omit the explicit assignment

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 15 Apr 2025 18:27:17 +0000

(on pgjdbc/src/main/java/org/postgresql/core/ProtocolVersions.java)

I guess the constants can be moved to ProtocolVesion class

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 15 Apr 2025 18:29:05 +0000

ProtocolVersion.from(..) would allow returning the well-known constants

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 15 Apr 2025 18:29:42 +0000

(on pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java:181)

I wonder if it breaks previous api

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 16 Apr 2025 01:45:06 +0000

I don't think so, the API is pretty much internal. That said we will see.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 16 Apr 2025 11:41:57 +0000

@vlsi I'm ready to merge this barring any objections

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 11:48:56 +0000

(on pgjdbc/src/main/java/org/postgresql/core/Parser.java)

`protocolVersion` looks unused in the method. Should we remove the parameter?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 11:52:25 +0000

(on pgjdbc/src/main/java/org/postgresql/core/ProtocolVersion.java)

Can we remove no-arg constructor?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 11:57:40 +0000

I'm not sure `(int protocol)` constructor belongs here at it is an implementation detail of `NegotiateProtocolVersion` message. Let's remove (int) constructor, and let's make the other constructor `private`.

```suggestion
  private ProtocolVersion(int major, int minor) {
    this.major = major;
    this.minor = minor;
  }
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 12:00:13 +0000

The naming in `ServerVersion` is like `v8_4`. WDYT of using the same naming?

Should we move the constants to the top of the class as it is probably the most useful info when reading the class?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 12:02:12 +0000

WDYT of a bit more verbose name?

```suggestion
  public static ProtocolVersion fromMajorMinor(int major, int minor) throws SQLException {
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 12:02:30 +0000

```suggestion

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 12:04:09 +0000

Should we log the unrecognized options?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 12:04:54 +0000

Did you intend something as follows here?

```suggestion
            pgStream.setProtocolVersion(ProtocolVersion.fromMajorMinor(...protocol));
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 12:07:28 +0000

By the way, `ServerVersion` is currently implemented as `enum + interface`. It might be ok to have `ProtocolVersion` as an-enum. It would yield equals/hashCode and the named references for free.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 16 Apr 2025 13:17:05 +0000

Probably, yes

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 16 Apr 2025 13:17:43 +0000

We probably should throw an exception here as the connection should not be completed with invalid options

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 16 Apr 2025 13:18:48 +0000

Done

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 13:19:11 +0000

Sadly `values()` allocates a fresh array for every call. WDYT of storing it in a private static field?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 16 Apr 2025 13:34:14 +0000

Done

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 15:06:34 +0000

```suggestion
  private static final ProtocolVersion[] values = values();
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 15:09:53 +0000

WDYT of the usual `PSQLException(GT.tr` dance?

```suggestion
    throw new PSQLException(GT.tr("Unsupported protocol version major: {0}, minor: {1}",
        major, minor), PSQLState.NOT_IMPLEMENTED);
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 15:11:05 +0000

Have you considered passing `protocol major/mior` as `ProtocolVersion`?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 15:12:35 +0000

Have you considered passing `protocol major/minor` as `ProtocolVersion`?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 16 Apr 2025 15:14:10 +0000

The result of `doAuthentication` does not seem to be used. WDYT of reverting the method to `void`?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 17 Apr 2025 12:34:28 +0000

@vlsi I think I have answered all your requests