support psycopg3 #213
support psycopg3 #213
Conversation
|
Thanks for the stab. Is there a way to wrap that compat into something a little more elegant than a big try and except? I'm not an expert on Python backwards compatibility, but I wonder if there's a common pattern from a big library like Django that we can borrow from. (Maybe something our friend Claude would know?) |
i followed django https://github.com/django/django/blob/d6e0c710173531645bd3689bc03e9b809aa3d220/django/db/backends/postgresql/psycopg_any.py#L4 |
TIL! ;) I'm going to blitz through the repo with a machete and update a bunch of other stale stuff while I'm at it here. |
|
Like the versions we're testing and such. |
that would be helpful, thanks. see my latest error with distutils on py 3.12. |
|
maybe we should switch away from |
I'd be up for this in a separate PR. The main trick is working in those different Django settings tricks. My standard approach for new libraries changed to uv and pyproject.toml a while ago (as seen here). I've been trying to avoid opening that can of worms thus far, but maybe we should. |
|
Looks like I got tests passing, even with 3.13. That's good progress. |
postgres_copy/psycopg_compat.py
Outdated
| if isinstance(destination, TextIOBase): | ||
| decoder = utf8_decoder_cls() | ||
| else: | ||
| decoder = NoopDecoder() | ||
| with cursor.copy(sql, params) as copy: | ||
| while data := copy.read(): | ||
| data = decoder.decode(data) | ||
| destination.write(data) | ||
| # TODO: is this extra one needed? | ||
| if data := decoder.decode(b"", final=True): | ||
| destination.write(data) |
There was a problem hiding this comment.
Can you explain this to me? I'm just ignorant of how the new version works.
There was a problem hiding this comment.
this is all related to the comment on line 15 # psycopg2.copy_expert has special handling of encoding
psycopg2 (but not 3) detects the destination being an "instance" of TextIOBase (https://github.com/psycopg/psycopg2/blob/c2b6a8aaeae71b30a96403bf5d6e7eafc21afaef/psycopg/utils.c#L217) and decodes the data before writing
the entire interface of for copy has change significantly in psycopg3. this pr tries to maintain the previous interface with all these hacks. maybe a better idea would be design a new interface that more closely matches the new copy https://www.psycopg.org/psycopg3/docs/basic/copy.html#copying-block-by-block
but it seems strange for this single library to have different interfaces for different psycopg versions and that makes migration harder for users
postgres_copy/psycopg_compat.py
Outdated
| def copy_from(cursor, sql, source): | ||
| with cursor.copy(sql) as copy: | ||
| while data := source.read(BUFFER_SIZE): | ||
| copy.write(data) |
There was a problem hiding this comment.
Why this buffer size? Can users expect any differences in speed based on how that is set?
There was a problem hiding this comment.
psycopg2 uses 8k by default https://github.com/psycopg/psycopg2/blob/c2b6a8aaeae71b30a96403bf5d6e7eafc21afaef/psycopg/cursor.h#L74 (configurable, though we don't) but i recently came across python/cpython#117151 which bumps the default buffer sizes in cpython so i thought i'd go for that instead.
it is worth noting that i don't have psycopg3 in production yet so i don't have any real benchmarks. if you (or any other reviewer) has has good test sets to benchmark with i'd be interested in feedback
|
@thorntonedgar and @wlorenzetti, do you have any comments or concerns about this implementation? |
postgres_copy/psycopg_compat.py
Outdated
| while data := copy.read(): | ||
| data = decoder.decode(data) | ||
| destination.write(data) | ||
| # TODO: is this extra one needed? |
There was a problem hiding this comment.
this is needed. was going to fixup and squash but maybe best to make more commits and fix history after the review?
There was a problem hiding this comment.
I'm not sure I follow. If you want to make another patch yet before merge, let's get 'er in.
There was a problem hiding this comment.
pushed, though should we wait a bit to give the others a chance to comment before we merge?
There was a problem hiding this comment.
I'd like to give them at least a beat, yeah. if we don't hear today we can revisit tomorrow.
|
I'm going to merge this in a few minutes, but before I do I'm going to experiment with asking Claude to drum up some additional unit tests. Mostly as an experiment for me. I'm not optimistic on how well it will do based on some early local tomfoolery. |
|
How come you want dedicated tests for the compt module? It is already tested by running the rest of the suite with each psycopg version |
|
I'm good with what you've got. It's strictly an experiment to see what AI comes up with. I'm just curious. I haven't got to play too much with these tools. I apologize for the indulgence. |
|
Alrighty. I'm good to merge this. I'm going to hammer in a couple long overdue modernizations before I ship a new version like uv for env management and some mypy static type hinting. I expect to get it shipped today. Thank you very much for this excellent pull request and I'll flag you when I get it out on pypi. |
|
Version 2.8.0 is released. Please give it a shot and let me know if you have any troubles. |
this is a bit fiddly because all the supported use-cases but all the tests pass locally with either psycopg version