Subject: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests

From: "sehrope (@sehrope)" <[email-masked]>
Date: Sat, 14 Feb 2026 18:35:05 +0000

Three commits here which:

* Add a new `@RequireServerVersion` annotation for tests that allows declaring the applicable server versions.
* Replaces the `@DisabledIfServerVersionBelow` and `@DisabledIfServerVersionGreater` usage with `@RequireServerVersion`
* Removes the unused annotations

Main driver for this was trying to understand which test is running when. The double negative for the annotation vs the check makes it confusing (e.g., it runs when it's not disabled and that is driven by the version being below...).

It's also ambiguous if something like "*Greater than 10*" really means greater than or equal 10.0.

The new approach positive affirms the situations where we want the thing to run with annotation args for:

* `lt` - Less than
* `lte` - Less than or equal
* `gt` - Greater than
* `gte` - Greater than or equal

I'd expect that `lt` and `gte` are going to be the most useful. The "less than" makes sense if you want all versions older than X (where presumably it breaks). And "greater than or equal" makes sense when X is the first version that supports a given feature that is being tested. Ex:

```diff
@@ -297,7 +297,7 @@ class LogicalReplicationStatusTest {
    * Test fail on PG version 9.4.5 because postgresql have bug.
    */
   @Test
-  @DisabledIfServerVersionBelow("9.4.8")
+  @RequireServerVersion(gte = "9.4.8")
   void applyLocationDoNotDependOnFlushLocation() throws Exception {
     PGConnection pgConnection = (PGConnection) replicationConnection;
```

Note that the old `@DisabledIfServerVersionGreater("19")` name implied a `>` check, but its implementation actually used `>=`, disabling the test for version 19 and above. The migration to `@RequireServerVersion(lt = "19")` preserves that existing runtime behavior while making the boundary semantics explicit.

One minor improvement to the old approach was caching the server response in the junit context. That shaves a connection open / close per annotation call. I doubt it's meaningfully different (24 uses of the annotation times 10ms per connection rounds to nothing) but nice to have an example of that in the test code. It may be useful somewhere else too.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sat, 14 Feb 2026 18:48:24 +0000

I would suggest going with junit-like naming: `@DisabledForServerVersionRange`.

See
* https://docs.junit.org/5.13.0/api/org.junit.jupiter.api/org/junit/jupiter/api/condition/DisabledForJreRange.html
* https://docs.junit.org/5.13.0/api/org.junit.jupiter.api/org/junit/jupiter/api/condition/package-summary.html

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Sat, 14 Feb 2026 21:40:50 +0000

Renamed to `@EnabledForServerVersionRange`. I kept it positive (i.e. "EnabledFor" v.s. "DisabledFor") as the whole point is that it's easier to reason about what it's doing when you don't have to double negate it.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sat, 14 Feb 2026 22:04:36 +0000

> EnabledFor v.s. DisabledFor

It might be reasonable to have both.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Sun, 15 Feb 2026 11:13:36 +0000

> It might be reasonable to have both.

I agree, It's plain to say disabled for.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Mon, 16 Feb 2026 15:33:31 +0000

Do you mean add `@DisabledForServerVersionRange` (and it's condition handler) with nothing using them as an option for the future? Or replace the `@EnabledForServerVersionRange` with `@DisabledForServerVersionRange`?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 16 Feb 2026 16:18:02 +0000

I mean we could add `DisabledForServerVersionRange` either now (even if it not used) or later when it would be helpful.
Having both at the same time might be helpful for different cases.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 23 Feb 2026 10:55:27 +0000

@sehrope are we waiting for something? Might as well merge this

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 23 Feb 2026 11:49:53 +0000

(on testkit/src/main/java/org/postgresql/test/annotations/EnabledForServerVersionRange.java)

Should the javadoc be reworded to avoid double negation as well? (disables .. does not..)

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 23 Feb 2026 11:50:17 +0000

(on testkit/src/main/java/org/postgresql/test/annotations/EnabledForServerVersionRange.java:33)

There's no check for contradictory specs like `@EnabledForServerVersionRange(lt = "9.4", gt = "10.0")` or specifying both lt and lte simultaneously. Worth at least validating that lt/lte aren't both set, and same for gt/gte.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 23 Feb 2026 11:50:49 +0000

(on testkit/src/main/java/org/postgresql/test/impl/EnabledForServerVersionRangeCondition.java)

If someone writes `@EnabledForServerVersionRange` with no parameters at all, the annotation is found but all four bounds are empty — so it silently does nothing and the test is enabled. It would be better to fail fast with an error if none of lt/lte/gte/gt are set.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 23 Feb 2026 12:02:48 +0000

> Worth at least validating that lt/lte aren't both set

An alternative option could be: `String min, String max, boolean minInclusive, boolean maxInclusive`.
Almost all the usages of the new annotation are `gte`, so we can go with `minInclusive = true`, `maxInclusive = false` by default. It would match the typical half-open ranges like in `List.subList(int fromIndex, int toIndex)`, and it would make `lte + lt` unrepresentable.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 23 Feb 2026 12:05:41 +0000

For reference, JUnit has both `@DisabledForJreRange` and `@EnabledForJreRange` because their `min` and `max` are always `inclusive`, so they need two annotations to distinguish between `less than` and `less or equal than`.

However, if we allow `minInclusive / maxInclusive` customization (or the current lt/lte/gt/gte), we don't really need a counterpart `@Disabled...` annotation.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Mon, 23 Feb 2026 13:06:55 +0000

No I don't like the hidden options with defaults as the entire purpose is to make the determination explicit. That's why I went with separate `lt` and `lte`. Ditto for not having a flag that flips it from "include" to "exclude".

The lt/lte or gt/gte check would be fine to add (the cross one is way more complicated).

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Mon, 23 Feb 2026 13:07:22 +0000

Was mostly done addressing the feedback but didn't get a chance yet to finish testing the changes.

Snowed in for the day so figure will get to wrapping it up by this afternoon.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Mon, 23 Feb 2026 21:55:55 +0000

Rebased and force pushed. Adds `@DisabledForServerVersionRange` as well (with no usage yet but ready if someone wants to use it).

Added a base parent for both as the majority of the version related code is the same. Each just defines how it matches and whether it's negative or positive result.

Also includes checks that:

* At least one of lt / lte / gte / gt is populated
* Both lt / lte cannot be populated
* Both gt / gte cannot be populated

To unify the code both are implemented by deciding if we match, and then returning either the enabled or disabled result depending on the annotation. Note that the "matches" logic for the disabled annotation is backwards. If it matches, the test is disabled.

The response message is slightly worse than before for the enabled annotation as we only say whether we did not match at all (previously we would explicitly say which of the lt / lte / gte / gt clauses was violated). But that was done because the same approach does not work for the disabled annotation and wanted to keep the code consistent. If someone is investigating can always look at the actual annotations anyway.

I'll merge this tomorrow if there's nothing new.