Date: Sat, 29 Apr 2023 22:21:27 -0700
From: Noah Misch <noah@leadboat.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: tushar <tushar.ahuja@enterprisedb.com>,
	Nathan Bossart <nathandbossart@gmail.com>,
	Joe Conway <mail@joeconway.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Stephen Frost <sfrost@snowman.net>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: replacing role-level NOINHERIT with a grant-level option
Message-ID: <20230430052127.GA2271597@rfd.leadboat.com>
References: <CAC6VRoZLfROLLsCOVkOSmPRwRS5h4Wyw_ntwaQ1K9s_HQp9Y+w@mail.gmail.com>
 <CA+Tgmobep7Zi1MCKhhUa1HnJZahf+Zj3aP7OoPAQzcAvEv56cw@mail.gmail.com>
 <8210e573-c861-fe45-302b-56d7352aae48@enterprisedb.com>
 <CA+TgmoauWDcn53qwUNR_ohof1HPoVhoP12xtAMC0FiObZvpVcw@mail.gmail.com>
 <bf371eff-a98b-2b8f-6c28-95c7f3cdd523@enterprisedb.com>
 <CA+Tgmob6pVopLZ+NJuy_AxDS0uvgHhjjEBYQTBZXek-vM9dmGw@mail.gmail.com>
 <030c1dbf-5a8e-706d-698c-b1cdfcb6ce0f@enterprisedb.com>
 <CA+TgmobWFL5eU=60egYuzL0C0cgaVFEOVriTk_Rbs_cpj=MreQ@mail.gmail.com>
 <dab97d44-3859-f58f-ff4c-aa79d3278869@enterprisedb.com>
 <CA+TgmoYkbcH=njVN-vbRDorj9VqTGNzGHCYizO_5k1RWxGOQfQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+TgmoYkbcH=njVN-vbRDorj9VqTGNzGHCYizO_5k1RWxGOQfQ@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: <https://www.postgresql.org/message-id/20230430052127.GA2271597%40rfd.leadboat.com>
Precedence: bulk

On Thu, Aug 25, 2022 at 10:19:39AM -0400, Robert Haas wrote:
> I read through this again and found a comment that needed to be
> updated, so I did that, bumped catversion, and committed this.

[commit e3ce2de]

> @@ -4735,8 +4735,8 @@ initialize_acl(void)
>  
>  		/*
>  		 * In normal mode, set a callback on any syscache invalidation of rows
> -		 * of pg_auth_members (for roles_is_member_of()), pg_authid (for
> -		 * has_rolinherit()), or pg_database (for roles_is_member_of())
> +		 * of pg_auth_members (for roles_is_member_of()) pg_database (for
> +		 * roles_is_member_of())
>  		 */
>  		CacheRegisterSyscacheCallback(AUTHMEMROLEMEM,
>  									  RoleMembershipCacheCallback,

I agree one could remove the "CacheRegisterSyscacheCallback(AUTHOID, ...)".
This updated the comment as though the patch were including that removal, but
AUTHOID remains.  Also, that comment needs s/pg_database/or &/.


These sites didn't change in v16 and may or may not warrant change:

doc/src/sgml/catalogs.sgml:1522:       <structfield>rolinherit</structfield> <type>bool</type>
doc/src/sgml/system-views.sgml:2585:       <structfield>rolinherit</structfield> <type>bool</type>
src/include/catalog/pg_authid.h:36:	bool		rolinherit;		/* inherit privileges from other roles? */

I likely would leave pg_authid.h as-is but change the doc/ phrases.


https://postgr.es/m/17901-93eacb513e503f43%40postgresql.org led me to notice
that v16 always inherits the implicit membership in role pg_database_owner,
with no way to override like one could in v15.  That message's test procedure
doesn't "fail" in v16.  I think that's fine, but I'm mentioning it since
pg_database_owner didn't appear upthread.