Re: [PATCH v2] use has_privs_for_role for predefined roles

Greetings,

On Sun, Mar 20, 2022 at 18:31 Joshua Brindle <joshua(dot)brindle(at)crunchydata(dot)com>
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.

Yeah, though that should really be very clearly explained in comments
around that code as anything that uses is_member should really be viewed as
an exception. I also wouldn’t be against using has_privs there anyway and
saying that you shouldn’t be trying to connect as one role on a replication
connection and using the privs of another when you don’t automatically
inherit those rights, but on the assumption that the committer intended
is_member there because SET ROLE isn’t something one does on replication
connections, I’m alright with it being as is.

Kind Regards,

Stephen P Frost

>