Skip to content

Sliding sync: various fixups to the sliding sync joined room background job #17673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikjohnston merged 5 commits into from
Sep 10, 2024
Merged

Sliding sync: various fixups to the sliding sync joined room background job #17673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b28fbdd
Read from local_current_membership
erikjohnston Sep 5, 2024
1c1925f
Ignore bad events
erikjohnston Sep 5, 2024
bf847e8
Remove stream ordering positive assertion
erikjohnston Sep 5, 2024
d60e55b
Newsfile
erikjohnston Sep 5, 2024
dc680fc
Add comment
erikjohnston Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/17673.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Pre-populate room data used in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint for quick filtering/sorting.
27 changes: 18 additions & 9 deletions synapse/storage/databases/main/events_bg_updates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1595,17 +1595,15 @@ def _txn(txn: LoggingTransaction) -> None:
# starve disk usage while this goes on.
#
# We upsert in case we have to run this multiple times.
#
# The `WHERE TRUE` clause is to avoid "Parsing Ambiguity"
txn.execute(
"""
INSERT INTO sliding_sync_joined_rooms_to_recalculate
(room_id)
SELECT room_id FROM rooms WHERE ?
SELECT DISTINCT room_id FROM local_current_membership
WHERE membership = 'join'
Comment on lines +1602 to +1603

@MadLittleMods MadLittleMods Sep 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you've tested that this is fast enough?

@erikjohnston erikjohnston Sep 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not ideal, but it completes in a few minutes on matrix.org, so should be fast enough everywhere else.

ON CONFLICT (room_id)
DO NOTHING;
""",
(True,),
)

await self.db_pool.runInteraction(
Expand Down Expand Up @@ -1689,7 +1687,15 @@ def _get_rooms_to_update_txn(txn: LoggingTransaction) -> List[Tuple[str]]:
if not current_state_ids_map:
continue

fetched_events = await self.get_events(current_state_ids_map.values())
try:
fetched_events = await self.get_events(current_state_ids_map.values())
except (DatabaseCorruptionError, InvalidEventError) as e:
logger.warning(
"Failed to fetch state for room '%s' due to corrupted events. Ignoring. Error: %s",
room_id,
e,
)
continue

current_state_map: StateMap[EventBase] = {
state_key: fetched_events[event_id]
Expand Down Expand Up @@ -1722,10 +1728,13 @@ def _get_rooms_to_update_txn(txn: LoggingTransaction) -> List[Tuple[str]]:
+ "given we pulled the room out of `current_state_events`"
)
most_recent_event_stream_ordering = most_recent_event_pos_results[1].stream
assert most_recent_event_stream_ordering > 0, (
"We should have at-least one event in the room (our own join membership event for example) "
+ "that isn't backfilled (negative `stream_ordering`) if we are joined to the room."
)
Comment on lines -1725 to -1728

@MadLittleMods MadLittleMods Sep 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is a room where this is possible? We should add a comment in this asserts place that ideally we could assert this but there is actually some case where it happens.

                # If we've just joined a remote room, then the last bump event may
                # have been backfilled (and so have a negative stream ordering).
                # These negative stream orderings can't sensibly be compared, but TODO

We need some value since the event_stream_ordering is a NOT NULL column. Negative works but we avoid doing it in other places since it would send the room to the bottom of the list on the client.

@erikjohnston erikjohnston Sep 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dc680fc


# The `most_recent_event_stream_ordering` should be positive,
# however there are (very rare) rooms where that is not the case in
# the matrix.org database. It's not clear how they got into that
# state, but does mean that we cannot assert that the stream
# ordering is indeed positive.

# Figure out the latest `bump_stamp` in the room. This could be `None` for a
# federated room you just joined where all of events are still `outliers` or
# backfilled history. In the Sliding Sync API, we default to the user's
Expand Down