Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add local_current_membership table #6655

Merged
merged 11 commits into from
Jan 15, 2020
Merged

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented Jan 7, 2020

The idea here is to track membership state for local users separately from current_state_events, as currently synapse may delete the contents of current_state_events for that room if its no longer joined to that room (e.g. if the server tries and rejoins and fails). This breaks things like the leave section in /sync responses and exporting user data.

While that is a bit of an edge case, I do plan on changing synapse to delete the contents of current_state_events when the server leaves the room (to make it easy to answer the question "does the server share a room with a given remote user", and generally to remove footguns), which would then properly break those features.

Two things:

  1. The initial insertion of local_current_membership happens in the delta script, which means it'll block start up. On a test on matrix.org this took ~40s, which is a while but not the end of the world. Doing it synchronously means we don't have to keep around various iterations of functions depending on the state of the various background updates, which got a bit fiddly.

  2. There is a bug where if a user gets state reset out of a room that room doesn't appear in the leave section of sync (due to us just deleting the row in current_state_events), which I haven't attempted to fix as I don't really know what we want to do there (since we don't have a leave event to pass down to clients).

Note: The renaming of the functions are in a separate commit.

(This is part of the work for #6399)

Currently we rely on `current_state_events` to figure out what rooms a
user was in and their last membership event in there. However, if the
server leaves the room then the table may be cleaned up and that
information is lost. So lets add a table that separately holds that
information.
They get tracked separately from current state events if the server
isn't already in the room.
@erikjohnston erikjohnston requested a review from a team January 7, 2020 17:10
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few questions and suggestions below, but generally seems plausible.

It feels odd though. It feels like we're essentially duplicating a significant part of current_state_events mostly in order to act as a backup for when it gets cleared. Are there not easier ways to solve the problems that we are trying to solve by deleting stuff from current_state_events ?

@erikjohnston
Copy link
Member Author

It feels odd though. It feels like we're essentially duplicating a significant part of current_state_events mostly in order to act as a backup for when it gets cleared. Are there not easier ways to solve the problems that we are trying to solve by deleting stuff from current_state_events ?

We want to answer the question of is the server in a room with a remote user efficiently, basically. (Which is fundamentally trying to solve the problem of figuring out if we need to resync keys for a remote user when they leave/join rooms).

I agree it feels odd, but a) I think not clearing out current_state_events is masking bugs as per PR description and b) its not obvious that having stale data in there isn't causing problems, e.g. get_rooms_for_user is called against remote users in the e2e key code.

Given the above I'm relatively happy that we should be clearing out current_state_events, but how we then track room membership for local users is a more open question. We could only track ban/leaves for rooms the server is no longer in in a separate table to avoid duplication, but not sure if that just makes things more fiddly or not.

We also create the indices at startup so that we can use the table
immediately.
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm now modulo some nits about comments and the merge conflict.

erikjohnston and others added 3 commits January 15, 2020 11:46
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@erikjohnston erikjohnston merged commit 28c98e5 into develop Jan 15, 2020
u1-liquid added a commit to u1-liquid/synapse that referenced this pull request Jan 17, 2020
erikjohnston added a commit that referenced this pull request Jan 17, 2020
Fix #6727
Related #6655

Co-authored-by: Erik Johnston <erikj@jki.re>
@erikjohnston erikjohnston deleted the erikj/local_current_membership branch February 5, 2020 17:35
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '28c98e51f':
  Add `local_current_membership` table (#6655)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants