Skip to content
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

Cache the lookup pepper in the HashingMetadataStore. #475

Merged
merged 3 commits into from
Dec 16, 2021

Conversation

reivilibre
Copy link
Contributor

The primary motivation for this is to make the casefolding migration script more performant
and to reduce the size of a strace log due to the repeated SQLite transactions involved with
fetching the lookup pepper for every single entry.

Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>
@reivilibre reivilibre requested a review from a team as a code owner December 14, 2021 18:00
@DMRobertson DMRobertson self-assigned this Dec 14, 2021
@squahtx
Copy link
Contributor

squahtx commented Dec 16, 2021

Presumably running two sydent instances off the same database is not supported?

@reivilibre
Copy link
Contributor Author

Presumably running two sydent instances off the same database is not supported?

As far as I know, no. It's worth noting that the lookup pepper is only generated/stored if it's absent on startup — concurrent Sydents using the same DB could already race and wind up rehashing over each other before, so I think it's fine to accept that we don't support that.

@DMRobertson
Copy link
Contributor

Thanks. I admit that it's probably over paranoid to update the cache if the pepper gets re-stored... but I'll sleep easier at night.

@squahtx
Copy link
Contributor

squahtx commented Dec 16, 2021

concurrent Sydents using the same DB could already race and wind up rehashing over each other before, so I think it's fine to accept that we don't support that.

True. The race is now slightly worse than before, since one of those instances will cache an outdated pepper and use that for new entries. Note that the rehash takes place in the same transaction as the pepper update, so that bit is "safe".

@reivilibre
Copy link
Contributor Author

True. The race is now slightly worse than before, since one of those instances will cache an outdated pepper and use that for new entries. Note that the rehash takes place in the same transaction as the pepper update, so that bit is "safe".

Oh that's true actually. 'Each function will queue some rehashing db transactions' is a bit misleading; those functions actually DO the transactions.

However I don't really see this being any worse than running two Synapses on the same database and expecting things to work just fine; you just wouldn't do it in the first place.

Do you think the PR's fine as is?

@squahtx
Copy link
Contributor

squahtx commented Dec 16, 2021

Do you think the PR's fine as is?

I'm not objecting to the PR! Just wanted to make sure we're aware of the limitations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants