Re: Default Roles

* Robert Haas (robertmhaas@gmail.com) wrote:
> 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))?

Sure, that works, done.

> +
> +       return;
>
> Useless return.

Removed.

> @@ -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.

That needed to be updated to be 9.6 and 9.5, of course.

Further, you make a good point that 9.6's pg_dumpall should really
always exclude any roles which start with 'pg_', throwing a warning if
it finds them.

Note that pg_upgrade won't proceed with an upgrade of a system that has
any 'pg_' roles.

> @@ -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.

This function ("check_is_install_user") is simply checking that the user
we are logged in as is the 'install' user and that there aren't any
other users in the destination cluster.  The initial check is perhaps a
bit paranoid- it shouldn't be possible for a 'pg_' role to be the one
which pg_upgrade has logged into the cluster with as none of the 'pg_'
roles have the 'login' privilege.

For the second check, pg_upgrade expects a pristine cluster to perform
the binary upgrade into, which includes checking that there aren't any
roles besides the 'install' role in the destination cluster.  Since
default roles are created at initdb time, the destination cluster *will*
have other roles in it besides the install role, but only roles whose
names start with 'pg_', and those are ok because they're from initdb.

Updated (and rebased) patch attached.

Thanks for the review!

Stephen