Re: Flexible permissions for REFRESH MATERIALIZED VIEW

Greetings,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, May 15, 2018 at 6:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > That seems like an awful lot of work to handle what's still going to be
> > a pretty small set of permissions. Every permission we add is going to
> > have to be enforced in the C code, and it'll break applications to some
> > extent to treat the situation differently from before, so I don't see
> > that we're going to add them willy-nilly.
> >
> > (For what it's worth, my first instinct would be to lump all three of
> > these proposals under a single grantable permission MAINTAIN, or some
> > such name. I don't think it's worth subdividing more finely.)
>
> That's certainly a fair opinion, but I'm just not sure whether users
> are going to agree.

I'm not a big fan of it- what happens when we introduce something else
which would seem like it'd fall under 'maintain' but provides some
capability that maybe it wouldn't be good for users who could only run
the above commands to have? I'm tempted to suggest that, really, we
might even be thinking about splitting up things further than the above
proposal- what about VACUUM vs. VACUUM FULL? Or REFRESH MATVIEW vs.
REFRESH MATVIEW CONCURRENTLY? Mistakes between those routinly cause
problems due to the heavy lock taken in some cases- as an administrator,
I'd be a lot more comfortable giving a user or some process the ability
to run a VACUUM vs. VACUUM FULL.

> >> To handle the on-disk issue, I think we could introduce a new varlena
> >> type that's like aclitem but permits extra variable-length data at the
> >> end. It would be a different data type but pretty easy to convert
> >> back and forth. Still probably a lot of work to make it happen,
> >> though, unfortunately.
> >
> > I think an appropriate amount of effort would be to widen AclMode to 64
> > bits, which wasn't practical back in the day but now we could do easily
> > (I think). That would move us from having four spare permission bits
> > to having 20. I don't think it'd cause an on-disk compatibility break
> > because type aclitem is generally only stored in the catalogs anyway.
>
> How much are we worried about users (or extensions) who have used it
> in user tables? We could introduce aclitem2 and keep the old one
> around, I guess.

I'm amazed we're even discussing these concerns. I don't see the
"on-disk" change as being a real issue since they're really only in the
catalogs and not advertised as a user-space type. We've also not
worried about breaking it in the past when we introduced new privileges.

I can see an argument for adding a check to pg_upgrade to bail if an
aclitem data type is found. I'm really not thrilled with the idea of
introducing a data type to handle this though- that strikes me as just
unnecessary code complication.

> If we're going to go to the trouble of making an incompatible change
> to aclitem, it seems like we should go all the way and make it into
> some kind of varlena type. Realistically, if we add another 32 bits
> to it, you're going to let 3 or 4 new permissions through -- max --
> and then go back to worrying about how many bits we have left and how
> quickly we're eating them up. I guess if somebody else is doing the
> work I'll be content to let them do it how they want to do it, but if
> I were doing the work, I would look for a bigger hammer.

When I've looked at this previously, it struck me that splitting up the
two halves of the ACL would be the way to go, especially since only the
GRANT/REVOKE commands actually care about the WITH ADMIN OPTION bits.
In past years I figured that would mean two uint32's, but these days I
don't think going to two uint64's would be a bad idea, except for one
downside- the increase in the size of aclitem itself.

One option for dealing with that might be to move the WITH ADMIN OPTION
bits out into their own table, since we don't care about them in the
general case. That's a bit radical and I've not looked into what it'd
take, but it does seem like that would have made more sense in a green
field.

I'm not entirely sure about the varlena suggestion, seems like that
would change a great deal more code and be slower, though perhaps not
enough to matter; it's not like our aclitem arrays are exactly optimized
for speed today.

In any case, I'm definitely all for adding more privileges to the system
and I'd really rather we not lump distinct actions into a single "grant
all" privilege- taken to extreme, that's what superuser is, and we're
actively trying to get away from that, for good reason. I do think we
need to solve the "we need more bits" problem before burning more though
(of course, I thought that was the general consensus before TRUNCATE get
a privilege bit...).

Thanks!

Stephen