Sliding Sync: Get `bump_stamp` from new sliding sync tables because it's faster #17658

MadLittleMods · 2024-09-03T23:20:41Z

Get bump_stamp from new sliding sync tables which should be faster (performance) than flipping through the latest events in the room.

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct
(run the linters)

MadLittleMods · 2024-09-03T23:26:26Z

synapse/storage/databases/main/events.py

+        # `stream_ordering` should be assigned for persisted events
+        assert max_stream_ordering is not None
+        # Check if the event is a backfilled event (with a negative `stream_ordering`).
+        # If one event is backfilled, we assume this whole batch was backfilled.
+        if max_stream_ordering < 0:
+            # We only update the sliding sync tables for non-backfilled events.
+            return


This is unrelated to the change to using new tables or the bugfix.

Just re-arranging to be a little more sane. Should have no functional difference from before.

MadLittleMods · 2024-09-03T23:27:40Z

synapse/handlers/sliding_sync/__init__.py

+            # We can quickly query for the latest bump event in the room using the
+            # sliding sync tables.
+            latest_room_bump_stamp = await self.store.get_latest_bump_stamp_for_room(
+                room_id
            )


Here is the new functionality that this PR is introducing. We're fetching the bump_stamp from the new sliding sync tables which should be faster than flipping through the latest events in the room.

MadLittleMods · 2024-09-03T23:30:01Z

synapse/storage/databases/main/events.py

+                    # 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, so
+                    # instead just leave it as `None` in the table and we will use their
+                    # membership event position as the bump event position in the
+                    # Sliding Sync API.
+                    if new_bump_event_pos.stream > 0:
+                        bump_stamp_to_fully_insert = new_bump_event_pos.stream


Had to add this bug fix here because now that we're using the new tables, the rest-layer tests would fail with the previous flawed logic storing the negative backfilled stream_ordering value (tests.rest.client.sliding_sync.test_rooms_meta.SlidingSyncRoomsMetaTestCase_new.test_rooms_bump_stamp_backfill).

I also added a new test, test_joined_room_bump_stamp_backfill, to do the same thing but just make sure our database tables are correct.

erikjohnston · 2024-09-05T11:14:26Z

synapse/handlers/sliding_sync/__init__.py

+                # `SCHEMA_COMPAT_VERSION` and run the foreground update for
+                # `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots`
+                # (tracked by https://github.com/element-hq/synapse/issues/17623)
+                await self.store.have_finished_sliding_sync_background_jobs()


Don't we need to check this for the second clause too?

We only need it for the first clause.

If the value is None, we can only trust it if the tables are reliable. If the background updates haven't finished yet, we can't tell None as a valid value from the row just not being there yet.

But if we have some value in the table, we can use it (second clause).

Instead of using simple_select_one_onecol, we could use simple_select_one and return an Optional[Tuple[int]] to be able to make this distinction. It's just a bit obtuse and we probably want it the way it's currently written after the gradual migration anyway with the few tweaks already called out.

synapse/handlers/sliding_sync/__init__.py

erikjohnston · 2024-09-09T08:56:15Z

synapse/handlers/sliding_sync/__init__.py

+        if room_membership_for_user_at_to_token.membership == Membership.JOIN:
+            # We can quickly query for the latest bump event in the room using the
+            # sliding sync tables.
+            latest_room_bump_stamp = await self.store.get_latest_bump_stamp_for_room(


As a separate PR maybe: we should also check in timeline for events that are bump events and use the stream ordering from that.

…ng-sync-bump-stamp-from-new-tables

MadLittleMods · 2024-09-09T16:07:31Z

Thanks for the review and merge @erikjohnston 🐠

MadLittleMods added 3 commits September 3, 2024 17:49

Get bump_stamp from new sliding sync tables

d0a6e2f

Better check

d24eb32

Add test to ignore backfilled events for bump_stamp

b5ea1e2

MadLittleMods added the A-Sync label Sep 3, 2024

MadLittleMods changed the title ~~Sliding Sync: Get bump_stamp for new sliding sync tables~~ Sliding Sync: Get bump_stamp from new sliding sync tables Sep 3, 2024

MadLittleMods added 2 commits September 3, 2024 18:22

Add changelog

7830b7a

Fix lints

c8103d9

MadLittleMods commented Sep 3, 2024

View reviewed changes

MadLittleMods marked this pull request as ready for review September 4, 2024 00:18

MadLittleMods requested a review from a team as a code owner September 4, 2024 00:18

MadLittleMods changed the title ~~Sliding Sync: Get bump_stamp from new sliding sync tables~~ Sliding Sync: Get bump_stamp from new sliding sync tables because it's faster Sep 4, 2024

erikjohnston reviewed Sep 5, 2024

View reviewed changes

MadLittleMods mentioned this pull request Sep 5, 2024

Fix bump stamp for non-joined rooms #17674

Merged

MadLittleMods requested a review from erikjohnston September 5, 2024 17:53

erikjohnston approved these changes Sep 9, 2024

View reviewed changes

erikjohnston and others added 2 commits September 9, 2024 15:18

Update synapse/handlers/sliding_sync/__init__.py

523ecbc

Merge remote-tracking branch 'origin/develop' into madlitlemods/slidi…

db31c3b

…ng-sync-bump-stamp-from-new-tables

erikjohnston merged commit e1ed959 into develop Sep 9, 2024
39 checks passed

erikjohnston deleted the madlitlemods/sliding-sync-bump-stamp-from-new-tables branch September 9, 2024 15:41

MadLittleMods mentioned this pull request Sep 9, 2024

Sliding Sync: Look for bump_stamp in the room timeline #17684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sliding Sync: Get `bump_stamp` from new sliding sync tables because it's faster #17658

Sliding Sync: Get `bump_stamp` from new sliding sync tables because it's faster #17658

MadLittleMods commented Sep 3, 2024 •

edited

Loading

MadLittleMods Sep 3, 2024

MadLittleMods Sep 3, 2024 •

edited

Loading

MadLittleMods Sep 3, 2024 •

edited

Loading

erikjohnston Sep 5, 2024

MadLittleMods Sep 5, 2024 •

edited

Loading

erikjohnston Sep 9, 2024

erikjohnston Sep 9, 2024

MadLittleMods commented Sep 9, 2024

Sliding Sync: Get bump_stamp from new sliding sync tables because it's faster #17658

Sliding Sync: Get bump_stamp from new sliding sync tables because it's faster #17658

Conversation

MadLittleMods commented Sep 3, 2024 • edited Loading

Pull Request Checklist

MadLittleMods Sep 3, 2024

Choose a reason for hiding this comment

MadLittleMods Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

erikjohnston Sep 5, 2024

Choose a reason for hiding this comment

MadLittleMods Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

erikjohnston Sep 9, 2024

Choose a reason for hiding this comment

erikjohnston Sep 9, 2024

Choose a reason for hiding this comment

MadLittleMods commented Sep 9, 2024

Sliding Sync: Get `bump_stamp` from new sliding sync tables because it's faster #17658

Sliding Sync: Get `bump_stamp` from new sliding sync tables because it's faster #17658

MadLittleMods commented Sep 3, 2024 •

edited

Loading

MadLittleMods Sep 3, 2024 •

edited

Loading

MadLittleMods Sep 3, 2024 •

edited

Loading

MadLittleMods Sep 5, 2024 •

edited

Loading