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

Make synapse_port_db correctly create indexes #6102

Merged
merged 37 commits into from
Oct 23, 2019

Conversation

babolivier
Copy link
Contributor

Make synapse_port_db correctly create indexes in the PostgreSQL database, by having it run the background updates on the database before migrating the data.

To ensure we're migrating the right data, also block the port if the SQLite3 database still has pending or ongoing background updates.

Fixes #4877

@erikjohnston erikjohnston removed the request for review from a team September 26, 2019 10:05
This way we don't have to rely on devs thinking about updating the script each time a new store starts using background updates.
@babolivier babolivier requested a review from a team October 1, 2019 10:20
@babolivier
Copy link
Contributor Author

ftr I'm looking at adding CI to the synapse_port_db script in another PR: #6140

@anoadragon453
Copy link
Member

anoadragon453 commented Oct 1, 2019

Manually tested on a homeserver with a few users and everything seemed to work ok \o/

richvdh
richvdh previously requested changes Oct 2, 2019
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.

So this looks like a good plan, but I have some architectural concerns...

scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
@babolivier babolivier requested a review from a team October 4, 2019 15:26
The StatsStore class hasn't been split because it's tied to a lot of code, including code outside of its own file (such as get_event()), and doesn't register any looping call, so it should be fine to leave it as is.
@babolivier babolivier force-pushed the babolivier/port_db_background_updates branch from 87d1de3 to 1eb0600 Compare October 7, 2019 17:03
@babolivier
Copy link
Contributor Author

babolivier commented Oct 7, 2019

As requested by @richvdh, the commits about factoring out the background updates from the database code have been moved to a dedicated PR: #6178 - this means that the reviewer can ignore any change to the synapse/storage directory when reviewing this PR.

The commits shown here are the ones pulled by merging that PR's branch into the current branch, since the current PR (#6102) now depends on it.

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 more suggestions for cleanups

scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
scripts/synapse_port_db Show resolved Hide resolved
@babolivier babolivier requested a review from a team October 22, 2019 17:13
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. feel free to fix up and merge.

scripts/synapse_port_db Show resolved Hide resolved
scripts/synapse_port_db Outdated Show resolved Hide resolved
@babolivier babolivier merged commit c97ed64 into develop Oct 23, 2019
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit 'c97ed64db':
  Make synapse_port_db correctly create indexes (#6102)
@babolivier babolivier deleted the babolivier/port_db_background_updates branch October 28, 2021 15:56
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.

3 participants