-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Incremental /sync
and /transactions
query events differently #11394
Description
As discovered in #11265 (comment)
/sync
looks for stream_ordering
but excludes all outliers
.
synapse/synapse/storage/databases/main/stream.py
Lines 519 to 527 in b09d90c
sql = """ | |
SELECT event_id, instance_name, topological_ordering, stream_ordering | |
FROM events | |
WHERE | |
room_id = ? | |
AND not outlier | |
AND stream_ordering > ? AND stream_ordering <= ? | |
ORDER BY stream_ordering %s LIMIT ? | |
""" % ( |
Whereas /transactions
(Application service API) only cares about stream_ordering
.
synapse/synapse/storage/databases/main/appservice.py
Lines 358 to 368 in b09d90c
def get_new_events_for_appservice_txn(txn): | |
sql = ( | |
"SELECT e.stream_ordering, e.event_id" | |
" FROM events AS e" | |
" WHERE" | |
" (SELECT stream_ordering FROM appservice_stream_position)" | |
" < e.stream_ordering" | |
" AND e.stream_ordering <= ?" | |
" ORDER BY e.stream_ordering ASC" | |
" LIMIT ?" | |
) |
The behavior of these two endpoints should probably return and push the same events.
Here is the history behind why we added AND not outlier
to the incremental sync endpoint, "Don't return outliers when we get recent events for rooms.", 1505055 but it doesn't explain the why or which interaction creates outlier
events that aren't backfilled
.
What does the spec say?
For /transactions
, the spec doesn't distinguish which events a homeserver should and shouldn't push, https://spec.matrix.org/v1.1/application-service-api/#put_matrixappv1transactionstxnid
For /sync
, there is also nothing so clear cut but it's obvious using the since
pagination query parameter that it should return anything after which equates to stream_ordering
in Synapse land.
Potential solutions
Exclude outliers
in both
Perhaps we should exclude outliers
in /transactions
so they both just match?
@richvdh had some critique about this approach though:
the outlier flag seems like a poor way to decide whether we should push this data. (Yes, outliers shouldn't be sent over
/transactions
, but there are probably many other events which shouldn't be sent).
FederationEventHandler._process_received_pdu
has abackfilled
parameter (seesynapse/handlers/federation_event.py#L945
), whose purpose is slightly unclear, but I think one of its jobs is this sort of thing. Maybe we should use similar logic to that, somehow?-- @richvdh, https://github.com/matrix-org/synapse/pull/11265#discussion_r746523400
But for /transactions
, we can't tell whether the event was backfilled
. The only indication is that the stream_ordering
would be negative which is what the /transactions
code already takes into account.
Only exclude backfilled
events
This means only relying on stream_ordering
.
Then any interaction creating outliers
that we don't want to appear down /sync
//transactions
, should be updated to be also marked as backfilled
.
Dev notes
/sync
stack trace
SyncRestServlet.on_GET
wait_for_sync_for_user
_wait_for_sync_for_user
current_sync_for_user
generate_sync_result
_generate_sync_entry_for_rooms
_get_rooms_changed
get_room_events_stream_for_rooms
get_room_events_stream_for_room
/transactions
stack trace
enqueue_event
_send_request
appservice.scheduler.send
create_appservice_txn
AppServiceTransaction.send
push_bulk
/transactions