Re: replacing role-level NOINHERIT with a grant-level option

On Wed, Jun 29, 2022 at 7:19 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > 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.

Yeah, I think if we're going to ERROR we should just remove the syntax
support entirely. A syntax error by any other name is still a syntax
error. If we're going to go with a WARNING there's some value in that
to allow existing dumps to be sorta-restored on new versions, but this
is also moot if we don't end up deprecating this syntax after all.

> 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 don't see why we need an option. pg_dump's charter is to produce
output suitable for the server version that matches the pg_dump
version. Existing pg_dump versions will do the right thing for the
server versions they support, and in the v16, we can just straight up
change the behavior to produce the syntax that v16 wants.

> > 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.

I don't think that it's a good idea to disallow grant-level options
when a role-level option is set, for two reasons. First, it would
necessitate restricting the ability to ALTER the role-level option;
otherwise no invariant could be maintained. Second, I'm interested in
this feature because I want to be able to perform a role grant that
will have a well-defined behavior without knowing what role-level
options are set. I thought initially that I wouldn't be able to
accomplish my goals if we kept the role-level options around, but now
I think that's not true. However, I think I need the grant-level
option to work regardless of how the role-level option is set. The
problem with the role-level option is essentially that you can't
reason about what a new grant will do.

-- 
Robert Haas
EDB: http://www.enterprisedb.com