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

Reintroduce membership tables event stream ordering #15128

Conversation

Fizzadar
Copy link
Contributor

@Fizzadar Fizzadar commented Feb 21, 2023

Reintroduces #14979 with fixes per #15014.

Original description:

This adds an event_stream_ordering column to current_state_events, local_current_membership and room_memberships. Each of these tables is regularly joined with the events table to get the stream ordering and denormalising this into each table will yield significant query performance improvements once used. Includes a background job to populate these values from the events table pushed back for now.

Same idea as #13703 which has been a significant improvement on our database instance, suspect once applied this will be far greater.

Signed off by Nick @ Beeper (@Fizzadar).

Pull Request Checklist

Closes: #15014

Specifically this adds the column to `current_state_events`,
`local_current_membership` and `room_memberships`. Each of these tables
is regularly joined with the `events` table to get the stream ordering
and denormalising this into each table will yield significant query
performance improvements once used.
@Fizzadar Fizzadar changed the title Membership tables event stream ordering redux Reintroduce membership tables event stream ordering Feb 22, 2023
@Fizzadar Fizzadar marked this pull request as ready for review February 22, 2023 08:23
@Fizzadar Fizzadar requested a review from a team as a code owner February 22, 2023 08:23
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

I think we should be bumping SCHEMA_VERSION to 74 here

SCHEMA_VERSION = 73 # remember to update the list below when updating

(with a comment explaining what's changed in version 74), but leave SCHEMA_COMPAT_VERSION at 73.

@DMRobertson DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Feb 27, 2023
@Fizzadar
Copy link
Contributor Author

Argh can't believe I missed this!

@DMRobertson I think this should be bumping the compat version now? Once this is applied the schema cannot be rolled back before 74. 6901682

@Fizzadar Fizzadar requested a review from DMRobertson February 27, 2023 14:15
@DMRobertson
Copy link
Contributor

Err, I think I got SCHEMA_VERSION and SCHEMA_COMPAT_VERSION mixed up. I also completely forgot about #15036. (It seems the local branch I used to generate that link is a little out of date 🤦)

Once this is applied the schema cannot be rolled back before 74

Agreed: as of this change, Synapse needs to make disambiguated queries.

Co-authored-by: David Robertson <david.m.robertson1@gmail.com>
@Fizzadar Fizzadar requested a review from DMRobertson February 27, 2023 16:58
DMRobertson
DMRobertson previously approved these changes Feb 27, 2023
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Looks sane to me, but it might be worth a second pair of eyes since we messed up last time.

synapse/storage/databases/main/events_bg_updates.py Outdated Show resolved Hide resolved
@DMRobertson DMRobertson dismissed their stale review February 28, 2023 11:14

Rich has concerns!

@Fizzadar
Copy link
Contributor Author

Concerns addressed and background update part removed!

@Fizzadar Fizzadar requested a review from richvdh March 16, 2023 20:25
@DMRobertson DMRobertson requested a review from a team March 16, 2023 21:22
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 otherwise

synapse/storage/databases/main/events.py Show resolved Hide resolved
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 other than typo

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@DMRobertson DMRobertson merged commit e6af49f into matrix-org:develop Mar 24, 2023
erikjohnston added a commit that referenced this pull request Mar 29, 2023
Fizzadar added a commit to beeper/synapse-legacy-fork that referenced this pull request Mar 30, 2023
Fizzadar added a commit to Fizzadar/synapse that referenced this pull request Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reintroduce DB changes from #14979, using SCHEMA_COMPAT_VERSION mechanism
4 participants