Re: [PATCH v2] use has_privs_for_role for predefined roles

On 3/20/22 12:31, Joshua Brindle wrote:
> On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail@joeconway.com> wrote:
>>
>> On 3/3/22 11:26, Joshua Brindle wrote:
>> > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com> wrote:
>> >>
>> >> On 2/10/22 14:28, Nathan Bossart wrote:
>> >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>> >> >> On 2/9/22 13:13, Nathan Bossart wrote:
>> >> >>> I do wonder if users find the differences between predefined roles and role
>> >> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
>> >> >>> govern predefined roles when this patch is applied.  Maybe the role
>> >> >>> attribute system should eventually be deprecated in favor of using
>> >> >>> predefined roles for everything.  Or perhaps the predefined roles should be
>> >> >>> converted to role attributes.
>> >> >>
>> >> >> Yep, I was suggesting that the latter would have been preferable to me while
>> >> >> Robert seemed to prefer the former. Honestly I could be happy with either of
>> >> >> those solutions, but as I alluded to that is probably a discussion for the
>> >> >> next development cycle since I don't see us doing that big a change in this
>> >> >> one.
>> >> >
>> >> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
>> >> > for v15.
>> >>
>> >> +1
>> >>
>> >> I am planning to get into it in detail this weekend. So far I have
>> >> really just ensured it merges cleanly and passes make world.
>> >
>> > Rebased patch to apply to master attached.
>>
>> Well longer than I planned, but finally took a closer look.
>>
>> I made one minor editorial fix to Joshua's patch, rebased to current
>> master, and added two missing call sites that presumably are related to
>> recent commits for pg_basebackup.
> 
> FWIW I pinged Stephen when I saw the basebackup changes included
> is_member_of and he didn't think they necessarily needed to be changed
> since they aren't accessible to human and you can't SET ROLE on a
> replication connection in order to access the role where inheritance
> was blocked.

Maybe worth a discussion, but it seems strange to me to treat them 
differently when the whole purpose of this patch is to make things 
consistent ¯\_(ツ)_/¯

Joe