-
Notifications
You must be signed in to change notification settings - Fork 217
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.
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
Sliding sync: various fixes to background update #17636
Conversation
4faa51f
to
70c6722
Compare
@@ -1887,11 +1889,11 @@ def _find_memberships_to_update_txn( | |||
FROM local_current_membership AS c | |||
INNER JOIN events AS e USING (event_id) | |||
LEFT JOIN rooms AS r ON (c.room_id = r.room_id) | |||
WHERE c.room_id > ? | |||
ORDER BY c.room_id ASC | |||
WHERE (c.room_id, c.user_id) > (?, ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this means the background update needed to be restarted to ensure we didn't drop anything along the way before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or actually, I'm guessing the background update was just stuck on one room because it had more members than the batch size. So it doesn't need to be restarted.
@@ -1993,13 +1995,16 @@ def _find_previous_membership_txn( | |||
WHERE | |||
room_id = ? | |||
AND m.user_id = ? | |||
AND (m.membership = ? OR m.membership = ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the root cause to add this caveat to only check for invite
/knock
?
Perhaps a double-leave event somewhere although Synapse doesn't seem to allow that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was a ban + leave I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh so ban and unban
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented in #17654
@@ -1861,7 +1861,7 @@ def _update_current_state_txn( | |||
VALUES ( | |||
?, ?, ?, ?, ?, | |||
(SELECT stream_ordering FROM events WHERE event_id = ?), | |||
(SELECT instance_name FROM events WHERE event_id = ?) | |||
(SELECT COALESCE(instance_name, 'master') FROM events WHERE event_id = ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I assume this became obvious after running the background update on matrix.org
since this is a NOT NULL
column 😵
…ted -> banned -> unbanned (#17654) Add comment to explain extra case where you can be invited -> banned -> unbanned and we want to be able to find the invite event. Follow-up to #17636 (comment)
Follows on from #17512, other fixes include: #17633, #17634, #17635