On Mon, Feb 29, 2016 at 10:02 PM, Stephen Frost <sfr...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers