Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Use native UPSERTs where possible #4306

Merged
merged 49 commits into from
Jan 24, 2019
Merged

Use native UPSERTs where possible #4306

merged 49 commits into from
Jan 24, 2019

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Dec 18, 2018

No description provided.

@hawkowl hawkowl changed the title Use native UPSERTs Use native UPSERTs where possible Dec 18, 2018
@hawkowl
Copy link
Contributor Author

hawkowl commented Dec 18, 2018

Fixes #4015 (on recent postgreses)

@codecov-io
Copy link

codecov-io commented Dec 18, 2018

Codecov Report

Merging #4306 into develop will increase coverage by 1.13%.
The diff coverage is 81.39%.

@@            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

Copy link
Member

@richvdh richvdh left a 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...

@richvdh richvdh requested a review from a team January 18, 2019 16:09
synapse/storage/engines/__init__.py Show resolved Hide resolved
"""
Can we use native UPSERTs? This requires PostgreSQL 9.5+.
"""
return self._version >= 90500
Copy link
Member

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

Copy link
Contributor Author

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?

synapse/storage/engines/sqlite.py Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
.coveragerc Show resolved Hide resolved
MANIFEST.in Outdated Show resolved Hide resolved
synapse/storage/_base.py Show resolved Hide resolved
synapse/storage/_base.py Outdated Show resolved Hide resolved
Copy link
Member

@erikjohnston erikjohnston left a 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.

synapse/storage/_base.py Show resolved Hide resolved
synapse/storage/_base.py Outdated Show resolved Hide resolved
synapse/storage/user_directory.py Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants