search

matrix-org / sydent

Sydent: Reference Matrix Identity Server
http://matrix.org
Apache License 2.0
303 stars 83 forks source link

Cache the lookup pepper in the `HashingMetadataStore`. #475

Closed reivilibre closed 2 years ago

reivilibre commented 2 years ago

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.

squahtx commented 2 years ago

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

reivilibre commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.