This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2608

• Aren't users and roles the same thing in postgres?
• Is this a declarative relation, or an imperative operation that copies permissions? In the latter case, we can't make a declarative option for it.
• It should remove declaratively specified roles when they are removed from the configuration. This implies extra state, that would best be tracked on the NixOS host or in the database. See also the discussion at...
• ... possible duplicate: Enrich PostgreSQL ensure users and databases support #203474

• Aren't users and roles the same thing in postgres?

Yes, I believe the USER syntax is only provided for compatibility. I didn't want to change the existing module too heavily.

• Is this a declarative relation, or an imperative operation that copies permissions? In the latter case, we can't make a declarative option for it.

Kind of? No permissions are copied per se, it's just effectively saying "this user/role inherits everything of this other role".

• It should remove declaratively specified roles when they are removed from the configuration. This implies extra state, that would best be tracked on the NixOS host or in the database. See also the discussion at...

It's the same deal for ensureUsers - users aren't deleted from the DB when they're removed from the NixOS config.
I'd personally be a bit apprehensive about potentially deleting database state without user interaction.

• ... possible duplicate:

I'd personally be a bit apprehensive about potentially deleting database state without user interaction.

For an ensure option, I wholeheartedly agree, but the issue has been raised that ensure-style options cause stale state, so we may do well to avoid ensure-style options in the first place, and go for a solution where the declared state is managed entirely, consistently by NixOS.

Whether ensure is good enough may be subjective, so I don't think it's a deal breaker.

Could you add this to the test suite?
I think the docs can also be improved. The role/user non-distinction should be addressed, and I think something went wrong with the explanation. Also it'd be nice to explain the effect in addition to the syntax produced.

I'd personally be a bit apprehensive about potentially deleting database state without user interaction.

For an ensure option, I wholeheartedly agree, but the issue has been raised that ensure-style options cause stale state, so we may do well to avoid ensure-style options in the first place, and go for a solution where the declared state is managed entirely, consistently by NixOS.

Whether ensure is good enough may be subjective, so I don't think it's a deal breaker.

I agree it's a bit iffy and does cause stale state under some circumstances - but what's the alternative? Short of doing some hackage such as with dumping all the existing roles/users at re-build time and comparing them to what's there (which, in turn has its own state issues), I don't really see a nicer way.

Could you add this to the test suite? I think the docs can also be improved.

Can do.

The role/user non-distinction should be addressed, and I think something went wrong with the explanation. Also it'd be nice to explain the effect in addition to the syntax produced.

What if I rename ensureUsers to ensureRoles, and change the new ensureRoles to memberOf or something similar?

For an ensure option, I wholeheartedly agree, but the issue has been raised that ensure-style options cause stale state, so we may do well to avoid ensure-style options in the first place, and go for a solution where the declared state is managed entirely, consistently by NixOS.
Whether ensure is good enough may be subjective, so I don't think it's a deal breaker.

I agree it's a bit iffy and does cause stale state under some circumstances - but what's the alternative? Short of doing some hackage such as with dumping all the existing roles/users at re-build time and comparing them to what's there (which, in turn has its own state issues), I don't really see a nicer way.

I just had a thought on this - we (NixOS) explicitly create a database (e.g. nixos) and store current state there. Then at activation-time, we simply compare against that. That's probably outside the scope of this PR though.

What if I rename ensureUsers to ensureRoles

We should stick to upstream terminology, so I'd be in favor of such a change.

and change the new ensureRoles to memberOf or something similar?

When I scanned the docs for this feature, there wasn't a term that jumped out, to name this relation, but I'm sure they've named it somewhere. Ideally we use a name they already use, even if the name isn't documented (and not terrible, because then they might change it or document it differently later). Maybe it shows in postgres' internal schema; something like pg_roles or whatever the table is named.

we (NixOS) explicitly create a database (e.g. nixos) and store current state there. Then at activation-time, we simply compare against that. That's probably outside the scope of this PR though.

Yeah, I think it should look like that, although we could possibly improve it by taking it in either of two directions:

• Use a file so that we can work towards a reusable framework for managing the state that is needed for declarative removals - reusable outside the context of postgres. We'd have infrastructure for doing similar things with any other kind of service, including things like NixOS mutable users (already implemented correctly - probably not worth refactoring, but I'm sure we can still apply the framework to other areas that aren't database servers).
• Turn this into a postgres admin tool that is reusable outside the context of NixOS, and can be applied to any database; not just a local one. This also helps NixOS, for example when users need to scale their service and move the database out of the app server into a different server or managed db. Ideally this tool already exists and we just translate/automate it in NixOS.

What if I rename ensureUsers to ensureRoles

We should stick to upstream terminology, so I'd be in favor of such a change.

👍

and change the new ensureRoles to memberOf or something similar?

When I scanned the docs for this feature, there wasn't a term that jumped out, to name this relation, but I'm sure they've named it somewhere. Ideally we use a name they already use, even if the name isn't documented (and not terrible, because then they might change it or document it differently later). Maybe it shows in postgres' internal schema; something like pg_roles or whatever the table is named.

Postgres uses pg_group internally (irrelevant rows removed):

postgres=> SELECT * FROM pg_catalog.pg_roles;
          rolname          | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolconnlimit | rolpassword | rolvaliduntil | rolbypassrls | rolconfig |  oid
---------------------------+----------+------------+---------------+-------------+-------------+----------------+--------------+-------------+---------------+--------------+-----------+--------
 pg_read_all_data          | f        | t          | f             | f           | f           | f              |           -1 | ********    |               | f            |           |   6181
 backup                    | f        | t          | f             | f           | t           | f              |           -1 | ********    |               | f            |           | 162320

postgres=> SELECT * FROM pg_catalog.pg_group;
          groname          | grosysid | grolist
---------------------------+----------+----------
 pg_read_all_data          |     6181 | {162320}

postgres=> SELECT * FROM pg_catalog.pg_roles WHERE oid=162320;
 rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolconnlimit | rolpassword | rolvaliduntil | rolbypassrls | rolconfig |  oid
---------+----------+------------+---------------+-------------+-------------+----------------+--------------+-------------+---------------+--------------+-----------+--------
 backup  | f        | t          | f             | f           | t           | f              |           -1 | ********    |               | f            |           | 162320

Looking at https://www.postgresql.org/docs/current/role-membership.html, there's "role membership", "group role", and "member roles". Some options:

    services.postgresql.ensureRoles = [ # Changed from ensureUsers
      { name = "backup"; ensureRoles = [ "pg_read_all_data" ]; }
      { name = "backup"; memberOf = [ "pg_read_all_data" ]; }
      { name = "backup"; memberships = [ "pg_read_all_data" ]; }
      { name = "backup"; groups = [ "pg_read_all_data" ]; }
      { name = "backup"; ensureGroups = [ "pg_read_all_data" ]; }
      { name = "backup"; additionalRoles = [ "pg_read_all_data" ]; }
      { name = "backup"; ensureAdditionalRoles = [ "pg_read_all_data" ]; }
    ];

we (NixOS) explicitly create a database (e.g. nixos) and store current state there. Then at activation-time, we simply compare against that. That's probably outside the scope of this PR though.

Yeah, I think it should look like that, although we could possibly improve it by taking it in either of two directions:

* Use a file so that we can work towards a reusable framework for managing the state that is needed for declarative removals - reusable outside the context of postgres. We'd have infrastructure for doing similar things with any other kind of service, including things like NixOS mutable users (already implemented correctly - probably not worth refactoring, but I'm sure we can still apply the framework to other areas that aren't database servers).

* Turn this into a postgres admin tool that is reusable outside the context of NixOS, and can be applied to any database; not just a local one. This also helps NixOS, for example when users need to scale their service and move the database out of the app server into a different server or managed db. Ideally this tool already exists and we just translate/automate it in NixOS.

All definitely outside the scope of this PR, but both options are worth looking into.

Some options:

Broadly the choice seems to be between just "role", "group" and "member".

• role is not very descriptive
• group seems to suggest an entity more than a relation between entities. It could be defined as a set of memberships, but it represents the inverse relation. I think we want to declare at the member which groups it is part of rather than declare at the group which members it has.
• memberOf or membership don't suffer from this, so I think they're most appropriate. Maybe of the two, memberships is preferable because it's plural and the value is a list.

I think it'd be good to highlight that we're still only ever adding to the memberships, by repeating ensure. How about ensureMemberships?
Just memberships may suggest that we are ensuring that a role exists, but with an exact set of memberships.
I think most ensureUsers.<name>.<attr> attrs intentionally have an ensure prefix for this reason, although I doubt that ensureClauses needs it.

I just started attacking this - turns out there is a difference between a user and a role. Users have the LOGIN permission applied by default. Do we still want ensureUsers renamed to ensureRoles?

A testcase would also be nice.

Indeed, I planned to add one once I'd finished the rest.

I just started attacking this - turns out there is a difference between a user and a role. Users have the LOGIN permission applied by default. Do we still want ensureUsers renamed to ensureRoles?

Btw, do you have an opinion/preference on this? I never received a response, which is why I stopped working on this PR.

I just started attacking this - turns out there is a difference between a user and a role. Users have the LOGIN permission applied by default. Do we still want ensureUsers renamed to ensureRoles?

Btw, do you have an opinion/preference on this? I never received a response, which is why I stopped working on this PR.

I think I prefer ensureUsers and ensureRoles separated, DBA PGSQL people knows what it means and how it differs, also it's synced with upstream docs.

I agree with @roberth's suggestion about a state management solution in the longterm, however

• ensureUsers is broken for quite a bunch of services apparently on master
• a new NixOS release is knocking on the door

so we'll need a solution.

Unless we go for a full revert this is IMHO the most reasonable solution[1] as it doesn't expand on the problematic ensure* approach.

I think I prefer ensureUsers and ensureRoles separated, DBA PGSQL people knows what it means and how it differs, also it's synced with upstream docs.

Just to be clear, you mean the way it's currently implemented in this PR, correct? If yes, I agree.

Inventing our own names means that everyone has to map the NixOS names onto the Postgres names when reading upstream docs and that seems like a very bad idea. And roles is the correct term for what we want to do now. Finally, I wouldn't want to bikeshed ensureUsers and block this PR on that now (and in the future the time is probably better invested into alternatives).

[1] Though an even simpler solution would be to unconditionally add the CREATE permission to restore the pre-15 behavior.

Okay I've had a look at some of the issues on master, and now I'm a bit confused (granted it is very late, so I'm looking at this through sleepy eyes).

PG15 doesn't add the CREATE permission on the public by default schema, right? That's not a role, so how would this help?

This PR adds ensureUsers.*.ensureRoles = ["role1" "role2"], which expands to GRANT role1,role2 TO user;

(Unless you're abusing the syntax with ensureRoles = ["CREATE"] expanding to GRANT CREATE TO ${user})?

Okay I've had a look at some of the issues on master, and now I'm a bit confused (granted it is very late, so I'm looking at this through sleepy eyes).

PG15 doesn't add the CREATE permission on the public by default schema, right? That's not a role, so how would this help?

This PR adds ensureUsers.*.ensureRoles = ["role1" "role2"], which expands to GRANT role1,role2 TO user;

(Unless you're abusing the syntax with ensureRoles = ["CREATE"] expanding to GRANT CREATE TO ${user})?

Excellent point. I am travelling ATM, I don't have PGSQL docs but this only hints to me that ensureRoles can do more and should have a more general name?

(Unless you're abusing the syntax with ensureRoles = ["CREATE"] expanding to GRANT CREATE TO ${user})?

This is a good point. I guess I was too focused on the user vs role thing that I missed we have a third entity that's relevant here, permissions 🤦 That's kinda embarassing and I'm very sorry for that.

Also unless I'm missing something that wouldn't even be sufficient, you'd have to commit crimes like ensureRoles = [ "CREATE ON SCHEMA public"]; (caution: untested!)

This means that

• I'm against the idea of this PR. The sole reason why I was OK with adding another ensure*-option is to fix an immediate issue.
• I see effectively two choices (short of dropping these options alltogether):
  • Make it possible to declare db ownership in ensureUsers and force everybody to use that.
  • Something like

diff --git a/nixos/modules/services/databases/postgresql.nix b/nixos/modules/services/databases/postgresql.nix
index 2d4ef0563182..f3fbabfab5a0 100644
--- a/nixos/modules/services/databases/postgresql.nix
+++ b/nixos/modules/services/databases/postgresql.nix
@@ -559,7 +559,10 @@ in
                 let
                   userPermissions = concatStringsSep "\n"
                     (mapAttrsToList
-                      (database: permission: ''$PSQL -tAc 'GRANT ${permission} ON ${database} TO "${user.name}"' '')
+                      (database: permission: ''
+                        $PSQL -tAc 'GRANT ${permission} ON ${database} TO "${user.name}"'
+                        $PSQL -U postgres -d wiki -tAc 'GRANT CREATE ON SCHEMA public TO "${user.name}"'
+                      '')
                       user.ensurePermissions
                     ); 

though we'd need a way to determine the database here properly (I hardcoded it to fix the wiki-js test which I just used as an example).

Yeah, then I'm not sure what the immediate path forward is here. Unfortunately, I have neither the time nor the patience to come up with a proper solution, getting it reviewed (potentially months), dealing with the inevitable bikeshedding, etc.

I'm using this as a standalone module as it solves an actual problem I had (a backup user needing access to everything).
If this is not the right way forward, that's fine. I'll close this PR now.

Standalone module, for reference:

{ config, lib, ... }: {
  options.services.postgresql.ensureUsers = with lib; mkOption {
    type = types.listOf (types.submodule {
      options = {
        ensureRoles = mkOption {
          type = types.listOf types.str;
          default = [];
          description = lib.mdDoc ''
            Roles to ensure for the user, specified as a list of strings.

            For more information on how to specify the role, see the
            [GRANT syntax](https://www.postgresql.org/docs/current/sql-grant.html).
            The attributes are used as `GRANT ''${role}.
          '';
          example = literalExpression ''
            [
              "pg_read_all_data"
              "pg_write_all_data"
            ]
          '';
        };
      };
    });
  };

  config = {
    systemd.services.postgresql.postStart = lib.concatMapStrings (user: let
        userRoles = lib.concatStringsSep "," user.ensureRoles;
      in ''
        $PSQL -tAc 'GRANT "${userRoles}" TO "${user.name}"'
      ''
    ) (lib.filter (user: lib.length user.ensureRoles != 0) config.services.postgresql.ensureUsers);
  };
}

Unfortunately, I have neither the time nor the patience to come up with a proper solution, getting it reviewed (potentially months), dealing with the inevitable bikeshedding, etc.

It's unfortunate because as we are running this immediate PostgreSQL 15 problem, us, contributors, were examining your solution as the most promising one for expedited review and merge. But it's fair.

Unfortunately, I have neither the time nor the patience to come up with a proper solution, getting it reviewed (potentially months), dealing with the inevitable bikeshedding, etc.

It's unfortunate because as we are running this immediate PostgreSQL 15 problem, us, contributors, were examining your solution as the most promising one for expedited review and merge. But it's fair.

I was probably a bit harsh, I apologise. I was just agitated.

As far as I understood, this solution can't be used to fix the pg15 problem in an elegant way, which is why I closed it.
It's about role membership, not user permissions. Unless I'm missing something?