-
Notifications
You must be signed in to change notification settings - Fork 331
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
[FEAT] Do not use single-user mode when initializing postgres #746
Conversation
There will be some minor error output for existing repositories. If the initial script performs a destructive change, though, it can/will cause downstream failures. If you have any suggestions on a reliable method of checking if postgres has had the initialization scripts run against it, I would be happy to help implement it. |
Thanks! I'll try this as soon as I can ❤️ |
Testing this is kind of hard; I had to make a git repo and manually point |
Thanks for taking on this task! Here is how you can turn any example into a local test. One thing to keep in mind is for more sophisticated task runners (like process-compose) that readiness probes might evaluate to true while the temporary DB is started up. This should be avoided. Please use the |
src/modules/services/postgres.nix
Outdated
echo "PostgreSQL appears unconfigured; going to run initialization scripts." | ||
echo | ||
|
||
pg_ctl -D "$PGDATA" -w start |
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.
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.
This, plus changing the default listen address to 127.0.0.1
(which should be safe), alongside changing the readiness probe to use TCP/IP instead of unix socket should fix the readiness probe problem.
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.
Alternatively the unix socket directory could be set to a temporary directory with mktemp
, if we don't want to break backward compatibility. In my experience most coworkers expect an IP to be bound, given most of the people migrate from docker-compose postgres to devenv.
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.
I don't really want to break backwards compatibility. I made the change to listen on "no addresses", I didn't have to modify the default listener (although I generally agree with the expectation of listening on TCP) to make sure that nothing was broken.
I now create a temporary directory using mktemp
to hold the temporary initialization server socket. It removes the directory when finished and a trap
removes it when the script EXIT
s.
I also removed the need for the "trailer file" to tell if the database has been configured. Instead, I use a variable and set it when you actually run initdb
. I like this better because it would have tried to run initialization scripts on all existing databases. This could be good or bad, depending on the scripts.
Side bar: if you have several minutes worth of initialization scripts, process-compose
will forcibly kill the postgres
process and you'll be stuck with an incomplete database. It's not something I am going to tackle right now, but be aware that those conditions exist.
This change will "hide" the temporary server from the readiness checks in addition to not listening on a TCP port. While initializing, the readiness should be "not ready". It creates a temporary directory inside of `$DEVENV_STATE` to hold the temporary UNIX socket. It also overwrites `$PGHOST` to set the default for `psql` and restores the old value when finished. It removes the temporary socket directory explicitly and sets a `trap` to remove if there is an error.
I updated the test repo to use |
The comment about local testing was 💯 spot on. Thanks, it made testing quite a bit easier. |
This makes the initialization process for
postgres
use the "multi-user" mode instead of "single-user" mode. This change allows for creating extensions automatically on first boot (liketimescaledb
).Initial log output
Minimal reproduction example
You should be able to use the example and then do a
devenv shell
thendevenv up
and it should have a bunch of TimescaleDB in the output. I haven't tested this much more extensively than that, but it does appear to be reliable.The process more or less mimics the pattern from the Docker library, but it's not much more than using
pg_ctl
to start the server at the beginning of the process (just afterinitdb
has been called and thepostgresql.conf
has been written in place) and then shutting the temporary server down usingpg_ctl
again.This should resolve #696