Re: [PATCH v2] use has_privs_for_role for predefined roles

On Sun, Mar 20, 2022 at 12:34 PM Joe Conway <mail(at)joeconway(dot)com> wrote:
>
> On 3/20/22 12:31, Joshua Brindle wrote:
> > On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail(at)joeconway(dot)com> wrote:
> >>
> >> On 3/3/22 11:26, Joshua Brindle wrote:
> >> > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail(at)joeconway(dot)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 ¯\_(ツ)_/¯
>

I don't have a strong opinion either way, just noting that it's a
possible exception area due to how it is used.

Thanks for committing this.