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