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: binary decoding of bool values #2640

Merged
merged 3 commits into from Oct 12, 2022

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 11, 2022

All Submissions:

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

Changes to Existing Features:

  • 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?

Fixes #2639

@davecramer
Copy link
Member

AFAIR there was a good reason why we didn't do this, however I don't recall the exact reason. @vlsi ?

@jorsol
Copy link
Member

jorsol commented Oct 11, 2022

AFAIR there was a good reason why we didn't do this, however I don't recall the exact reason. @vlsi ?

IIRC, there's no benefits for using binary transfer with boolean type, and actually it was slower than the normal transfer.

@davecramer
Copy link
Member

IIRC, there's no benefits for using binary transfer with boolean type, and actually it was slower than the normal transfer.

I guess we should have commented that :)

@davecramer
Copy link
Member

@olavloite do you see any performance benefit to this ?

@olavloite
Copy link
Contributor Author

olavloite commented Oct 12, 2022

@olavloite do you see any performance benefit to this ?

No (haven't checked if there's any difference, but I don't expect any that would be measurable). I ran into this while testing our proxy for using PostgreSQL drivers with Cloud Spanner. One of the things we do is testing that we receive the same values from Cloud Spanner when we use PostgreSQL driver X compared to what we get when we just use the native client library for Cloud Spanner. We do so for both text and binary encoding when the driver supports this. In this case, we ran into an issue with boolean values when using binary encoding, where we only received false values when using the PostgreSQL JDBC driver.

Just to be sure: This PR is not for adding support for binary decoding of boolean values. That is (implicitly) already supported in the JDBC driver. At least: You can configure the driver so it uses the binary format for receiving boolean values. However, if you do so, you will only ever get the value false when you call ResultSet#getBoolean(...). See also #2639

@davecramer davecramer merged commit 56a487c into pgjdbc:master Oct 12, 2022
@olavloite olavloite deleted the fix-binary-decode-bool branch October 13, 2022 07:57
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.

binary bool values are not decoded correctly
3 participants