This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
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.
Fetch edits for multiple events in a single query #11660
Fetch edits for multiple events in a single query #11660
Changes from 1 commit
b16ce01
4344f10
7e97e55
d4a41c8
e9908c8
7ac2a9d
aacf47e
e79d656
8132180
e00cec0
05c38f9
8400c20
e2f905b
ffad611
5ce3e07
9a7cc1a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Ideally we would be only pull out the latest edit event per original event, but I haven't found a reasonable way to do that (maybe a lateral join, but that's not supported on sqlite). Any thoughts on how to improve this would be appreciated!
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.
Does latest edit mean "edit with largest depth"?
Might we be able to use a trick like https://stackoverflow.com/a/27802817/5252017 ? (Sorry, not an expert here)
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.
It means the largest
origin_server_ts
(which is why we're ordering by that descending.)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.
N.B. SQLite does support lateral joins as long as you don't write the word
LATERAL
— if that's the only thing blocking you, you should be able to work around that by only inserting that word for Postgres.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.
@reivilibre Do you have any reference for that? I've been unable to get it to work (and my searching online has yielded "you can't do this on SQLite").
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.
On Postgres you can do
SELECT DISTINCT ON (original.event_id) ...
which will choose the first row (as defined by theORDER BY
clause). I don't think SQLite has that, and so you would need to use a different query, at which point you may as well fall back to the old behaviour.Potentially you could use window functions and
first_value
, which I think both postgres and sqlite support, but those sorts of queries really are voodoo magic.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.
❤️ @erikjohnston Thank you! That gave me some breadcrumbs to realize we do something very similar elsewhere:
synapse/synapse/storage/databases/main/end_to_end_keys.py
Lines 616 to 639 in 3e0536c
I think we can abstract out the changes in 8400c20, but that is proving a bit tedious / invasive so I'd like to do it separately.
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.
Can we make this class depend on
EventsWorkerStore
instead of ignoring the error?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 can attempt to do that! Note that we have similar ignores all over, see #11165.
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 gives other errors (similar to #11165) about inconsistent MROs. I'm going to leave this to be solved in #11165.
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.
Fair!