Subject: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 07:00:45 +0000

Commit [d232e605](https://github.com/pgjdbc/pgjdbc/commit/d232e605de960f3a8ca2dfef4974dbe546b860bb) removed the pre-describe step from internalExecuteBatch for queries with wantsGeneratedKeysAlways. Without it the driver cannot estimate result row sizes before pipelining, so flushIfDeadlockRisk falls back to 250 bytes per query. For INSERT ... RETURNING with large columns the actual response is orders of magnitude larger, overflowing TCP buffers and causing client/server deadlock.

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing](https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md) document?
* [x] Have you checked to ensure there aren't other open [Pull Requests](../../pulls) for the same update/change?

<!-- You can erase any parts of this template not applicable to your Pull Request. -->

### New Feature Submissions:

1. [x] Does your submission pass tests?
2. [x] Does `./gradlew styleCheck` pass ?
3. [x] Have you added your new test classes to an existing test suite in alphabetical order?

### Changes to Existing Features:

* [x] Does this break existing behaviour? If so please explain.
* [x] Have you added an explanation of what your changes do and why you'd like us to include them?
* [x] Have you written new tests for your core changes, as applicable?
* [x] Have you successfully run tests with your changes locally?

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 07:08:06 +0000

@vlsi  this undoes a previous change of yours so would be good to get your eyes on it too

I know for certain that 42.5.6 does not have this issue and worked my way to this commit as the potential cause

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Thu, 09 Apr 2026 08:34:21 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java:917)

Nice catch

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 09:05:21 +0000

@vlsi  could you Execute CI on this branch when able :)

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 09 Apr 2026 09:40:18 +0000

This is an AI generated review 
### 1. forceBinaryTransfers is no longer handled

The original code triggered pre-describe for preDescribe || forceBinaryTransfers. This PR only restores the wantsGeneratedKeysAlways path. The forceBinaryTransfers case was also removed by d232e605 and is not restored here. That's probably
fine since forceBinaryTransfers has its own pre-describe in executeInternal() (line ~499), but it's worth confirming that internalExecuteBatch doesn't need it. The batch execution calls 
connection.getQueryExecutor().execute(queries, parameterLists, ...) which goes through a different path than executeInternal.

### 2. Error handling difference

The original code had a comment // Will not reach here (see above) after handler.handleCompletion(). The new code has the same structure but without that comment. More importantly — if the describe fails, the original code would 
effectively abort the batch (since handleCompletion throws). The new code does the same, which is correct.

However, there's a subtle issue: after the catch, execution continues to the describeResult null check. If handleCompletion() throws (which it should for errors), this is unreachable. But if for some reason it doesn't throw, 
describeHandler.getResults() could return stale/partial results. This isn't a regression since the original code had the same pattern, but it's worth noting.

### 3. The sameQueryAhead logic is not restored — and that's correct

The original code also set preDescribe when sameQueryAhead was true (i.e., same query repeated in the batch). That was the part causing #2882 — it prevented rewritten batch inserts from becoming server-side prepared. This PR correctly 
leaves that out.

### 4. Missing the oneshot flag interaction

The original code had this logic:

java
boolean sameQueryAhead = queries.length > 1 && queries[0] == queries[1];
if (!sameQueryAhead || isOneShotQuery(null)) {                                                                                                                                                                                                 
    flags |= QueryExecutor.QUERY_ONESHOT;                                                                                                                                                                                                      
}                                                                                                                                                                                                                                              

That was also removed by d232e605 and simplified to just if (isOneShotQuery(null)). This PR doesn't touch that, which means for wantsGeneratedKeysAlways batches with repeated queries, the statement might still be marked oneshot when it 
shouldn't be. That's a pre-existing issue from d232e605 though, not introduced by this PR.

### 5. Test coverage

The three tests are well-designed:
- prepareStatement(sql, String[]) — the wantsGeneratedKeysAlways path via column names
- prepareStatement(sql, RETURN_GENERATED_KEYS) — same path via integer flag
- prepareStatement(sql) with explicit RETURNING * — this one actually tests a different path where wantsGeneratedKeysAlways is false, so it's a control test

The test parameters (500 rows × 10KB = 5MB) are well-chosen to reliably trigger the deadlock without the fix.

### 6. Relationship to PR #4013

This PR and #4013 address different aspects of the same deadlock:
- #4013: the threshold was wrong (hardcoded 64KB vs actual socket buffer)
- #4014: the estimation was wrong (250 bytes vs actual row size)

They're independent fixes. Either one alone would help in some scenarios, but both are needed for full coverage. A large-column RETURNING batch could deadlock even with the correct threshold if the estimation is 250 bytes per row.

### Summary

The fix is correct and well-scoped. It restores exactly the pre-describe behavior that was accidentally removed, without reintroducing the sameQueryAhead logic that caused #2882. One thing to verify: does the forceBinaryTransfers case also
need restoring for batch execution, or is it covered elsewhere?

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 10:19:07 +0000

@davecramer  Looks like the pipeline Java 25, liberica, PG 17, scram, no_ssl, windows, server_tz Pacific/Chatham, client_tz Pacific/Chatham, fr_FR, no_gss, replication, no_slow_tests, adaptive_fetch, autosave always, cleanupSavepoints, assume min version 8.0 is stuck -> rest look good i suspect its flaky?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Thu, 09 Apr 2026 10:52:28 +0000

@bilal-shehata , Do the tests reproduce the failure without the fix?
I tried running it with my macOS, and it succeeds even without the fix.

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 11:26:15 +0000

@vlsi, yeah good call, LOOPBACK tends to behave a bit differently than real world (I don't fully understand the details as to why this is the case) Ill put together more detailed instructions on how to reproduce

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 13:04:28 +0000

@vlsi  with the payload set to 100_000 i was able to cause the deadlock M4 mac client  PostgreSQL 18.3 (Debian 18.3-1.pgdg13+1) on aarch64-unknown-linux-gnu, compiled by gcc (Debian 14.2.0-19) 14.2.0, 64-bit server (running in docker) - incase youre still unable to reproduce i would increase the payload size a bit more at some point it will hard stuck

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 13:11:53 +0000

@vlsi , slight nuance, if you run this in docker i found that if you deadlock you will need to restart the server before switching branches to the fix to verify it will no longer deadlock (otherwise it will still be deadlocked when you try to run the tests again)

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 09 Apr 2026 23:33:10 +0000

@davecramer  @vlsi  could i get a another CI run

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Fri, 10 Apr 2026 07:01:12 +0000

@vlsi  it does seem like for this specific arrangement
[Java 26, oracle, PG 10, simple query, no_scram, ssl, ubuntu, server_tz Pacific/Chatham, client_tz UTC, de_DE, no_gss, no_replication, no_slow_tests, adaptive_fetch, stress JIT](https://github.com/pgjdbc/pgjdbc/actions/runs/24229762672/job/70738784170?pr=4014#logs)
Started 30m 19s ago

this causes some genuine test failures
FAILURE 300,0sec,    2 completed,   1 failed,   1 skipped, org.postgresql.test.jdbc2.BatchDeadlockTest > org.postgresql.test.jdbc2.BatchDeadlockTest[5], [5] returningInQuery=NO, binaryMode=REGULAR

Ill spend some time investigating later

I suspect it might be simple query but need to dig more

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Fri, 10 Apr 2026 07:17:32 +0000

@vlsi actually wasnt that complicated

needed to make an additional code change - you can reproduce the problem by using  the following (before and after applying my recent commit)
`./gradlew :postgresql:test --tests "org.postgresql.test.jdbc2.BatchDeadlockTest" --rerun-tasks
-DpreferQueryMode=simple`

if you rerun CI it should work now 🍀

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Fri, 10 Apr 2026 07:37:37 +0000

Disable batching for simple query would effectively revert batching to non-batching, thus it would hurt performance.

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Fri, 10 Apr 2026 07:40:39 +0000

> Disable batching for simple query would effectively revert batching to non-batching, thus it would hurt performance.

Agreed, but only in the case of nonPreDescribed Returning queries. Unless we proceed with the partial fix? i.e. leave simple query at risk and fix the extended query mode since thats the default anyway - and would solve this problem for most users

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Fri, 10 Apr 2026 08:00:42 +0000

@vlsi  ive put in a new approach, let me know what you think

- basically allow batching for simple query but if we do we need to constantly flush the buffer

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Fri, 10 Apr 2026 08:43:35 +0000

>basically allow batching for simple query but if we do we need to constantly flush the buffer

Please clarify how it is different from disabling the batch altogether

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Fri, 10 Apr 2026 09:43:05 +0000

> Please clarify how it is different from disabling the batch altogether

you're correct i misunderstood the code. 
I think fixing it for simple query will be quite complex and will require more robust approach. Meanwhile I think we can get this change out for extended, ive filtered our my test cases for simple query mode

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Sat, 11 Apr 2026 00:27:38 +0000

@davecramer  @vlsi , let me know if there is anything else remaining for approval. Thanks

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Sun, 12 Apr 2026 04:58:20 +0000

@vlsi gentle bump on this one when you get the chance

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 16 Apr 2026 05:53:41 +0000

@davecramer @vlsi , if you could let me know if anything is remaining on the PR

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 19 Apr 2026 08:13:25 +0000

I made the needed changes, notably:
1) The initial revert of d232e605de960f3a8ca2dfef4974dbe546b860bb did not account for `statement.closeOnCompletion()` (it was a long-standing bug), so I added `DiscardResultHandler` to avoid creating unwanted resultsets
2) The test used `text` column, and there was no way to estimate the max number of bytes returned. I switched to `varchar(N)` in tests, and added `varchar` estimator (it was not really working previously), see `estimateMaxLength`
3) Previously `flushIfDeadlockRisk` could issue `sync` even before the first query which apparently makes no sense. In my tests the logic works as expected, and it attempts to keep receiving buffer under 64000.

For the reference, 
1) I've checked `getReceiveBufferSize()` and it easily grows to 400KiB, no matter if I configure it explicitly or not
2) I've tested different combinations for `MAX_BUFFERED_RECV_BYTES` and `receiveBufferSize`, and the current 64000 is still the sweet spot for my machine (Apple M1, PostgreSQL in Docker)

---

I will let the dust to settle, and it would be great if somebody else could review it as well.

---

From: "bilalshehata (@bilalshehata)" <[email-masked]>
Date: Thu, 30 Apr 2026 04:09:20 +0000

@vlsi do you need additional support on this one?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 13 May 2026 18:24:09 +0000

I've rebased the PR. Will merge it if the tests pass.