Re: Default Roles - Mailing list pgsql-hackers

On Mon, Feb 29, 2016 at 10:02 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Attached is a stripped-down version of the default roles patch which
> includes only the 'pg_signal_backend' default role.  This provides the
> framework and structure for other default roles to be added and formally
> reserves the 'pg_' role namespace.  This is split into two patches, the
> first to formally reserve 'pg_', the second to add the new default role.
>
> Will add to the March commitfest for review.

Here is a review of the first patch:

+       if (!IsA(node, RoleSpec))
+               elog(ERROR, "invalid node type %d", node->type);

That looks strange.  Why not just Assert(IsA(node, RoleSpec))?

+
+       return;

Useless return.

@@ -673,6 +673,7 @@ dumpRoles(PGconn *conn)                        "pg_catalog.shobj_description(oid,
'pg_authid') as rolcomment, "                                                 "rolname =
current_user AS is_current_user "                                                 "FROM pg_authid "
+                                                 "WHERE rolname !~ '^pg_' "
    "ORDER BY 2");       else if (server_version >= 90100)               printfPQExpBuffer(buf,
 
@@ -895,6 +896,7 @@ dumpRoleMembership(PGconn *conn)                                          "LEFT JOIN pg_authid ur
on
ur.oid = a.roleid "                                          "LEFT JOIN pg_authid um on
um.oid = a.member "                                          "LEFT JOIN pg_authid ug on
ug.oid = a.grantor "
+                                          "WHERE NOT (ur.rolname ~
'^pg_' AND um.rolname ~ '^pg_')"                                          "ORDER BY 1,2,3");

If I'm reading this correctly, when dumping a 9.5 server, we'll
silently drop any roles whose names start with pg_ from the dump, and
hope that doesn't break anything.  When dumping a 9.4 or older server,
we'll include those roles and hope that they miraculously restore on
the new server.  I'm thinking neither of those approaches is going to
work out, and the difference between them seems totally unprincipled.

@@ -631,7 +637,8 @@ check_is_install_user(ClusterInfo *cluster)       res = executeQueryOrDie(conn,
                                 "SELECT rolsuper, oid "                                                       "FROM
 
pg_catalog.pg_roles "
-                                                       "WHERE rolname
= current_user");
+                                                       "WHERE rolname
= current_user "
+                                                       "AND rolname
!~ '^pg_'");
       /*        * We only allow the install user in the new cluster (see comment below)
@@ -647,7 +654,8 @@ check_is_install_user(ClusterInfo *cluster)
       res = executeQueryOrDie(conn,                                                       "SELECT COUNT(*) "
-                                                       "FROM
pg_catalog.pg_roles ");
+                                                       "FROM
pg_catalog.pg_roles "
+                                                       "WHERE rolname
!~ '^pg_'");
       if (PQntuples(res) != 1)               pg_fatal("could not determine the number of users\n");

What bad thing would happen without these changes that would be
avoided with these changes?  I can't think of anything.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company