Re: Flexible permissions for REFRESH MATERIALIZED VIEW - Mailing list pgsql-hackers
| From | Isaac Morland |
|---|---|
| Subject | Re: Flexible permissions for REFRESH MATERIALIZED VIEW |
| Date | |
| Msg-id | CAMsGm5c4DycKBYZCypfV02s-SC8GwF+KeTt==vbWrFn+dz=Keg@mail.gmail.com Whole thread Raw |
| In response to | Re: Flexible permissions for REFRESH MATERIALIZED VIEW (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: Flexible permissions for REFRESH MATERIALIZED VIEW
("David G. Johnston" <david.g.johnston@gmail.com>)
|
| List | pgsql-hackers |
Thanks for taking the time to look at this. I think I was unclear in a couple of places so I think my proposal may have appeared worse than it is. Details below:
Isaac Morland <isaac.morland@gmail.com> writes:
> The original idea was to allow access to REFRESH MATERIALIZED VIEW to be a
> grantable permission, rather than being reserved to the table owner.
I'm not really on board with making that a separately grantable
permission. You can do what you need today by having the matview be owned
by a single-purpose group role and granting membership in that role as
needed. We do not have an infinite supply of available privilege type
bits --- in fact, without breaking on-disk compatibility, there are only
four free bits in AclMode. So I can't see using one of them up for
REFRESH, let alone using three of them up for REFRESH, CLUSTER and
REINDEX.
I didn't mean to suggest introducing 3 new permissions, just one, which would allow all three operations (clustering and reindexing for tables, including matviews, and refreshing matviews). So that still leaves 3 remaining bits for future use. I recognize that using up the limited supply of privilege bits is a legitimate concern. However, I think of "refresh" as a general concept that may apply differently to different objects. A future permissions proposal may well be able to use it, similar to how USAGE was re-used when it was decided to have a permission on types. So we aren't fully using up even that bit, although clearly it is gone as to table-related permissions, and it shouldn't be used for non-table objects for something that doesn't feel like it can be described with the word REFRESH.
One question I would have is: what proposals exist or have existed for additional privilege bits? How much pressure is there to use some of the remaining bits? I actually looked into the history of the permission bits and found that we can summarize and approximate the history as 10 years of expansion from 4 to 12, then nothing added in the last 10 years.
1996-07-09 - first git commit with append/read/write/rule - total 4
2001-05-27 - split write into update and delete, rename append to insert and read to select, add references and trigger - total 7
2002-04-21 - add execute, usage, create, temp - total 11 - expand AclMode from uint8 to uint32
2004-01-14 - add connect - total 12
2006-09-05 - remove rule, leaving gap - total 11
2008-09-08 - add truncate - total 12
If we do end up running out, how hard would it be to change AclItem to have 2 uint64 fields, one for the actual permissions and one for the grant permissions? I haven't done a thorough study, but looking at the various macros defined in utils/acl.h and where they are used it looks to me like it might be not too bad (e.g. the only direct references to AclItem.ai_privs are in acl.c and acl.h). I'm concerned about changing the disk format, however - I don't know enough to say how painful that would be.
Also consider that these are the only non-DDL table-related actions that are not controlled by permission bits but instead can only be done by owner (at least I think they are - am I forgetting something?). Filling in that gap feels to me like a reasonable thing to want to do.
> The patch so far uses TRUNCATE permission to control access as a
> proof-of-concept.
I can't see doing that either, TBH. It's just ugly, and it surely doesn't
scale to cases where the conflated operations all apply to the same kind
of object. (For instance, including CLUSTER permissions in TRUNCATE
wouldn't be very sane.)
My original idea was to say that some combination of existing privileges would grant the ability to refresh a materialized view. But I came to prefer introducing a new privilege, especially once I realized it would be sensible to include clustering and reindexing. The only reason I used an existing privilege for this initial trivial version of the patch was to check that the concept actually works without going to the effort of actually writing in a new privilege. So I agree that using TRUNCATE in particular and any existing set of privileges more generally would not be my preference.
It's conceivable that we could somehow allow new bits to get overlaid
onto bits currently used only for other object types, without that being
user-visible. But I believe there are significant implementation issues
that'd have to be surmounted; for instance aclitemout has no idea what
sort of object the ACL it's printing is for. Also, the ACL machinery
doesn't currently think that "matview" is a distinct type of object
from "table" anyway; it just sees 'em all as "relation".
This sounds harder than changing AclItem!
Or, maybe I'm wrong and people feel that REFRESH-by-non-owner is important
enough to be worth consuming an AclMode bit for. But I'm dubious.
> I think backward compatibility is pretty good.
No, actually, backwards compatibility would be bad. If someone
had granted any privileges on their matview, which they almost certainly
would have if they cared about this scenario at all, then the owner's
privileges would have been instantiated in pg_class.relacl as
ACL_ALL_RIGHTS_RELATION, which doesn't (currently) include the
hypothetical new ACL_REFRESH bit. So after upgrading, they'd have to
explicitly grant themselves the new REFRESH right before they could
refresh. (I think pg_dump makes some attempt to cater for this case
by printing "GRANT ALL PRIVILEGES" in cases where all currently-extant
privileges are granted, but that's not very bulletproof; especially not
when pg_dump and server versions don't match, as they would not in an
upgrade situation.)
I will admit I did not think about the pg_dump issue much before. My immediate reaction was that I would need to dig into pg_dump and pg_upgrade quite a bit, to ensure that database update scenarios would result in self-grants of REFRESH whenever the source database had a self-grant. But after thinking it over a bit, I don't think it's possible to cover all scenarios. To do it properly, one has to add the new permission to all self-grants if the source of the dump was a pre-REFRESH version. But loading a dump simply consists of running a bunch of SQL commands through psql. The file itself, if generated by pg_dump, has comments indicating what version the source database was and what version pg_dump was, but this information won't be used by psql to edit GRANT commands on the fly.
As an alternative, we could decide that REFRESH is not self-revokable and make the permissions test be "has REFRESH privilege or is owner". This would be inconsistent with the other privileges but would ensure that no privileges disappeared during an upgrade. I think this is a better approach. A slight inconsistency between the new permission and older permissions is better than weird problems with upgraded databases not allowing object owners to refresh/cluster/reindex. With this change, the permissions are completely backward compatible: anybody who had permission before still does, and additional people can be authorized if needed, but by default no additional people are authorized.
Now, none of these things are particularly the fault of this patch;
the bottom line is we don't have a good design for adding privilege
types to existing object kinds. But nonetheless these are problems.
Thanks for taking the time to look at this. I should finish by saying that while the initial patch is trivial, I recognize that any useable patch for this concept will be non-trivial. I'm interested enough in the issue and with getting some experience with Postgres coding however that I'm willing to do the work necessary, as long as I have approval in principle from the community, meaning not a guarantee that the full patch will be applied, but sufficient approval that I think it likely that I can eventually get it into a form which will be considered acceptable.
pgsql-hackers by date: