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 issue #2215 - handle OIDs >= 2**31 #2217

Merged
merged 1 commit into from Jul 29, 2021

Conversation

MMeent
Copy link
Contributor

@MMeent MMeent commented Jul 20, 2021

Correctly handle the non-signedness of OIDs in relation to signed java integers in this new OID-based type storage.

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:

  • Does this break existing behaviour? If so please explain.
    Previously, OIDs >= 2**31 could not be handled by the driver. This was exposed in 42.2.23, when OIDs became the primary source of truth about type information (before that version this was the type string).

  • Have you added an explanation of what your changes do and why you'd like us to include them?
    This patch fixes the bugged behaviour of 42.2.23, and other misbehaviours with int-typed OIDs by first casting the oids to positive Long values, such that the full OID space is represented in integer values, and can be handled correctly.

  • Have you written new tests for your core changes, as applicable?

  • Have you successfully run tests with your changes locally?

@MMeent MMeent force-pushed the fix-issue-2215 branch 2 times, most recently from 3c88ab0 to 2c6b37a Compare July 20, 2021 16:35
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2021

Codecov Report

Merging #2217 (67fc988) into master (67fc988) will not change coverage.
The diff coverage is n/a.

❗ Current head 67fc988 differs from pull request most recent head adde580. Consider uploading reports for the commit adde580 to get more accurate results

@@            Coverage Diff            @@
##             master    #2217   +/-   ##
=========================================
  Coverage     70.75%   70.75%           
  Complexity     4383     4383           
=========================================
  Files           200      200           
  Lines         18394    18394           
  Branches       3021     3021           
=========================================
  Hits          13014    13014           
  Misses         3977     3977           
  Partials       1403     1403           

@MMeent
Copy link
Contributor Author

MMeent commented Jul 21, 2021

I can't fully validate this PR because I don't have a system on-hand that has had 2**31 OIDs allocated.

However, the paths that got updated in #1949 to use OIDs should now be able to correctly handle OIDs > 2**31 - 1, so we should now be able to connect to databases which have such large OIDs without the driver crashing on those large values.

@davecramer
Copy link
Member

Hmm... I haven't looked fully at the patch, but can't we fake it somehow to create an oid greater than 2**31?

@MMeent
Copy link
Contributor Author

MMeent commented Jul 21, 2021

seeing that we use pg_catalog.pg_type instead of pg_type in getSQLTypeQuery, I don't think we can shadow that table to fake an oid greater than 2**31.

Other than that, OIDs are system-assigned, so to guarantee such high values for OIDs we'd need to create somewhere in the order of 2**31 objects, which would take a long time.

@davecramer
Copy link
Member

select 3539503657::oid;
oid

3539503657

seems to work

@MMeent
Copy link
Contributor Author

MMeent commented Jul 21, 2021

Yes, getSQLType(3539503657) is a path we can still test, but we can't put 3539503657::oid in the path that broke for #2215 without generating somewhere near 2**31 entries in the pg_catalog.pg_type table or otherwise manipulating the postgresql clusters' oid counter.

@davecramer
Copy link
Member

Yes, getSQLType(3539503657) is a path we can still test, but we can't put 3539503657::oid in the path that broke for #2215 without generating somewhere near 2**31 entries in the pg_catalog.pg_type table or otherwise manipulating the postgresql clusters' oid counter.

This is still of value to make sure that we are properly decoding an unsigned int

@jorsol jorsol added the triage/backport? This PR may be backported to a previous version label Jul 26, 2021
@MMeent MMeent force-pushed the fix-issue-2215 branch 2 times, most recently from d423415 to 20e02b0 Compare July 26, 2021 10:30
@MMeent MMeent changed the title Potentially fix issue 2215 Fix issue #2215 - handle OIDs >= 2**31 Jul 26, 2021
Correctly handle the non-signedness of OIDs in relation to signed java integers in the new OID-based type cache.

Previously, this would cause 'bad value for type int'-errors, now this is correctly handled both ways in this part of the code.
@davecramer
Copy link
Member

As soon as this passes I will push it, I will also backpatch it into the 42.2.x branch

@MMeent
Copy link
Contributor Author

MMeent commented Jul 28, 2021

I'm quite suprised with this failure in appveyor, as it is in a part of the code I haven't touched.

The error that was thrown was not INVALID_PASSWORD("28P01"), but CONNECTION_UNABLE_TO_CONNECT("08001"). I wouldn't know why only this one connection failed while the rest had no issues, and I haven't been able to reproduce this issue locally against pg10.

@davecramer
Copy link
Member

I'm not too concerned about appveyor failures at the moment, Although history suggests I should be.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/backport? This PR may be backported to a previous version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants