feat: schema cache to prevent thundering herd#1258
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| let schema = shard.fetch_schema().await?; | ||
|
|
||
| *guard = schema.clone(); |
There was a problem hiding this comment.
in theory here could be a tricky data race with SchemaCache::global().clear();:
on replace_databases the following happens: clear -> new_databases.launch -> old_databases.shutdown
if for some reason we trigger replace_databases while schemas are still loading for old (like with retries due to errors) we may try to write into the cache that is basically associated with the new databases already.
The shutdown token would've be helpful, but this is called after the clear and launch, leaving this race window.
There is another race that is not important I think: lock -> fetch_schema -> reload_databases -> set to old guard. But it's not a big issue since the guard will not point to the cache map entry anymore and that'd be a separate mutex.
There was a problem hiding this comment.
I think I know what to do here. I'll add a global "config is reloading" signal that can cancel all of these long-running loops. I've been meaning to add something like this; making everything more deterministic.
There was a problem hiding this comment.
Might also have to be versioned too. Each config reload increments the config version so each Cluster knows which "epoch" it belongs to.
There was a problem hiding this comment.
*race condition, not a data race.
There was a problem hiding this comment.
Yeah, this one is kinda tough. We may need to refactor the config system from an ArcSwap to a watch, or something. to ensure this works across the whole app.
Right now, the race condition is real, but it may not have a real impact. i.e., if they race, they happen around the same time, so the schema is probably identical anyway. This was built specifically to busted when the user wants it to, e.g., they create a new table, then hit RELOAD in admin. Either way, the table already exists, and the schema loaded in the cache is fresh.
There was a problem hiding this comment.
I figured it out. We should attach all config-scoped "globals" to the Cluster. Duh. It's already versioned in a way that each config version gets its own Cluster, so as long as the cache is scoped to the lifetime of that Cluster, we don't have a race between config versions.
We should ban globals lol. Everything from now on should be Cluster-scoped.
| #[derive(Debug, Default)] | ||
| pub(crate) struct SchemaCache { | ||
| // Database => shard => Schema | ||
| cache: DashMap<String, DashMap<usize, Entry>>, |
There was a problem hiding this comment.
Let's newtype these so we don't need a comment to know what they are
There was a problem hiding this comment.
Either it will have automatic conversion from string, which will do nothing to prevent type issues, or it won't and we have to refactor String to Database in like 100 files. Pick your poison. Comment is fine for now.
There was a problem hiding this comment.
newtype is still useful even with auto conversion - it has clear meaning on type levels and allows for specific functionality. And the migration could be also done partially by converting to/from String on the boundaries.
not saying do it here, but we should prefer this for new code maybe
| // This is syncrhonized too, | ||
| // so only one shard/user can fetch the schema at a time. | ||
| let mut guard = entry.lock().await; | ||
|
|
||
| if guard.is_loaded() { | ||
| return Ok(guard.clone()); | ||
| } | ||
|
|
||
| let schema = shard.fetch_schema().await?; | ||
|
|
||
| *guard = schema.clone(); |
There was a problem hiding this comment.
If we use OnceCell for this
| // This is syncrhonized too, | |
| // so only one shard/user can fetch the schema at a time. | |
| let mut guard = entry.lock().await; | |
| if guard.is_loaded() { | |
| return Ok(guard.clone()); | |
| } | |
| let schema = shard.fetch_schema().await?; | |
| *guard = schema.clone(); | |
| entry.get_or_try_init(|| shard.fetch_schema()) |
There was a problem hiding this comment.
That feels obscure to me. What I have here is easier to read, at least for me and to understand its purpose.
There was a problem hiding this comment.
Using the purpose built thing is more obscure than implementing it yourself in a worse way? I do not understand why you're being so wildly dismissive here.
There was a problem hiding this comment.
hey, I had to agree with Sage - the OnceCell is just built for exactly this but does it with simpler code and uses just AtomicBool instead of mutex for synchronization.
doc just for reference - https://docs.rs/tokio/latest/tokio/sync/struct.OnceCell.html
There was a problem hiding this comment.
there is a subtle difference though - guard.is_loaded used to check the initialization instead of Option. I guess the empty schema will be refetched every time because of this 🤷🏻♂️
Co-authored-by: Sage Griffin <sage@sagetheprogrammer.com>
| pub regex_parser_limit: usize, | ||
| pub pub_sub_enabled: bool, | ||
| pub identity: &'a Option<String>, | ||
| name: &'a str, |
There was a problem hiding this comment.
@meskill Ignore this diff, just removed pub.
3 participants
Sharded deployments with dozens / hundreds of users will stampede the servers to load the schema. With large schemas, e.g., thousands of tables, and multiple replicas, this is pretty slow and will cause issues.
The cache is using the database name and shard number, which uniquely identifies a host in
pgdog.toml.