Re: [PATCH v2] use has_privs_for_role for predefined roles

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.

> On that last note, I did not find basebackup_to_shell.required_role
> documented at all, and did not attempt to fix that.
>
> When this thread petered out, it seemed that Robert was at least neutral
> on the patch, and everyone else was +1 on applying it to master for pg15.
>
> As such, if there are any other issues, complaints, etc., please speak
> real soon now...
>
> Thanks,
>
> Joe