On Thu, Mar 24, 2022 at 1:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > However, it might. And if it does, I think it would be best if
> > removing that exception were the *only* change in this area made by
> > that release.
>
> Good idea, especially since it's getting to be too late to consider
> anything more invasive anyway.
I'd say it's definitely too late at this point.
> > So I propose to commit something like what I posted here:
> > http://postgr.es/m/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com
>
> +1, although the comments might need some more work. In particular,
> I'm not sure that this bit is well stated:
>
> + * A role cannot have WITH ADMIN OPTION on itself, because that would
> + * imply a membership loop.
>
> We already do consider a role to be a member of itself:
>
> regression=# create role r;
> CREATE ROLE
> regression=# grant r to r;
> ERROR: role "r" is a member of role "r"
> regression=# grant r to r with admin option;
> ERROR: role "r" is a member of role "r"
>
> It might be better to just say "By policy, a role cannot have WITH ADMIN
> OPTION on itself". But if you want to write a defense of that policy,
> this isn't a very good one.
That sentence is present in the current code, along with a bunch of
other sentences, which the patch renders irrelevant. So I just deleted
all of the other stuff and kept the sentence that is still relevant to
the revised code. I think your proposed replacement is an improvement,
but let's be careful not to get sucked into too much of a wordsmithing
exercise in a patch that's here to make a functional change.
--
Robert Haas
EDB: http://www.enterprisedb.com
