Sliding Sync: Use stream_ordering based timeline pagination for incremental sync

[MadLittleMods]

Use stream_ordering based timeline pagination for incremental /sync in Sliding Sync. Previously, we were always using a topological_ordering but we should only be using that for historical scenarios (initial /sync, newly joined, or haven't sent the room down the connection before).

This is slightly different than what the spec suggests

Events are ordered in this API according to the arrival time of the event on the homeserver. This can conflict with other APIs which order events based on their partial ordering in the event graph. This can result in duplicate events being received (once per distinct API called). Clients SHOULD de-duplicate events based on the event ID when this happens.

But we've had a discussion below in this PR and this matches what Sync v2 already does and seems like it makes sense. Created a spec issue matrix-org/matrix-spec#1917 to clarify this.

Related issues:

• Clarify order of initial vs incremental /sync matrix-org/matrix-spec#1917
• Consider changing /messages to use stream ordering for events matrix-org/matrix-spec#852
• MSC4033: Explicit ordering of events for receipts matrix-org/matrix-spec-proposals#4033

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]

As an alternative, we could adapt paginate_room_events(...) to conditionally use topological_ordering vs stream_ordering

[MadLittleMods]

Since we seem to need to reverse this everywhere it's used, we could just make it the default. To not create surprises, I tried to align with what paginate_room_events(...) currently does.

[MadLittleMods]

There is a possible question of, do we even want to do this?

Using stream_ordering makes sense in terms of /sync returning new items that the server received. it's also what the spec says we should do. We also make sure that if a new event is received, it's returned to the client. Otherwise, it seems like there is an edge case if we use topological_ordering that a new event might not be returned because it's topological position is calculated to be older.

Using topological_ordering makes sense in terms of matching /messages so people don't have to worry about different orders of things between API's.

[erikjohnston]

Yeah, so the way that sync v2 works in synapse currently is:

1. The first time a room is sent down /sync we use paginate_room_events to get a chunk of history for the client, this basically results in the same as if no history was sent down /messages was used.
2. We use get_room_events_stream_for_rooms to get all updates for rooms since the last /sync, which is in stream ordering.

I think this is probably the right way of looking at it, we use topological ordering when we paginate backwards, but stream ordering to fetch new updates to the room (even if those updates happened a while ago in the past).

What we probably want to do for sliding sync is to more or less replicate that behaviour: when we have an incremental fetch all updates (i.e. get_room_events_stream_for_rooms for joined rooms and fetch all membership changes), and then only use paginate_room_events for the new rooms.

(This also has the advantage we'd only need to sort and filter rooms we know have updates)

[MadLittleMods]

Sounds pretty reasonable to me 👍

I've created a spec issue to track this: matrix-org/matrix-spec#1917

[erikjohnston]

to_key is never none?

[MadLittleMods]

Makes sense but probably should just update the signature to what get_room_events_stream_for_room(...) uses where it can be None

[MadLittleMods]

Updated and aligned ✅

[erikjohnston]

Thinking about this: we should enforce that to_key is not none when we're going forwards, as we always need to have an upper bound when querying the events stream (as otherwise we'll potentially pick up events that are not fully persisted).

We could do this in the type system, but probably easier to doc and assert it

[MadLittleMods]

I don't think we should do that in this PR (some future task) given that it looks like we don't actually do this in practice yet. For example /messages?dir=f -> get_messages() will call paginate_room_events_by_topological_ordering(from_key=xxx, to_key=None, Direction.FORWARDS). Making this change would require some more default tokens in various places that should have their own PR to think through.

Added some FIXME comments

[erikjohnston]

Oh lord really? Bleurgh, then fair.

[erikjohnston]

Yeah, so the way that sync v2 works in synapse currently is:

1. The first time a room is sent down /sync we use paginate_room_events to get a chunk of history for the client, this basically results in the same as if no history was sent down /messages was used.
2. We use get_room_events_stream_for_rooms to get all updates for rooms since the last /sync, which is in stream ordering.

I think this is probably the right way of looking at it, we use topological ordering when we paginate backwards, but stream ordering to fetch new updates to the room (even if those updates happened a while ago in the past).

What we probably want to do for sliding sync is to more or less replicate that behaviour: when we have an incremental fetch all updates (i.e. get_room_events_stream_for_rooms for joined rooms and fetch all membership changes), and then only use paginate_room_events for the new rooms.

(This also has the advantage we'd only need to sort and filter rooms we know have updates)

[erikjohnston]

We should keep these outside, as if we hit the code paths we will save getting a DB connection and switching threads

[MadLittleMods]

Should we have it in both places? I moved it because we have plenty of places that use _paginate_room_events_by_topological_ordering_txn(...) directly but don't have the checks outside

(currently updated with the checks in both places)

[erikjohnston]

Yeah, having in both is fine, though ideally we'd always try and catch it before we went on a DB thread

[erikjohnston]

Thinking about this: we should enforce that to_key is not none when we're going forwards, as we always need to have an upper bound when querying the events stream (as otherwise we'll potentially pick up events that are not fully persisted).

We could do this in the type system, but probably easier to doc and assert it

[MadLittleMods]

Thanks for the review @erikjohnston 🐓