Subject: [pgjdbc/pgjdbc] PR #3895: implement require_auth, this is pretty much how libpq does this.

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 16 Dec 2025 18:04:18 +0000

I still need to figure out how to run the tests.
And add documentation.

---

From: "gitguardian[bot] (bot) (@gitguardian[bot])" <noreply+gitguardian[bot]@github.com>
Date: Tue, 16 Dec 2025 18:04:54 +0000

#### ⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

<details>
<summary>🔎 Detected hardcoded secret in your pull request</summary>
<br>

| GitGuardian id | GitGuardian status | Secret                         | Commit           | Filename        |                      |
| -------------- | ------------------ | ------------------------------ | ---------------- | --------------- | -------------------- |
| [-](https://dashboard.gitguardian.com/workspace/59849/incidents/secrets) | - | Generic Password | 23d62b377ef2c1da898d3d9f27ae537e7088ea9b | pgjdbc/src/main/java/org/postgresql/PGProperty.java | [View secret](https://github.com/pgjdbc/pgjdbc/commit/23d62b377ef2c1da898d3d9f27ae537e7088ea9b#diff-f1f17a9527eed81e0d547e9d6b7d5a00c62627e6c9ddeac6416fe290a2bd07d3R623) |

</details>

<details>
<summary>🛠 Guidelines to remediate hardcoded secrets</summary>
<br>

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secret safely. [Learn here](https://blog.gitguardian.com/secrets-api-management?utm_source=product&amp;utm_medium=GitHub_checks&amp;utm_campaign=check_run_comment) the best practices.
3. Revoke and [rotate this secret](https://docs.gitguardian.com/secrets-detection/secrets-detection-engine/detectors/generics/generic_password#revoke-the-secret?utm_source=product&amp;utm_medium=GitHub_checks&amp;utm_campaign=check_run_comment).
4. If possible, [rewrite git history](https://blog.gitguardian.com/rewriting-git-history-cheatsheet?utm_source=product&amp;utm_medium=GitHub_checks&amp;utm_campaign=check_run_comment). Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

-   following these [best practices](https://blog.gitguardian.com/secrets-api-management/?utm_source=product&amp;utm_medium=GitHub_checks&amp;utm_campaign=check_run_comment) for managing and storing secrets including API keys and other credentials
-   install [secret detection on pre-commit](https://docs.gitguardian.com/ggshield-docs/integrations/git-hooks/pre-commit?utm_source=product&amp;utm_medium=GitHub_checks&amp;utm_campaign=check_run_comment) to catch secret before it leaves your machine and ease remediation.

</details>

---

<sup>🦉 [GitGuardian](https://dashboard.gitguardian.com/auth/login/?utm_medium=checkruns&amp;utm_source=github&amp;utm_campaign=cr1) detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.<br/></sup>

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 16 Dec 2025 18:21:58 +0000

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

WDYT of `enum` and `EnumSet`?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 16 Dec 2025 18:26:32 +0000

libpq says "Negated and non-negated forms may not be combined in the same setting".
WDYT of replicating the same behavior?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 16 Dec 2025 19:04:23 +0000

sure, that makes more sense

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 16 Dec 2025 19:04:38 +0000

seems appropriate, will change

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Tue, 16 Dec 2025 19:32:18 +0000

Why are we doing it with bit masks rather than throwing the allowed / disallowed in a `Set<string>(...)` and calling `.has(...)`?

We have to do the mapping from the FEBE code to a name once and then can use that for the check. It's not like this happening in some tight loop. It'd be a single check for the whole connection handshake right?

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Tue, 16 Dec 2025 19:35:26 +0000

Missed this before I wrote the other comment. Yes this whole thing would be easier to implement and understand with enums and sets. The parsing step becomes much easier too (just check if we've seen the value already for the dupe error).

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 16 Dec 2025 20:17:19 +0000

changed in latest

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 16 Dec 2025 20:17:30 +0000

changed in latest

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Tue, 16 Dec 2025 20:18:38 +0000

We're silently skipping unknown methods here. Is that intentional?

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Tue, 16 Dec 2025 20:19:01 +0000

(Because the `fromString(...)` is going to return null)

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 16 Dec 2025 20:46:19 +0000

I changed fromString to throw an exception for unrecognized options

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 17 Dec 2025 05:47:08 +0000

There's no need to track two sets just one set with "seen flags" is enough

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 17 Dec 2025 05:47:47 +0000

It does not need to have its own loop

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Wed, 17 Dec 2025 13:00:50 +0000

Can put all of this logic in the enum itself and expose a single method to check the auth mode vs the connection's actual value. So it'd just be `public static void check(String requireAuth, String authMethod)`.

That'd make the logic itself easier to unit test vs all the possible server configs. Would still be good to have some server related coverage for it as well, but we could do something pretty exhaustive in the unit test.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 24 Dec 2025 14:20:20 +0000

```suggestion
    static void checkAuth(Set<AuthMethod> allowedMethods, AuthMethod authMethod) throws PSQLException {
```

`isAllowed(...)` results in repeated parsing, so why don't we parse the set once and pass it over the methods?

I'm not fond of `is...` methods that return void.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 24 Dec 2025 14:23:17 +0000

(on pgjdbc/src/test/java/org/postgresql/test/core/RequireAuthTest.java)

```suggestion
```

Just rethrow the exception

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 24 Dec 2025 14:24:14 +0000

```suggestion
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 24 Dec 2025 14:26:47 +0000

```suggestion
    TestUtil.setTestUrlProperty(props, PGProperty.PG_DBNAME, "authtest");
```

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 24 Dec 2025 15:17:57 +0000

agreed on changing the name. 
As for parsing once, we only ever call checkAuth once so it is only parsed once, and requireAuth could be null

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 24 Dec 2025 15:42:55 +0000

`AUTH_REQ_OK` brings the second parse, doesn't it?

Frankly, I find it much eaiser to reason about if the inputs are parsed once, and the code deals with parsed values rather than parse at multiple locations.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 24 Dec 2025 15:55:24 +0000

AFAIUI `AUTH_REQ_OK` will only require a parse if it is simple password. Otherwise `pgStream.isFinishedAuthenticationRequests()` will be true and we don't call checkAuth. That said if you find it easier to reason, that's a good enough reason for me to change it

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 23 Feb 2026 12:49:25 +0000

@vlsi I believe this is ready to go? Everything has been resolved.

---

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

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

GSS encryption != GSS authentication.
I think AuthMethod.GSS should be in `case AUTH_REQ_GSS`

---

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

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

I would rather have `@Nullable EnumSet<AuthMethod>` to distinguish "not set" vs "property was set to an empty value" cases.

---

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

Whitespace looks odd

```suggestion
                  if (!pgStream.isFinishedAuthenticationRequests()) {
```

---

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

Is there a reason for explicit `= true`?

---

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

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

Does it really mean `finished authentication requests`?
Can it be the case the backend issues SCRAM request after MD5?

If the only reason for this call is to track "have at least one auth request", we should consider renaming the property to something like "has auth requests", and we could set it like `if (authReq != AUTH_REQ_OK) { pgStream.hasAuthRequests(); }`

---

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

GSS encryption is strange the way it is implemented. IIRC it's a bit like TLS I don't believe there is ever a GSS auth request, just an encryption request.

---

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

We have `case AUTH_REQ_GSS`. Does it mean something different from "gss authentication requested"?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 23 Feb 2026 19:28:00 +0000

Yeah, it's strange the way they implemented encrypted GSS . Starts at https://github.com/pgjdbc/pgjdbc/blob/5aa76f88df5c95f1a8a9f5235ba45acffd5bb50e/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java#L200 and the function is https://github.com/pgjdbc/pgjdbc/blob/5aa76f88df5c95f1a8a9f5235ba45acffd5bb50e/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java#L485 I think we have to attempt the encrypted GSS and if it succeeds then fail with an error.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 23 Feb 2026 19:46:26 +0000

any idea why https://github.com/pgjdbc/pgjdbc/actions/runs/22321662457/job/64581275265?pr=3895 is suddenly failing with ```org.postgresql.util.PSQLException: ERROR: out of shared memory
      Hinweis: You might need to increase max_locks_per_transaction.```

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Tue, 24 Feb 2026 16:16:54 +0000

I'm able to reproduce this error locally and tracked it down with git bisect:

```
3fab588d93ae4cfeb13f75ba08894b804b256d51 is the first bad commit
commit 3fab588d93ae4cfeb13f75ba08894b804b256d51
Author: Vladimir Sitnikov <[email-masked]>
Date:   Mon Jan 19 12:16:57 2026 +0300

    test: add autosave=always|never|conservative and cleanupSavepoints=true|false to the randomized CI jobs

:040000 040000 f663e7608969c1d7de2dbcc4df3bf8003a51c112 a2ace7b120c0c1e16f7640077410ce511aec429d M	.github
:040000 040000 93916651a74db1749db76dba81068287f9c87e0a c44dce9d078a5385473e9eccf9143057f0f38937 M	build-logic
:040000 040000 d1201b2777e25f2e89f51c6729e97511fb352e02 fa625fdb5b472f40550997b24dd625da8d265b09 M	pgjdbc
bisect run success
```

Here's the steps if you want to try this locally:

Save this script as `build/database-encoding-test` and `chmod +x`:

```
#!/usr/bin/env bash

main () {
    ./gradlew -Duser.country=de -D.user.language=DE -DreWriteBatchedInserts=true -Dautosave=always cleanTest postgresql:test --tests DatabaseEncodingTest
}

main "$@"
```

Start the 9.1 server with:

```
PGV=9.1 TZ=UTC XA=no SSL=no SCRAM=no CREATE_REPLICAS=no docker/bin/postgres-server 
```

Then starting at the HEAD of this PR (or any other "bad") commit:

```
git bisect start
git bisect bad
git bisect good 79b784e3 # This is REL42.7.9
git bisect run build/database-encoding-test
```

That commit message in 3fab588d93ae4cfeb13f75ba08894b804b256d51 is deceiving as well. It's labeled `test: add autosave=... to the CI jobs` but it's actually making some changes to the `QueryExecutorImpl` as well. Still have to step through to understand why, but it's something in that commit that changed either when we're creating a save point or when we're cleaning them up.

That combined with the lower defaults for 9.1 is causing this to fail over there for some specific combo of parameters.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 24 Feb 2026 16:27:32 +0000

> That commit message in [3fab588](https://github.com/pgjdbc/pgjdbc/commit/3fab588d93ae4cfeb13f75ba08894b804b256d51) is deceiving as well. It's labeled `test: add autosave=... to the CI jobs` but it's actually making some changes to the `QueryExecutorImpl` as well. Still have to step through to understand why, but it's something in that commit that changed either when we're creating a save point or when we're cleaning them up.
> 

I expect this is just the extra options in the test combined with the lower defaults in 9.1
@vlsi ?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 24 Feb 2026 16:32:44 +0000

By the way, I asked Opus 4.6 to identify the reason for the failure (I gave it a stacktrace and a link to the CI log), and it ended up with virtually the same analysis (I haven't had time to test it manually, so I did not post the results):

> The failure occurs in only one CI configuration:
> Java 21, PG 9.1, autosave always, rewrite_batch_inserts, query_timeout 15
> The key parameter is **autosave=always**.
> **What's happening**
> 1. DatabaseEncodingTest.encoding() performs ~11,100 INSERTs in a single transaction (autoCommit=false):
>   - 1..0xd800 step 100 → ~553 inserts
>   - 0xe000..0x10000 → ~82 inserts
>   - 0x10000..0x110000 → ~10,486 inserts
> 2. With autosave=always, pgjdbc wraps every statement in SAVEPOINT / RELEASE SAVEPOINT. That's ~11,100 nested subtransactions.
> 3. In PostgreSQL, each savepoint (subtransaction) consumes entries in the shared lock table. Even after RELEASE SAVEPOINT, lock entries are merged into the parent transaction, but shared memory slots are not freed until commit.
> 4. The lock table size = max_locks_per_transaction × max_connections = 64 × 100 = 6,400 slots (defaults). With 11,000+ subtransactions, the table overflows.
> 5. PG 9.1 is a very old version with less efficient subtransaction lock management. The same test passes on newer PG versions (9.3, 11, 14, 17).
> **How to investigate further**
> 1. Check if it reproduces without autosave=always — most likely it won't
> 2. Check history — this may be a flaky test unrelated to PR #3895. Look at whether this job also fails intermittently on master
> 3. Reproduce locally — run PG 9.1 with default max_locks_per_transaction=64 and autosave=always, then run the encoding test
> **Possible fixes**
> - Increase max_locks_per_transaction in the CI entrypoint.sh (e.g., to 256)
> - Add periodic con.commit() calls inside the test loop to release locks
> - Skip the test when autosave=always on old PG versions
> - Drop PG 9.1 from the test matrix — it has been EOL since 2016
> The simplest and most correct fix is either increasing max_locks_per_transaction in the CI PostgreSQL configuration, or removing PG 9.1 from the test matrix entirely.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 24 Feb 2026 16:36:21 +0000

Why 3fab588 is the first bad commit

The commit does two things:

1. Adds `autosave=always` to the CI test matrix
  Before this commit, tests never ran with autosave=always.
  The unlucky random combination assigned autosave=always to the PG 9.1 job.

2. Changes shouldCreateAutomaticSavepoint() to create more savepoints

The old code checked isTransactionActive() before checking AutoSave.NEVER, meaning it would bail out early in some cases. The new code reorders the checks and also expands isSpecialQuery() — but the net effect is that with autosave=always, every non-special statement now gets wrapped in SAVEPOINT / RELEASE  SAVEPOINT.

The combined effect

DatabaseEncodingTest.encoding() does ~11,100 INSERTs in one transaction. With autosave=always, each INSERT creates a subtransaction. PG 9.1's default max_locks_per_transaction=64 gives only ~6,400 lock table slots — far fewer than the ~11,100 needed.

---

Frankly, I think the test did the right thing: it identified a bug.

Then we could have several workarounds:
  1. Try if increase max_locks_per_transaction in the CI PostgreSQL 9.1 config helps (e.g., -c max_locks_per_transaction=256 in entrypoint.sh).
  2. Exclude autosave=always from PG 9.1 in the matrix — add a rule like:
  `matrix.imply({pg_version: '9.1'}, {autosave: {value: 'never'}});`
  3. Add periodic commits in `DatabaseEncodingTest` to keep subtransaction count manageable
  
I am leaning towards 1 or 2.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 24 Feb 2026 16:44:59 +0000

> Why [3fab588](https://github.com/pgjdbc/pgjdbc/commit/3fab588d93ae4cfeb13f75ba08894b804b256d51) is the first bad commit
> 

easy enough to try 1 in the PR.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 25 Feb 2026 11:51:44 +0000

OK, tests are passing, any objections to merging this ?
@vlsi ?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 27 Feb 2026 14:00:08 +0000

This mimics the server https://github.com/postgres/postgres/blob/16743db061e431d40522547c6436af6616026caa/src/interfaces/libpq/fe-auth.c#L955

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Fri, 27 Feb 2026 14:14:46 +0000

There's server-side comment: https://github.com/postgres/postgres/blob/16743db061e431d40522547c6436af6616026caa/src/interfaces/libpq/fe-auth.c#L969-L971

"If implicit GSS auth has already been performed via GSS
					 * encryption, we don't need to have performed an
					 * AUTH_REQ_GSS exchange"

It implies GSS auth can indeed happen via GSS encryption.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 27 Feb 2026 14:43:57 +0000

Correct. Which is why you need to check after the stream has been encrypted.