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 PSQL KeyValueStore implementation for python 2/3 compatibility #365

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Purg
Copy link
Member

@Purg Purg commented Aug 17, 2018

The python instance to psycopg2 binary object conversion was not
operating correctly in python 3. This updates the method to be cross
compatible as well as updates the tests to use cross compatible
accessors.

The python instance to psycopg2 binary object conversion was not
operating correctly in python 3.  This updates the method to be cross
compatible as well as updates the tests to use cross compatible
accessors.
@Purg
Copy link
Member Author

Purg commented Aug 17, 2018

PTAL @wphicks @KyleFromKitware

"""

PICKLE_PROTOCOL_VER = -1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will Python 3 attempt to read objects that have been pickled in Python 2? The documentation makes it seem like this is the case, which would defeat the purpose of changing the protocol number.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A) I just noticed that I didn't try using the fix_imports flag for python 3's pickle. I'll have to try that in my compatibility failures I was seeing.

B) A python 3 instance of PostgresKeyValueStore will attempt to read the data in the table and columns its pointed to, even if that data was written by a python 2 instance of PostgresKeyValueStore. I had tried using a pinned protocol version of 2 (since both python versions understand that version) but was still seeing compatibility issues. This may have been the correct thing to do when I try (A).

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.

2 participants