PostgreSQL cannot be reloaded in place for routine configuration changes

[ocharles]

The way we have services.postgresql set up currently prohibits in-place reloading (kill HUP) of postgres, which is doable for a lot of routine configuration changes (e.g., new pg_hba rules). We've put ourself into a hole by symlinking postgresql.conf at service startup, so just doing nixos-rebuild with reloadIfChanged = true; restartIfChanged = false; won't work, because it just reloads the current configuration rather than the new one.

One idea would be to re-symlink on reload. Another might be to copy the configuration files around at system activation time.

Thoughts?

[edolstra]

We can't really use reloadIfChanged because we currently don't have a way to distinguish between configuration changes that require a restart versus those for which a reload is sufficient. If we did, resymlinking or copying would be fine.

[ocharles]

Sure, but I'm wondering if we should provide a reload command to do that anyway. Maybe the official module will still restart by default, but at least users have the option of mkForceing restartIfChanged to false and using the reload behaviour.

[danbst]

Just to note, what is the current way to overcome this issue.

1. We include or include_dir additional file/files to config outside from /nix/store, where we can specify dynamic options (didn't test yet if we can specify hba_file option twice. EDIT: tested, it works but generates a warning in logs)
2. Send kill HUP (but why not systemctl reload postgresql? EDIT: systemctl kill -s HUP postgresql)
3. Select reloadable options by parsing default postgresql.conf file (non-reloadable options have "changes require restart" in comment)

[jagajaga]

PIng all

[ocharles]

Pinging about what?

[jagajaga]

About any new information and status :)

[spinus]

@danbst
3. detecting this stuff could be tricky, I think just a simple flag is enough to say that.
Let's say we impement set of flags

service.postgresql.postgresql-conf-path =
service.postgresql.pg-hba-path = 

They could be used in two ways

service.postgresql.postgresql-conf-path = "/var/lib/postgresql/my-dynamic.config";
# In this case user is responsible to delivery configuration, for example using another systemd service as dependency.

or

service.postgresql.postgresql-conf-path = pkgs.writeText "my-static-config" "....."

In that case user can chose what he wants. We could detect if the file is in nix store or not and based on that we could do "reload".

What do you think?

Besides that, @ocharles, I think quite a lot of configuration in postgres can be changed inside the database. You can set most of the configuration by running sql commands. Those options which can't be changed there probably require restart anyway (I'm only guessing here), I didn't cover you pg-hba case which would still be valid as external file which chanced.

[ocharles]

I wonder if we can use the include functionality to have files that only contain configuration options that require a restart, and configuration files that require a reload. It might then be possible to orchestrate restarting with some systemd units - for example one that on launch restarts PostgreSQL and one that reloads PostgreSQL.

[danbst]

@spinus @ocharles I've tested my module (heh) and fixed remaining issues. I'll post it here before doing PR, if somebody can test too - I'd be welcome to hear feedback. It doesn't require patching Nixpkgs and is used as a typical module. It requires one PG restart on first switch. Tested on 16.09

Pros: service interface is not changed, your postgresql configuration and authentication changes no longer cause PG restart

https://gist.github.com/danbst/382852cae4d93664634e830fa1df4736

[ocharles]

This doesn't look bad. It might be cleaner to use PropagatesReloadTo on the config service, and set reloadIfChanged on the config service. That should save an unnecessary reload on initial startup, and avoid querying systemd from within that unit, which feels like an antipattern.

[spinus]

@danbst I would propose to write a test for it :-)

[danbst]

writing the proper test is blocked by #20886, IMO. I'm very slowly working on this front.

Though, I can reuse your PG test, the only change would be add imports = [ ./postgresql-reloadable.nix ];

[danbst]

closing this in favor for #65296