Greetings, On Sun, Mar 20, 2022 at 18:31 Joshua Brindle <joshua.brin...@crunchydata.com> 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. 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 >
