-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Fixes #4015 (on recent postgreses) |
Codecov Report
@@ Coverage Diff @@
## develop #4306 +/- ##
==========================================
+ Coverage 73.66% 74.8% +1.13%
==========================================
Files 302 336 +34
Lines 29822 33997 +4175
Branches 4896 5527 +631
==========================================
+ Hits 21969 25432 +3463
- Misses 6421 7001 +580
- Partials 1432 1564 +132 |
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.
looks plausible but since there are conflicts and test failures I assume you're still working on it...
""" | ||
Can we use native UPSERTs? This requires PostgreSQL 9.5+. | ||
""" | ||
return self._version >= 90500 |
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.
This is going to blow up if called before on_new_connection
. This is probably unlikely, but it may be worth defaulting it to something like 0
. Or have a _can_do_native_upsert
parameter set to False
by default instead, and have on_new_connection
update it when its called
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.
This won't ever be called before we're actually connected, since it's in transaction code?
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.
Woo, this looks like it'll work! Just some small nits.
It's a shame that we can't get rid of the return value of _simple_upsert
. The only way I can think of is to implement a version of _simple_upsert
that lets you specify generic expressions, rather than simply key/value pairs, but there's probably not much point if we're going to drop support for 9.4 soon.
No description provided.