Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle ParameterStatus messages in QueryExecutorImpl.receiveFastpathResult #2247

Conversation

strassl
Copy link
Contributor

@strassl strassl commented Sep 9, 2021

QueryExecutorImpl.receiveFastpathResult did not properly handle ParameterStatus messages.
This in turn caused failures for some LargeObjectManager operations.
Fixed by adding the missing code path, based on the existing handling in processResults.

Closes #2237

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Existing Features:

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

…pathResult

QueryExecutorImpl.receiveFastpathResult did not properly handle ParameterStatus messages.
This in turn caused failures for some LargeObjectManager operations.
Fixed by adding the missing code path, based on the existing handling in processResults.

Closes pgjdbc#2237
@davecramer davecramer merged commit 1ca79d4 into pgjdbc:master Sep 10, 2021
@davecramer
Copy link
Member

I'll have to rewrite the test to get it to pass for Java 6 when I backpatch it.

@sehrope
Copy link
Member

sehrope commented Sep 10, 2021

@strassl Nice. Great initial bug report with all the details and logs.

@davecramer You don't have to rewrite the test as the test suite is JDK 8+ already. Only the driver code itself is 6+ and we don't actually run the tests on 6+. We just trust gradle to build a compatible jar.

@davecramer
Copy link
Member

@sehrope good point, thx for watching my back.

@strassl
Copy link
Contributor Author

strassl commented Sep 10, 2021

@sehrope Thanks!

@davecramer I just noticed that I forgot to add a teardown method that closes the connection to the test. I'm not exactly sure if it will cause any issues (depends on how to test suite is set up, I suppose), but it feels dirty to leak connections like this. Should I open a new PR to add it?

@davecramer
Copy link
Member

Hmmm if you extend the BaseTest4 it will do it for you, so feel free to open another PR to fix it, thx

@sehrope
Copy link
Member

sehrope commented Sep 10, 2021

Better to handle the connection creation directly in the test as it mucks with the auto commit. Can do it with a try-with-resources:

  public void testOpenWithErrorAndSubsequentParameterStatusMessageShouldLeaveConnectionInUsableStateAndUpdateParameterStatus() throws Exception {
    try (PgConnection con = (PgConnection) TestUtil.openDB()) {
        String originalApplicationName = con.getParameterStatus("application_name");
        // ... rest of test goes here ...
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

receiveFastpathResult fails if ParameterStatus message is received after ErrorResponse
3 participants