pgjdbc/pgjdbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
24+ messages / 3 participants
[nested] [flat]
* [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 07:00 "bilalshehata (@bilalshehata)" <[email protected]>
0 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 07:00 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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?
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 07:08 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 07:08 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 08:34 ` "vlsi (@vlsi)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: vlsi (@vlsi) @ 2026-04-09 08:34 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java:917)
Nice catch
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 09:05 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 09:05 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@vlsi could you Execute CI on this branch when able :)
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 09:40 ` "davecramer (@davecramer)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: davecramer (@davecramer) @ 2026-04-09 09:40 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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?
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 10:19 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 10:19 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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?
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 10:52 ` "vlsi (@vlsi)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: vlsi (@vlsi) @ 2026-04-09 10:52 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 11:26 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 11:26 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 13:04 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 13:04 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 13:11 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 13:11 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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)
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-09 23:33 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-09 23:33 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer @vlsi could i get a another CI run
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-10 07:01 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-10 07:01 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-10 07:17 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-10 07:17 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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 🍀
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-10 07:37 ` "vlsi (@vlsi)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: vlsi (@vlsi) @ 2026-04-10 07:37 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Disable batching for simple query would effectively revert batching to non-batching, thus it would hurt performance.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-10 07:40 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-10 07:40 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> 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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-10 08:00 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-10 08:00 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-10 08:43 ` "vlsi (@vlsi)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: vlsi (@vlsi) @ 2026-04-10 08:43 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
>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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-10 09:43 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-10 09:43 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> 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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-11 00:27 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-11 00:27 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer @vlsi , let me know if there is anything else remaining for approval. Thanks
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-12 04:58 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-12 04:58 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@vlsi gentle bump on this one when you get the chance
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-16 05:53 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-16 05:53 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer @vlsi , if you could let me know if anything is remaining on the PR
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-19 08:13 ` "vlsi (@vlsi)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: vlsi (@vlsi) @ 2026-04-19 08:13 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-04-30 04:09 ` "bilalshehata (@bilalshehata)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: bilalshehata (@bilalshehata) @ 2026-04-30 04:09 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@vlsi do you need additional support on this one?
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches
@ 2026-05-13 18:24 ` "vlsi (@vlsi)" <[email protected]>
22 siblings, 0 replies; 24+ messages in thread
From: vlsi (@vlsi) @ 2026-05-13 18:24 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
I've rebased the PR. Will merge it if the tests pass.
^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2026-05-13 18:24 UTC | newest]
Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-09 07:00 [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-09 07:08 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-09 08:34 ` "vlsi (@vlsi)" <[email protected]>
2026-04-09 09:05 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-09 09:40 ` "davecramer (@davecramer)" <[email protected]>
2026-04-09 10:19 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-09 10:52 ` "vlsi (@vlsi)" <[email protected]>
2026-04-09 11:26 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-09 13:04 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-09 13:11 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-09 23:33 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-10 07:01 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-10 07:17 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-10 07:37 ` "vlsi (@vlsi)" <[email protected]>
2026-04-10 07:40 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-10 08:00 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-10 08:43 ` "vlsi (@vlsi)" <[email protected]>
2026-04-10 09:43 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-11 00:27 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-12 04:58 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-16 05:53 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-04-19 08:13 ` "vlsi (@vlsi)" <[email protected]>
2026-04-30 04:09 ` "bilalshehata (@bilalshehata)" <[email protected]>
2026-05-13 18:24 ` "vlsi (@vlsi)" <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox