add: load schema cache dump from URI at startup - #5031
Conversation
ecd794f to
d6968c9
Compare
d6968c9 to
3541b96
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Helping to clarify, the motivation is to solve the same problems on #4999. This by loading the schema cache from a file. |
|
Q: If the schema cache file is stale, we still pick it up and serve requests as "best effort"? |
@steve-chavez I am still working on the right design and will provide more details soon. I believe now that we can solve in this PR all pain points we want to solve with materialized views. |
Yes the idea is to treat schema cache preloaded from the dump as stale and trigger asynchronous load from the db. |
3541b96 to
6417d00
Compare
6417d00 to
25e4372
Compare
1d96a81 to
a51ed0b
Compare
| def test_slow_schema_cache_dump_uri_does_not_delay_startup(defaultenv): | ||
| "A slow schema cache dump URI should not delay database-backed startup." |
There was a problem hiding this comment.
So if the URI is slow then we will prefer querying the schema cache.
Won't this will always be the case when doing HTTP requests? It seems like this feature will only make sense when it's obtained from a file since that'd be faster.
There was a problem hiding this comment.
So if the URI is slow then we will prefer querying the schema cache.
Won't this will always be the case when doing HTTP requests? It seems like this feature will only make sense when it's obtained from a file since that'd be faster.
Well... given performance issues with schema cache queries we've seen reported, I wouldn't be so sure.
HTTP is needed because:
- In
k8senvironment it is rare to have a shared filesystem to read the file from. - Since we provide schema cache dump from admin server it is very convenient for blue/green deployments - just configure each to point to the other one's admin endpoint and you have schema cache pre-loading for free.
Besides: why would HTTP be slow? The most important thing here is not the access protocol but the fact that we serve the dump from memory. I am sure establishing database session and then querying the catalog is going to be much slower than HTTP GET from localhost.
I am pretty sure HTTP is going to be faster than reading from a materialized view as well.
But really we don't want to decide up front which method is preferred. We need to query the database anyway because pre-loaded dump might be stale. So why wait if we are going to do it anyway? It is better to race - that way we are going to have startup as fast as possible - we really don't care which method wins. We only need to make sure the database result is not overwritten.
There is also another angle: we want to deal with thundering herd issue when querying the db. In clustered environments HTTP gives us a natural way to load balance initial schema cache load among existing PostgREST instances. Once we have #4643 or similar it will become even more important as it might affect startup time.
There was a problem hiding this comment.
Good points. So we're always going to race here to get the fastest possible startup, got it. We should make this clear on the docs.
There was a problem hiding this comment.
There is also another angle: we want to deal with thundering herd issue when querying the db. In clustered environments HTTP gives us a natural way to load balance initial schema cache load among existing PostgREST instances.
@mkleczek I'm still not seeing why is racing preferable to opting for only a schema-cache-uri load.
The Listener should still be there to tell the instance when the cache is stale, so it requests it again from schema-cache-uri.
If we have a mode for only requiring schema-cache-uri to start, then I believe that should solve #4643 too. The replica instances would make a request to a master one.
Considering #5031 (comment), the schema cache is not all that we need for a postgREST to start right now; but maybe we can make it that way.
There was a problem hiding this comment.
There is also another angle: we want to deal with thundering herd issue when querying the db. In clustered environments HTTP gives us a natural way to load balance initial schema cache load among existing PostgREST instances.
@mkleczek I'm still not seeing why is racing preferable to opting for only a
schema-cache-uriload.The
Listenershould still be there to tell the instance when the cache is stale, so it requests it again fromschema-cache-uri.If we have a mode for only requiring
schema-cache-urito start, then I believe that should solve #4643 too. The replica instances would make a request to a master one.
That is, I'm afraid, racy:
- Replicas can issue
HTTP GETto master PostgREST instance before it actually re-loaded the schema cache. - There might not be a "master" PostgREST instance - it might be dynamic and routing might be based on
HTTPmethod.
Considering #5031 (comment), the schema cache is not all that we need for a postgREST to start right now; but maybe we can make it that way.
I think we should think about making it more organized into "database dependent" and "database independent" configuration - right now some aspects are mixed (ie. config/role settings/schema cache).
There was a problem hiding this comment.
Replicas can issue HTTP GET to master PostgREST instance before it actually re-loaded the schema cache.
@mkleczek But we could put a "version" on the schema cache so if the replica requests a new cache and it's the same version then it would retry. (we discussed a "version" previously in #4270 (comment)).
There was a problem hiding this comment.
Replicas can issue HTTP GET to master PostgREST instance before it actually re-loaded the schema cache.
@mkleczek But we could put a "version" on the schema cache so if the replica requests a new cache and it's the same version then it would retry. (we discussed a "version" previously in #4270 (comment)).
Adding schema versioning that requires parsing a function body and then CREATE OR REPLACE FUNCTION with a new function body in an event trigger is waaaay to brittle and complex for my taste, to be honest.
And the gain is not substantial while has quite a big downside: it slows down start-up in case of master instance being unresponsive (eg. due to temporary network conditions).
Besides - there is a chicken and egg problem here: you want to load schema cache without accessing the database. How would you know the schema version number in this case?
a51ed0b to
6fd483f
Compare
|
Since this is about fast startups, I was wondering if you have considered the in-db config + role settings queries, which if used are also required for startup (done prior to the schema cache): postgrest/src/PostgREST/Config/Database.hs Lines 96 to 134 in 6191243 postgrest/src/PostgREST/Config/Database.hs Lines 136 to 185 in 6191243 The in-db configs are not cached but the role settings are cached here: postgrest/src/PostgREST/Config.hs Lines 128 to 129 in 6191243 When adding those I wondered if they should go into schema cache or not, I decided to make them separate to save some work on filtering them for the openAPI output. But with this feature, it looks we should make them part of the schema cache to really get the faster startup? |
Actually I don't think the "in-db config" can be put inside the schema cache because it depends on it ( |
249ef2e to
bfbc506
Compare
f019f80 to
5c4f8c3
Compare
wolfgangwalther
left a comment
There was a problem hiding this comment.
Gave this a high-level readthrough. I wonder what happens when you try to load a schema cache dump from a different PostgREST version, which has made changes to the schema cache structure?
Some of these might just fail the JSON parsing. But others might be more subtle and not cause that failure.
I think we should put the current PostgREST version into the schema cache dump / response by the admin server and check it when loading the dump. If it doesn't match, reject to load the dump.
5c4f8c3 to
b95499f
Compare
Note that cross-version schema cache dump read is one of the main use cases: zero-downtime upgrades. So while understanding the concern, I think we should leave it as is: the risk is minuscule IMO:
I guess what we can do is making sure parsing of schema cache dump is as strict as possible to minimize the problem you describe. |
I don't understand how that relates. For zero-downtime upgrades, aka #5112, I can just read the schema cache from the database, I don't need to read a dump, right? |
I think we should:
It only makes sense to load the schema cache dump when it was created with exactly the same configuration. If not, we need a new schema cache. |
Ideally, you run PostgREST like I'd even consider having a switch that disables standard schema cache load on startup when |
This won't work because of in-db configuration. The whole point of loading from URI is to be able to start serving requests without connecting to the database to read any metadata. With in-db configuration you are always going to have different configuration before reading it from the database. |
But by using a schema cache from a different version, we are going to be more prone to errors. I think it's fine to either provide fast startup or zero-downtime - but we don't need to provide incredibly fast zero-downtime handovers. Especially in the zero-downtime scenario, I'd value a higher chance of success much more than being a few seconds faster. |
I am not convinced: when was the last time you had a change in schema cache dump that would cause the errors you described (ie. parsing fine, results wrong)? |
I think this should support both If you want to serve requests without hitting the database before the actual request, you will just need to set |
It doesn't matter when the last time was that this happend - I have no idea, I didn't have to pay attention to that. What matters is, that I don't want to pay attention to this in the future, when making changes to PostgREST. If we have to keep schema cache compatibility in mind, that's going to be really annoying for development. |
Only in theory, but not in practice. Schema cache changes are so rare that it is practically non issue IMO. Schema cache changes not breaking strict parsing (eg. missing fields cause errors) but only semantics - not a chance :) |
Or load both schema cache and config from URI. |
I think what you could do would be:
This would combine both the verification phase and the ability to get a consistent config/schema snapshot from the database. |
If you have consistent config and schema cache snapshot, why would you need to verify anything? |
Because changes to the not-in-db-config should be reflected. For example when I manage exposed schemas via regular configuration and I start a new instance, explicitly removing one of the schemas - I sure don't want this instance to respond to requests for that old schema. We're only optimizing the cache here, but we should make sure to respect explicit configuration changes the user makes. |
One example would be the schema cache just having the same format, but including/excluding more list items somewhere. For example the schema cache queries were wrong and returned too many rows for, let's say, overloaded RPCs. The new version fixes this. We release a new version, claiming a fix. But PostgREST loads the old schema cache and will serve wrong responses from that. I'd rather have the new version start up properly and serve the correct responses from the start. Edit: This is even more important when you disable regular schema cache load if an URI is provided. |
Hmm... if you want to change static configuration and restart - why add
|
I'm thinking about that AWS lambda example. I assume you'd just want to always start with There is no decision-making involved in how to actually start and/or reload the process. This is hardcoded. What am I missing? |
In this case it makes sense to dump both config and schema cache, load both of them from |
See #5031 (comment) for why I think this is wrong. Also, there is another problem with both config validation and dumping/loading, that we didn't consider: LIBPQ environment variables are effectively a part of configuration, but we don't store their values anywhere. So changing any of these (for example by connecting to a different pg server!) would not fail verification and would also create a problem when loading old configuration from file and combining it with new LIBPQ environment variables... |
I don’t see how - the new config will be used, the exact timing is not that important because either way there always going to be a time window between you changing configuration and PostgREST reflecting it in its responses.
And that’s even more the reason why we should not do any validation and treat this feature as an advanced one that requires the user to understand the risks. |
I thought about it a little more and I think this is a malformed scenario: You have to think about lambda function shutdown/startup as "hibernate/wakeup". This PR idea is the same: schema cache dump is a "frozen state" that is used for wake-up - after restart the program is conceptually in the same state as before. Additionally it hooks into after-wakeup to refresh its state with any changes that might have happened in the environment while it was asleep - but that is conceptually "a bonus" that makes user's life easier (ie. no need for You cannot rely on lambda shutdown/startup to reload configuration anyway - because shutdown might never happen. So you must take explicit action to reload configuration (and it is a good question what would be the right trigger to reload config in lambda environment as neither Ideally the "hibernate/wakeup" idea should cover both config and schema cache. Unfortunately, to make that happen, PostgREST changes would be much more substantial: configuration right now is read very early in the startup sequence - even before deciding if it is a I would leave this PR as is and not go into the consistency check direction at the moment:
I've created #5129 to discuss the direction. |
Resolves #4999
by making it unnecessary.