Message-ID: From: "davecramer (@davecramer)" To: "pgjdbc/pgjdbc" Date: Thu, 09 Apr 2026 09:40:18 +0000 Subject: Re: [pgjdbc/pgjdbc] PR #4014: fix: restore pre-describe for generated-key batches In-Reply-To: References: List-Id: X-GitHub-Author-Login: davecramer X-GitHub-Comment-Id: 4213160134 X-GitHub-Comment-Type: issue_comment X-GitHub-Issue: 4014 X-GitHub-Repo: pgjdbc/pgjdbc X-GitHub-Type: comment X-GitHub-Url: https://github.com/pgjdbc/pgjdbc/pull/4014#issuecomment-4213160134 Content-Type: text/plain; charset=utf-8 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?