Use receipts event_stream_ordering column, remove joins to events
#13918
Conversation
Fizzadar
force-pushed
the
use-receipts-stream-ordering
branch
2 times, most recently
from
fe25007 to
95e77d7
Compare
|
Worth noting that we are running this in our synapse install and it has yielded a significant reduction to databse IOPs when generating push actions in large rooms. |
|
@Fizzadar is this worth revisiting now that it has been several releases? |
|
@clokep yes, is on my list! However it's actually a bit blocked currently by receipts for unknown event IDs which I'm just starting to investigate on our end (starting point: beeper/synapse@21964cf). |
Fizzadar
force-pushed
the
use-receipts-stream-ordering
branch
3 times, most recently
from
1379742 to
5e0c976
Compare
This removes a bunch of joins against the events table.
This should be impossible, check is here to pevent bad actors from breaking synapse with receipts for non-events.
Fizzadar
force-pushed
the
use-receipts-stream-ordering
branch
from
5e0c976 to
5eb0ef2
Compare
| # Now, pretend that we receive a large burst of read receipts (300 total) that | ||
| # all come in at once. | ||
| for i in range(300): | ||
| for _ in range(300): | ||
| room_id = self.helper.create_room_as( | ||
| self.local_user, tok=self.local_user_token | ||
| ) | ||
| resp = self.helper.send(room_id, tok=self.local_user_token) | ||
| event_id = resp["event_id"] | ||
|
|
There was a problem hiding this comment.
Creating a room in between each read receipt does a lot of stuff, including creating new events. I'm not sure if this test is still testing the intended scenario.
There was a problem hiding this comment.
Agreed, this is also very slow, not sure how to proceed here. The original test specifies a unique room/event per receipt so I was trying to replicate that...
squahtx
added
the
X-Awaiting-Changes
I think this may cause issues with rotation of event push actions -> event push summaries. Wouldn't break anything user-facing but would impact the database. I think best bet here is to force through the migration as was done here. Will address this first and re-request review then. |
|
@Fizzadar Any interest in continuing this? I think the unknown events for receipts bit was fixed now too? |
|
@clokep yes and yes I believe so! Hoping to loop back on this later this week time permitting! |
# Conflicts: # tests/rest/client/test_receipts.py
|
I've merged in the latest develop. I don't think the migration needs to be forced because push summary rotation iterates over receipts using stream ID, and sufficient time has passed since the column was populated which will mean anything being rotated now will have the value. The DB compat version (72 at original PR time) is now 80 so there's no concerns with downgrades. So I think the only outstanding thing here is the heavy test code, I'll see what I can do there and re-request review. |
Follow up to: cdbb641
This relies on the completion of the background task introduced by the above change. I suggest that we leave this in draft for N(?) releases and then force the migration similar to da41a7c#diff-ddb3c5844daede09acca491cc04a3740730179fca1fa75aa25b921727209947a, probably with an appropriate upgrade note at the time.
Signed off by Nick @ Beeper (@Fizzadar).
Pull Request Checklist
(run the linters)