Re: [PATCH] remove is_member_of_role() from header, add can_set_role() - Mailing list pgsql-hackers

From Tom Lane
Subject Re: [PATCH] remove is_member_of_role() from header, add can_set_role()
Date
Msg-id 254275.1635357633@sss.pgh.pa.us
Whole thread Raw
In response to Re: [PATCH] remove is_member_of_role() from header, add can_set_role()  ("Bossart, Nathan" <bossartn@amazon.com>)
List pgsql-hackers
"Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/27/21, 10:22 AM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
>> On Wed, Oct 27, 2021 at 1:12 PM Mark Dilger
>> <mark.dilger@enterprisedb.com> wrote:
>>> I don't understand the purpose of this.  You are defining can_set_role(member,role) as a simple wrapper around
is_member_of_role(member,role). Couldn't the comment: 

> I think a comment about the intended usage is sufficient.

I agree with the position that a better function header comment is
sufficient.  However, if we're to rename it, please not "can_set_role".
That's bad in at least two ways:

* it's quite unclear what "set" means in this context ... maybe the
function is testing whether you're allowed to alter properties of
the role, or do "ALTER ROLE ... SET parameter = value"?  It only makes
sense if you already know that the specific command SET ROLE is being
thought of.

* it's unclear which argument is which end of the relationship.

Something like "can_set_role_to()" would help with the second problem,
but I'm not sure it does much for the first one.

On the whole I think the existing name is fine.

            regards, tom lane



pgsql-hackers by date:

Previous
From: Joshua Brindle
Date:
Subject: Re: [PATCH] Conflation of member/privs for predefined roles
Next
From: "Bossart, Nathan"
Date:
Subject: Re: Isn't it better with "autovacuum worker...." instead of "worker took too long to start; canceled" specific to "auto