nixos/postgresql: add statements option to replace the ensureDatabases and ensureUsers options #84146
Conversation
|
👍 , also your idea about "reloading script" is implementation of last section of #65296 I also agree that without separate reloader script this |
|
Random thought: I don't speak SQL at all. The |
|
@symphorien I'd assume most nixos modules will still provide some sql statements out of the box, if your database is local. |
|
@flokli exactly. |
There was a problem hiding this comment.
Thanks a lot for your work!
I have to admit though that the more I think about it, the more I think that people should take care of their databases rather than modules, but this is IMHO the right place to discuss this issue 😅
add a statements (this is a horrible name, please someone come up with a better one)
probably idempotentStatements? (I know, I'm very bad at naming, but this would make it at least a bit clearer).
replace all usage of ensureDatabases and ensureUsers in NixOS with the new statements option
How about opening e.g. a discourse thread as soon as we've discussed the basics of this change? Would like to hear opinions of a few more maintainers.
which @Ma27 ended up being the victim of, see the recent matrix-synapse fiasco for details on that
For the record, we're talking about #80447.
It isn't reproducible and if we extend the option any further there is serious potential using that option could end up eating your data.
Correct me if I'm missing something, but this usually shouldn't happen with ensure*, right? The main issue is that it gives false promises: with those options you can declare that e.g. a user X should exist with a given configuration, but this is not ensured if X exists already. In that case, the DB user X is not touched.
| </itemizedlist> | ||
| <para> | ||
| See https://stackoverflow.com/a/18389184 for suggestions on writing idempotent sql | ||
| statements for PostgreSQL. |
There was a problem hiding this comment.
Please correct me if I'm wrong, but this wouldn't fix issues like changed collation requirements for databases (as it happened with matrix-synapse), right?
Or in other words, what would happen, if
- the collation of a database has to be changed
- the module-author of an arbitrary service modifies the sql statement in
services.postgresql.statements - a user deploys this configuration to a server with an existing database
?
There was a problem hiding this comment.
The intention is not to fix the situation you described, but instead push volatile db work off of nixos and onto the sysadmin.
To answer your question: release notes explaining to users what they need to do should be issued. A special mention of backups should also be mentioned.
|
@Ma27 thanks for your comments. The "eating your data" comment is something that would be more of a concern if we extend the current ensure* options to include things like locale. Upstream documentation states databases should always be backed up immediately before operations like changing locale. I agree we need more people in this conversation. Who should we ping? |
|
I avoid using these options because of their lack of flexibility. Instead I uses a Sqitch project that contains all the things I need to ensure. I have created a { config, lib, pkgs, ... }:
with lib;
let
cfg = config.services.sqitch;
deploymentOptions = {
options = {
bundle = mkOption {
type = types.package;
example = "pkgs.mybundle";
description = ''
PostgreSQL Sqitch bundle to deploy.
'';
};
targetURI = mkOption {
type = types.str;
example = "db:pg://postgres@localhost:5432/postgres";
description = ''
Database database connection URI of the target. Only PostgreSQL targets are supported.
'';
};
user = mkOption {
type = types.str;
default = "postgres";
description = ''
User running the Sqitch deployment.
'';
};
group = mkOption {
type = types.str;
default = "postgres";
description = ''
Group running the Sqitch deployment.
'';
};
requires = mkOption {
default = [];
type = types.listOf types.str;
description = ''
systemd `Requires=` dependencies to add to the Sqitch deployment service.
'';
};
after = mkOption {
default = [];
type = types.listOf types.str;
description = ''
systemd `After=` dependencies to add to the Sqitch deployment service.
'';
};
};
};
in
{
options = {
services.sqitch = {
enable = mkOption {
type = types.bool;
default = true;
description = ''
Whether to run Sqitch deployments.
'';
};
deployments = mkOption {
default = { };
type = with types; attrsOf (submodule deploymentOptions);
description = ''
Attribute set of deployments to run. Creates
<literal>sqitch-''${deployment}.service</literal> systemd units for
each deployments defined here. Other services can add dependencies
to those services if they rely on the deployments.
'';
};
};
};
config = mkIf (cfg.enable && cfg.deployments != { }) {
assertions = let
deployments = (mapAttrsToList (deployment: options: options) cfg.deployments);
in [
{
assertion = all (deployment: hasPrefix "db:pg://" deployment.targetURI) deployments;
message = ''
Options `service.sqitch.deployments.<name>.targetURI` must start with "db:pg://" because only PostgreSQL is supported.
'';
}
];
environment.systemPackages = [
pkgs.sqitchPg
];
systemd.services =
let
toServicePair = deployment: options:
let
deploymentService = {
description = "Sqitch deployment for ${deployment}";
requires = options.requires;
after = options.after;
path = [
config.services.postgresql.package
pkgs.sqitchPg
];
script = ''
sqitch --cd ${options.bundle} deploy --target ${options.targetURI}
'';
serviceConfig = {
Type = "oneshot";
RemainAfterExit = true;
User = options.user;
Group = options.group;
PrivateTmp = true;
ProtectSystem = "strict";
ProtectHome = true;
ProtectKernelTunables = true;
ProtectKernelModules = true;
ProtectControlGroups = true;
SyslogIdentifier = "sqitch-${deployment}";
};
};
in {
name = "sqitch-${deployment}";
value = deploymentService;
};
toServices = deployments: listToAttrs (mapAttrsToList toServicePair deployments);
in
toServices cfg.deployments;
};
} |
|
@datafoo thanks for sharing that. I'm sure some people would find that very useful. The Anyone? |
|
I agree that the options are currently problematic and I ended up creating a custom I have minor preference for By the way, there was a PostgreSQL patch trying to add |
|
@jtojnar thanks for your comments. My thought would be to eliminate and replace the |
|
👍 for getting rid of |
|
Thanks @peterhoeg! Would you see any value in a |
|
As I see it, there are 2 problems with the current situation:
1. Many times more flexibility is needed than what ensureXX supports (ie, creating a role with a given password for pg)
2. initialScript isn't expected to be idempotent and thus changing it after it has run once, has no effect.
Writing idempotent sql is hard and thus people will inevitably end up writing custom sql for their special cases anyway.
|
|
@peterhoeg if we're looking to cover simple cases and give a reasonable amount of flexibility to users, with warning around breaking reproducibility, I think the So is the |
|
ping @peterhoeg
|
…s and ensureUsers options
…tions with new statements option
Motivation for this change
I have been
brainwashedconvinced in a reasonably fashion by @grahamc that theensure*options aren't a good fit for NixOS. I'm seeking to remove these options from both themysqlandpostgresqlmodules, but still leave other module authors who wish to provision databases with a reasonable way to do so. My plan forpostgresqlandmysqlwas as follows:ensureDatabasesandensureUsersoptionsstatements(this is a horrible name, please someone come up with a better one) option which module authors (and NixOS users) can use to write idempotent sql statements which will be executed as themysql/postgresqlsuper admin user to do things like provision local accounts, databases, etc... (ie. replace theensureDatabasesandensureUsersoptions with a new option that provides the same functionality, but with even more flexibility)ensureDatabasesandensureUsersin NixOS with the newstatementsoptionstatementsoption, very explicitly explaining how it leads to systems which aren't reproducible and why that is badstatementsoption from thepostgresql/mysqlsystemdservice so a full restart ofpostgresql/mysqlis not required every time someone altersstatements(this step would be done in a separate PR immediately following the acceptance and merging of this PR)I wrote some code for the
mysqlmodule to prove this idea works, and I was relatively happy with the results. Not being apostgresqlguy myself, I decided I needed to prove this idea translated adequately intopostgresql. I was surprised to learnpostgresqldoesn't provide a very convenient way to write idempotent statements for database and user creation likemysqldoes 😱 You can't writecreate database if not exists! I searched around and found what appears to be the "best" solution and adopted that method for this PR. For example, creating a database:select 'create database nextcloud owner nextcloud' where not exists (select from pg_database where datname = 'nextcloud')\gexecYou might ask why should we get rid of the
ensure*options?Aside from promoting systems which aren't reproducible, the
ensure*options have a few other major problems withpostgresql, mainly dealing with encoding and extensions (which @Ma27 ended up being the victim of, see the recentmatrix-synapsefiasco for details on that 😞). Currently all attempts to extend theensureDatabaseoptions have led to scenarios where a usersconfiguration.nixcould very easily not match the reality of what the database actually looks like. After investigating what it would take to make theensure*options truly reproducible and actually enforce what they describe I came to the conclusion this is simply a bad idea because any database changes we might make can lead to data corruption and require a backup before making them - something we don't want to do every time the user does a rebuild.I am very interested in feedback from
postgresqlusers to see how they feel about this PR. I'm guessing that some of you will argue that users can simply add sql statements to the.postStartof thepostgresqlsystmedunit - while I'm not completely opposed to that idea, I do see it as somewhat hacky, provides no documentation, and causespostgresqlrestarts with no easy and user-facing-api friendly way correct that.If you have the capacity to please take a few moments to consider this proposal and comment with your thoughts. I'm very interested in improving the database ecosystem we have here, but will need to rely on the
postgresqlexperts because I live in amysql(mariadb) world.Also note that this is a draft PR and the code is very sloppy proof of concept. I'll get around to cleaning it up after I get some feedback to see if I'm heading in the right direction here.
Thanks 🎉
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)cc @flokli