Re: [PATCHES] TRUNCATE, VACUUM, ANALYZE privileges - Mailing list pgsql-hackers

Tom, et al,

Sorry for the longish email; if you're most interested in a change to
the ACL system to allow more privileges then skip to the bottom where
I worked up a change to give us more options without much of a
performance impact (I don't think anyway).  Personally, I'd prefer that
to overloading the bits we have (except perhaps for vacuum/analyze).

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > GRANT ADD COLUMN, etc, aren't maintenance commands, they're DDL
> > statements and as such should be the purview of the owner.  TRUNCATE,
> > VACUUM and ANALYZE are DML commands and are commands a user of
> > the table would use through the normal course of inserting, updating or
> > deleteing data in the table.
>
> I find this reasoning fairly dubious.  In particular, it's hard to argue
> that there is no DDL component to TRUNCATE when it effectively does an
> implicit disable-triggers operation.  Another thing setting TRUNCATE
> apart from run-of-the-mill DDL operations is that it inherently violates
> MVCC rules (by deleting rows that should still be visible to concurrent
> transactions).

Kind of makes one wish you could know what tables were going to be
touched for a given transaction at the start of the transaction.  That's
not really here nor there tho.  I could see limiting truncate privileges
to tables which don't have on-delete triggers, that doesn't help with
the MVCC problem though and ends up being why we can't just use 'delete'
privileges for it.

Could we base vacuum and analyze rights off of other privileges though?
vacuum allowed if yoou have 'delete' privileges, analyze if you have
'insert', 'update', or 'delete'?  And for 'truncate' permissions...

> But my real problem with the approach is that I don't see where it
> stops.  If you're allowed to do ANALYZE, why not ALTER TABLE SET
> STATISTICS?  If you're allowed to do TRUNCATE, why not the
> recently-discussed ALTER TABLE SET RELIABILITY?  And how about CLUSTER?
> All of these could be pretty useful for some applications not too far
> removed from yours.  And there will be someone wanting a bit for
> DISABLE/ENABLE TRIGGER coming along right afterwards.  Must we implement
> a separate nonstandard privilege bit for every operation that someone
> comes up and wants a bit for, if they have the necessary cut-and-paste
> skill to submit a patch for it?

I think analyze is distinct from set statistics.  SET STATISTICS, if
used improperly (perhaps by mistake or misunderstanding), could cause
serious havoc on the system as potentially very poor plans are chosen
because the statistics aren't at all correct.  I don't see how running
analyze a couple times would have a detrimental effect (except for the
effort of running the analyze itself, but that's really not all that
much and if they want to DoS the box in that way there are other
things they can do).  SET STATISTICS is also hopefully something you're
not having to change or do every time you add/remove/update data.

SET RELIABILITY is a more interesting question since it could be used in
a situation similar to why truncate's popular (mass data
loading/reloading).  The same is true for disable/enable triggers.

> I'd feel happier about an approach that adds *one* privilege bit
> covering a range of operations that we agree to be useful.  This will
> avoid chewing a disproportionate amount of ACL storage space, and it
> will force us to confront the decision about which operations are out
> as well as which are in.

If we modify VACUUM/ANALYZE to be based off what I suggested above, then
we could add just one 'BYPASS' permission bit which would allow
TRUNCATE right now and then SET RELIABILITY, and DISABLE/ENABLE TRIGGER
later.  I'm not a particularly big fan of this though because, while I'd
like to be able to give TRUNCATE permissions I'm not a big fan of SET
RELIABILITY because it would affect PITR backups.  I suppose a user
would still have to intentionally do that though and so they'd have only
themselves to blame if the data they loaded wasn't part of the backup.
DISABLE/ENABLE TRIGGER has a similar issue though because that could
probably be used to bypass CHECK and REFERENCES constraints, which I
wouldn't want to allow.

A BYPASS bit would be better than having to give ownership rights
though.  We could also look at restructuring the ACL system to be able
to handle more permissions better.  As was suggested elsewhere, one
option would be to have a seperate set of ACLs (at least internally)
such that a given set of commands/permissions would be associated
with one set or the other set (and hopefully rarely, both).  These could
be divided up by either level or frequency (which I think would actually
result in the same set).  Level would be row/table/database, frequency
would be estimation of usage in the real world.  This would seperate
SELECT, INSERT, UPDATE, DELETE into one set of permissions, and then
REFERENCES, RULE, TRIGGER, TRUNCATE, SET RELIABILITY, DISABLE/ENABLE
TRIGGER, CLUSTER into the other set.  I havn't looked into this very
deeply but I like the basic idea of it better than having a 'catch-all'
permission bit.

Looking into utils/acl.h, grant options are pretty rarely accessed, even
less so than a 'truncate' permission would be, and I could see changing
AclItem to have:

typedef struct AclItem
{ Oid ai_grantee;             /* ID that this item grants privs to */ Oid ai_grantor;             /* grantor of privs
*/AclMode ai_privs;               /* privilege bits */ AclMode aig_privs;              /* grant option bits */ 
} AclItem;

This makes AclItem slightly larger but I don't think it would have a
detrimental affect on performance other than that, it seems unlikely
that AclItem is scanned in such a way that the structure size would
adversely affect performance much.  There's cacheing issues I suppose
but an extra couple of bytes doesn't seem all that terrible.

This would give us an extra 16 bits to work with though for ACL
privileges.  At the moment we're talking about adding perhaps 6, or 4 if
we do the overloading for VACUUM/ANALYZE I suggested, which means still
having 15 or 17 bits left.  We could postpone the grant split till we
actually go over 16 ACL bits used but I don't think there's much need
to.

If that's more palatable then I'd be happy to work up the changes to the
ACL system to implement that.
Thanks,
    Stephen