Sliding Sync: Speed up background updates to populate Sliding Sync tables#17676
Sliding Sync: Speed up background updates to populate Sliding Sync tables#17676MadLittleMods wants to merge 26 commits into
Conversation
…und-updates Conflicts: synapse/storage/databases/main/events_bg_updates.py
…e_upsert_many_txn(...)`
| can_use_last_snapshot = False | ||
| # Once we find one relevant state event that changed, no need to | ||
| # look any further | ||
| break |
There was a problem hiding this comment.
There is potential for a further optimization of using what we can from the last snapshot and fetching what's changed on top.
| # they can't be confused. | ||
| *, | ||
| insertion_value_names: Collection[str] = [], | ||
| insertion_value_values: Collection[Iterable[Any]] = [], |
There was a problem hiding this comment.
Adding support for insertion only values on upsert.
Same as we already have for simple_upsert(insertion_values={})
…und-updates Conflicts: synapse/storage/databases/main/events_bg_updates.py
…ream_ordering` is nullable And seems to actually have `null` values in it
…und-updates Conflicts: synapse/storage/databases/main/events_bg_updates.py tests/storage/test_sliding_sync_tables.py
| WHERE (c.room_id, c.user_id) > (?, ?) | ||
| AND (m.user_id IS NULL OR c.event_id != m.membership_event_id) | ||
| ORDER BY c.room_id ASC, c.user_id ASC | ||
| ORDER BY c.room_id ASC, e.stream_ordering ASC, c.user_id ASC |
There was a problem hiding this comment.
Does this still allow the background update to finish when there are too many members in a room to fit in a single batch?
We keep track of progress in the initial_phase on last_room_id/last_user_id. Given that each stream_ordering position will only correspond to a single membership/user_id, I think this works fine. I think this probably works even if we drop the c.user_id ASC
There was a problem hiding this comment.
I don't think we can do this, as this sorting must match the "bounds" we're using (i.e. (c.room_id, c.user_id) > (?, ?)), otherwise we could skup users, etc.
| "Current state was updated after we gathered it to update " | ||
| + "`sliding_sync_joined_rooms` in the background update. " | ||
| + "Raising exception so we can just try again." | ||
| ) |
There was a problem hiding this comment.
If current state has changed, won't we have updated the tables with the new information in the persist events path? If so we can just skip this room since we know its now up to date?
One thing we have to be careful of is if a background update fails 5 times in a row we will stop retrying the update.
| WHERE (c.room_id, c.user_id) > (?, ?) | ||
| AND (m.user_id IS NULL OR c.event_id != m.membership_event_id) | ||
| ORDER BY c.room_id ASC, c.user_id ASC | ||
| ORDER BY c.room_id ASC, e.stream_ordering ASC, c.user_id ASC |
There was a problem hiding this comment.
I don't think we can do this, as this sorting must match the "bounds" we're using (i.e. (c.room_id, c.user_id) > (?, ?)), otherwise we could skup users, etc.
| to_token=RoomStreamToken( | ||
| stream=membership_event_stream_ordering, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
I think this could a large range of stuff. I think instead of looking at the state deltas, instead we can just look at the topological ordering of the info in room_stats_state and compare that to the membership info. i.e. if the room name was set at topological_ordering 50 then we can assume that any membership events with a higher topological ordering can reuse that room name.
I'm thinking of something like: "take max of topological ordering across the current state events for the state we care about, then compare against each topological ordering". We don't need this logic to be 100% accurate, as a) if we pull out the state groups for more events than strictly necessary that's fine, and b) there is a small edge case where this picks up a room name that had been state reset (but I think we can live with that). However, this will be a lot faster than trying to pull out loads of state deltas.
There was a problem hiding this comment.
I'm actually wondering if we should just skip calculating these values for joins, and have the "joined rooms" background update fill these values out when it gets to them? 🤔
|
Closing as this never landed and I'm unlikely to put any more effort into improving this. This PR stalled after |
2 participants
Speed up background updates to populate Sliding Sync tables
Follow-up to #17512
room_stats_statetable forroom_type,name, andis_encryptedconcurrently_executeprocessing and insertingsimple_upsert_many_txnwhere we canDev notes
The quick and dirty script that Erik wrote to run on
matrix.org, https://github.com/erikjohnston/synapse-fast-bg-update/blob/main/src/main.rsPull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)