-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix PGRST002 errors not logging the JSON messages and reloading the schema unnecessarily on startup #4367
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
Fix PGRST002 errors not logging the JSON messages and reloading the schema unnecessarily on startup #4367
Conversation
319f1a5 to
794f22c
Compare
794f22c to
b1a49bd
Compare
b1a49bd to
c30a9d0
Compare
src/PostgREST/Error.hs
Outdated
| hint (SchemaCacheErr err) = hint err | ||
| hint (JwtErr err) = hint err | ||
| hint NoSchemaCacheError = Nothing | ||
| hint NoSchemaCacheError = Just "This usually happens when PostgREST is starting up. Try again later." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the requests to wait like what happens after the schema cache is loaded? This would be the ideal behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laurenceisla Maybe if we drop the maybe from:
postgrest/src/PostgREST/AppState.hs
Line 85 in 706a489
| , stateSchemaCache :: IORef (Maybe SchemaCache) |
The waiting will automatically happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if we drop the maybe
Tried it out and did not work. There is no Nothing schema cache if this is the case, we have to use one with empty values (e.g. empty tables, empty functions, etc.). It doesn't wait automatically, it returns an immediate 404 response with this initial schema instead of a 503 with the PGRST002 error.
Can we get the requests to wait like what happens after the schema cache is loaded? This would be the ideal behavior.
I also think this would be ideal, it would also stop flooding the logs with PGRST002 errors for apps with a lot of requests. However this would be a different behavior from what we currently do when we restart the schema and respond before the schema is loaded. For example:
-- On startup:
Config reload
-- Requests are immediately rejected here with `PGRST002` errors
Schema queried
-- Requests wait until schema is loaded
Schema loaded
-- On a Schema Cache Reload:
Config reload
-- Requests are immediately handled using the stale schema
Schema queried
-- Requests wait until schema is loaded
Schema Loaded
So instead of immediately reject the request with PGRST002 we would wait instead. I'll check further on how to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Maybe this is too complicated right now. As it is, the PR is an improvement though as it surfaces an error.
The only problem is that the message looks a bit sloppy 🤔. I can't think of a better choice, maybe leave it as before or add a TODO comment.
| hint NoSchemaCacheError = Just "This usually happens when PostgREST is starting up. Try again later." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Maybe this is too complicated right now.
Yeah, can confirm that it's not so trivial.
The only problem is that the message looks a bit sloppy 🤔. I can't think of a better choice, maybe leave it as before or add a TODO comment.
Yes, wasn't too convinced either. I'll remove it.
39886b2 to
a2bb823
Compare
a2bb823 to
6f2993a
Compare
6f2993a to
4ee98d7
Compare
It happens right after the configuration is loaded and before the schema cache is queried.
When 503 errors happen if the Schema Cache is empty, it should not retrigger the connection worker since there's no Schema Cache loaded yet.
4ee98d7 to
95720c5
Compare
Closes #4129 as mentioned in the revision #4129 (comment).