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

Schema cache reload with zero downtime #1559

Merged
merged 4 commits into from
Jul 16, 2020

Conversation

steve-chavez
Copy link
Member

The current SIGUSR1 schema cache reload causes downtime for some milliseconds(depending on schema size). To test this:

# Start
postgrest conf
# Reload
killall -SIGUSR1 postgrest & curl localhost:3000/projects
{"message":"Database connection lost, retrying the connection."}

This is because the current reload(implemented in #869) was meant for a single schema and the error message prevented getting a response from a stale schema cache.

(The error message is inaccurate when the db connection is actually up and a reload is done).

But now that we support multiple schemas in db-schema, is not appropriate to err a response on an up-to-date schema just because a newly added one can be stale.

So with this PR, responses can come from a stale cache. Embedding and rpc can be innacurate for a short time, but pgrst will be available on this lapse.

@steve-chavez
Copy link
Member Author

Should be ready!

I've improved the failed connection error messages. Before we would only get:

{"message":"Database connection lost, retrying the connection."}

But now we can get:

{"details":"no connection to the server\n","message":"Database client error. Retrying the connection."}

# or
{"details":"could not connect to server: Connection refused\n\tIs the server running on host \"localhost\" (::1) and accepting\n\tTCP/IP connections on port 5432?\ncould not connect to server: Connection refused\n\tIs the server running on host \"localhost\" (127.0.0.1) and accepting\n\tTCP/IP connections on port 5432?\n","code":"",
"message":"Database connection error. Retrying the connection."}

Some other error messages arise during recovery now. But they include the "Retrying the connection" message to be clear that pgrst is trying to recover.

@steve-chavez steve-chavez marked this pull request as ready for review July 16, 2020 00:20
# add v1 schema to db-schema
replaceConfigValue "db-schema" "test, v1" ./configs/sigusr2-settings.config
# reload
kill -s SIGUSR2 $pgrPID
kill -s SIGUSR1 $pgrPID
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change on the test is ensuring the SIGUSR1 doesn't cause downtime.

@steve-chavez steve-chavez merged commit 1898479 into PostgREST:master Jul 16, 2020
@wolfgangwalther
Copy link
Member

Looking at the distinction between "reloadable" and "non-reloadable" parts of the config file, I wonder whether it would be possible to use global postgres variables on a database level for the "realoadable" parts?

So something like this:

ALTER DATABASE xyz SET pgrst.db_schema = 'public';

Those could be read together with all the other info about the schema on schema reload.

The main benefit of this would be to have only one source of truth. Schema cache and some of the config options depend on each other - so it would make sense to store them all in code. Should be easier for users and maybe easier to handle in haskell code down the road as well?
Together with NOTIFY this could be even more "configure once, ...".

@steve-chavez
Copy link
Member Author

@wolfgangwalther Hmm.. how would the db-schema in the config file look like in that case?

One drawback of the ALTER DB statement is that it doesn't work unless you're the db owner or superuser - relevant on cloud managed services where you don't have those. Still, it could be worth having.

Together with NOTIFY this could be even more "configure once, ...".

I agree, all in the db is tempting. If we come up with a solution, I believe that would also solve #1429.

monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021
* Improve error messages and comments

* Reorder Main.hs functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants