Date: Wed, 29 Jun 2022 16:19:39 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Joe Conway <mail@joeconway.com>,
	"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: <20220629231939.GA362451@nathanxps13>
References: <0ED10807-9E7F-4BB5-9473-4668EE68AE2D@amazon.com>
 <4117557.1641329799@sss.pgh.pa.us>
 <CAGB+Vh4jWh-KuV6qt0FKC3M=HCZT1X=Kn5E1g68s1+0OSfoCdQ@mail.gmail.com>
 <c81ba78d-ae4f-02bc-9518-61cbe00c1f2e@joeconway.com>
 <730520.1644168290@sss.pgh.pa.us>
 <CA+TgmoYeNB1PxMO-TYv4_Ky7XR3-3Le+fpL87T7QMR=OWc7U1Q@mail.gmail.com>
 <80af9ba7-ec24-9134-8fd9-00bf13f5c494@joeconway.com>
 <CA+Tgmoax3J2D1RoqSqBRuLVifMjinZ5SPvR+rBqBT89u57zOJQ@mail.gmail.com>
 <1066202.1654190251@sss.pgh.pa.us>
 <CA+TgmoYmJtLA1aFF2dpGLuqWMuWwrf2SEoT-Ueqd4XCzLL61sA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoYmJtLA1aFF2dpGLuqWMuWwrf2SEoT-Ueqd4XCzLL61sA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220629231939.GA362451%40nathanxps13>
Precedence: bulk

On Wed, Jun 22, 2022 at 04:30:36PM -0400, Robert Haas wrote:
> On Thu, Jun 2, 2022 at 1:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Point 2 would cause every existing pg_dumpall script to fail, which
>> seems like kind of a large gotcha.  Less unpleasant alternatives
>> could include
>>
>> * Continue to accept the syntax, but ignore it, maybe with a WARNING
>> for the alternative that doesn't correspond to the new behavior.
>>
>> * Keep pg_authid.rolinherit, and have it act as supplying the default
>> behavior for subsequent GRANTs to that role.
> 
> Here's a rather small patch that does it the first way.

I've been thinking about whether we ought to WARNING or ERROR with the
deprecated syntax.  WARNING has the advantage of not breaking existing
scripts, but users might not catch that NOINHERIT roles will effectively
become INHERIT roles, which IMO is just a different type of breakage.
However, I don't think I can defend ERROR-ing and breaking all existing
pg_dumpall scripts completely, so perhaps the best we can do is emit a
WARNING until we feel comfortable removing the option completely 5-10 years
from now.

I'm guessing we'll also need a new pg_dumpall option for generating pre-v16
style role commands versus the v16+ style ones.  When run on v16 and later,
you'd have to require the latter option, as you won't always be able to
convert grant-level inheritance options into role-level options.  However,
you can always do the reverse.  I'm thinking that by default, pg_dumpall
would output the style of commands for the current server version.
pg_upgrade would make use of this option when upgrading from <v16 to v16
and above.  Is this close to what you are thinking?

> I suppose if we did it the second way, we could make the syntax GRANT
> granted_role TO recipient_role WITH INHERIT { TRUE | FALSE | DEFAULT
> }, and DEFAULT would copy the current value of the rolinherit
> property, so that changing the rolinherit property later would not
> affect previous grants. The reverse is also possible: with the same
> syntax, the rolinherit column could be changed from bool to "char",
> storing t/f/d, and 'd' could mean the value of the rolinherit property
> at time of use; thus, changing rolinherit would affect previous grants
> performed using WITH INHERIT DEFAULT but not those that specified WITH
> INHERIT TRUE or WITH INHERIT FALSE.

Yeah, something like this might be a nice way to sidestep the issue.  I was
imagining something more like your second option, but instead of continuing
to allow grant-level options to take effect when rolinherit was true or
false, I was thinking we would ignore them or even disallow them.  By
disallowing grant-level options when a role-level option was set, we might
be able to avoid the confusion about what takes effect when.  That being
said, the syntax for this sort of thing might not be the cleanest.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com