-
Notifications
You must be signed in to change notification settings - Fork 522
Support psycopg as a PostgreSQL driver #18999
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
Open
clokep
wants to merge
47
commits into
element-hq:develop
Choose a base branch
from
clokep:psycopg3
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 14 commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
ecb20a4
Add psycopg package.
clokep c7e3120
Method to set statement timeout.
clokep 91ef287
Separate PostgresEngine into Psycopg2Engine and PsycopgEngine.
clokep 0a06be1
Run tests in CI against psycopg.
clokep 260a5b7
Update user directory to handle psycopg3.
clokep ce8ad96
Support execute_values on psycopg3.
clokep 93b1740
Fix-up simple_* tests.
clokep d49827d
Fix-up calls to end_to_end_keys.
clokep 287f0a6
Use superclass version of `executescript()` (#1)
realtyem 9f77ac4
Switch out formatting placeholder for what psycopg2 is expecting (#2)
realtyem 3bbd562
Merge remote-tracking branch 'upstream/develop' into psycopg3
clokep f5b6429
Linting (and a fix) (#3)
realtyem 7ff4584
Merge remote-tracking branch 'refs/remotes/origin/psycopg3' into psyc…
clokep 5353f8d
Try running complement builds?
clokep 1bec3d7
Merge branch 'develop' into psycopg3
realtyem f6c2364
Adjust type: ignore line to where mypy will apply it
realtyem 279791d
Add both PsycopgEngine and Psycopg2Engine to database.engines.__all__…
realtyem 1ceb332
Adjust _mark_state_groups_as_pending_deletion_txn() to use execute_ba…
realtyem 95eb7f8
Adjust set_profile_field() insertion sql for a narrower type on param…
realtyem 4ad733f
Try an update to the config schema?
realtyem f65a885
Adjust unit tests to reflect updated minimal versions of python/postgres
realtyem 425971f
Minor changes to profile field
clokep 61a3aaa
Merge remote-tracking branch 'upstream/develop' into psycopg3
clokep f31d8d2
Newsfragment
clokep ad229a9
Merge remote-tracking branch 'upstream/develop' into psycopg3
clokep 2f4352b
poetry lock again
clokep 467a8c4
Merge branch 'develop' into psycopg3
clokep e59ea08
Merge branch 'develop' into psycopg3-adjust-settings
realtyem d9226e0
Update poetry.lock file
realtyem 0693794
linting fixups
realtyem dcc4b5a
Merge pull request #4 from realtyem/psycopg3-adjust-settings
clokep c195ee6
Merge branch 'develop' into jason/psycopg-merge-develop
jason-famedly 99ff2ac
Adjust for execute_values() usage with fetch=False to only be appropr…
jason-famedly 698a56d
Attempt forcing a lower case comparison to "psycopg" in complement.sh
jason-famedly 088ead3
Add mini docstrings to the abstract postgres engine class, as well as…
jason-famedly 123aeac
I forgot that the new driver is not wired up in the complement script…
jason-famedly 680ffee
Merge pull request #6 from jason-famedly/jason/psycopg-merge-develop
clokep 4cd7924
Use the 'c' extra instead of the 'pure python' version for C-compiled…
jason-famedly a320d8a
Update 'c' extra to 3.2.8 as a minimum. This avoids olddebian's packa…
jason-famedly d94d4c0
Allow complement to be ran with WORKERS=1 and still allow the psycopg…
jason-famedly 73415a2
Merge pull request #7 from jason-famedly/jason/adjust-for-feedback
clokep 0e083b1
Merge branch 'develop' into jason/update-psycopg3
jason-famedly 63c7f03
update poetry lock
jason-famedly 0af6799
Merge branch 'develop' into jason/update-psycopg3
jason-famedly 5c4f8a9
Merge pull request #8 from jason-famedly/jason/update-psycopg3
clokep efcb28c
Merge branch 'develop' into jason/update-poetry-lock
jason-famedly 59cd733
Merge pull request #9 from jason-famedly/jason/update-poetry-lock
clokep File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
judging from the CI yaml this needs to be case-insensitive (to match
Psycopgfrom the CImatrix.database); or am I wrong?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine you are correct. I think I was using it from the command line and tested with it's lower case function. I will adjust to use the bash
"${POSTGRES,,}}"pattern instead, which should lower case all the lettersThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually opened a can of worms. You were correct to bring this up, as it turned out that Complement in CI was not using this driver for two reasons:
WORKERS=1mode 🤦♂️ which was how the example testing was set uppsycopgin it's "pure python" installation method without significant changes, that I am not willing to attempt at this time. After 4 hours of exploring how to make this work I came to the conclusion that the easiest method to move past this is to not use the "pure python" installation but use either the "binary" or "c" installations instead. Neither of these have the same symptoms(these other installation types have the necessary libraries bundled with them)some more details about point 2:
This was manifesting asWhich suggested that the module of
psycopgwas not even installed. This module does appear during thepipinstallation logs in CI, so was a slight misdirection.I then removed the exception catch for the importing of
PsycopgEnginehoping it would give me more information. It did.Inspection of the functions being called here(and some minor sleuthing in the
psycopgissues page) led me to the conclusion that thectypes.util.find_library("pq")was not finding what it wanted, namely the system level library oflibpq5. This is installed in the base docker image, and does appear to be in the filesystem at the correct place in the final complement image. But was not found by this function anyway. Using a roughapt install -y libpq5in the final complement image did allow it to work. But why was not found even though it was present? The issues tracker onpsycopgdid suggest that it could have also been caused by passing around data between distro-less docker images, this does not appear to be the case here at any point.Since the next stages of introducing
psycopgsupport to Synapse will be changing the installation method to eitherbinaryorcanyway, spending time to figure out why this is not working seems like a waste of time. This is the easy solution, it just means the time table gets moved up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After much testing and some debate, the
cextra was chosen. Thebinaryextra does not have a source distribution(only wheels) and building debian packages was failing(this appeared to be because the bundles libs were not being found, even though they were verifiably in the correct location. I suspected this was aRPATHissue).The minimum version of
psycopgchosen was originallyv3.1.x. With thecextra this was causing the 'old deps' trial to fail. Bookworm packages a copy oflibpq5that was updated to support Postgres 18, but also brokepsycopgprior tov3.2.8. Since we control the version ofpsycopgbut notlibpq5, the minimum version ofpsycopgwas bumped to this version