Thread: Flexible permissions for REFRESH MATERIALIZED VIEW

Flexible permissions for REFRESH MATERIALIZED VIEW

From
Isaac Morland
Date:
This is a proposal for a Postgres feature enhancement. I've attached a preliminary patch. However, the patch is extremely preliminary: there is no documentation or testing change, and I think I actually want to make the change itself in a different way from what this 2-line patch does.

Right now I'm really looking for whether anybody observes any problems with the basic idea. If it's considered to be at least in principle a good idea then I'll go and make a more complete patch.

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 found that permission checking is done in RangeVarCallbackOwnsTable(), which is also used for CLUSTER and REINDEX.

My initial implementation was to duplicate RangeVarCallbackOwnsTable() and make a new version just for REFRESH, but then I realized that allowing CLUSTER and REINDEX to be grantable permissions also might make sense so the attached patch just modifies RangeVarCallbackOwnsTable().

The patch so far uses TRUNCATE permission to control access as a proof-of-concept. However, I am considering whether we could add a new REFRESH permission ('R'?) on tables (including materialized views) to control access. Of course, we would also rename RangeVarCallbackOwnsTable() to accurately describe its new function.

When I first had the idea, one concern I had was what would happen to the security context during REFRESH, but it turns out that checking whether the operation is allowed and actually setting up the context are completely separate, I think so that REFRESH triggered by superuser and by the owner (or for that matter by a role which is a member of the owner) result in the same security context. So the same should apply if we allow other users to do a REFRESH.

I think backward compatibility is pretty good. If the feature is ignored and no REFRESH permissions are granted, then it should work out to the same as what we have now: owner and superuser are considered to have all table permissions. pg_upgrade might need to upgrade explicit owner permissions - I'm not yet absolutely clear on how those work. Anybody who wants the new feature would be able to use it by granting the new permission, while for anybody who doesn't need it things should continue working as before, with the only difference being the exact error message resulting from a permission violation.

I think the feature is pretty well justified in terms of the overall Postgres authorization model too. DDL can in general be changed only by object owners: e.g., renaming, altering columns, that sort of thing. Changing relation contents, however, is a grantable privilege...but not when it comes to refreshing materialized views or clustering or reindexing indexes. So this just makes it so that more non-DDL changes are grantable. Arguably CLUSTER should require the owner to change on which index the table is clustered, but my inclination is not to have that additional complication.

I welcome any comments, questions, and suggestions.
Attachment

Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Tom Lane
Date:
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.

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

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

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

We could avoid that set of problems if we just redefined TRUNCATE as
meaning "TRUNCATE or REFRESH", but then you find out that people who
didn't have REFRESH ability before suddenly do.  Admittedly there was
no reason to grant such a privilege on a matview before, so maybe
people didn't; but by the same token there was no harm in granting
such a privilege, so maybe people did.  It's certainly not free from
the backwards-compatibility standpoint.

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.

            regards, tom lane


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Isaac Morland
Date:
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:

On 18 March 2018 at 20:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.

Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
"David G. Johnston"
Date:
On Wed, Mar 28, 2018 at 6:38 PM, Isaac Morland <isaac.morland@gmail.com> wrote:
​​
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. 

​I made an argument for an "ANALYZE" grant a little while back, and it kinda leads one to want one for VACUUM as well.


​David J.​

Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Isaac Morland
Date:
Thanks for pointing me to this. I also did a search in the archives and found a 2006 thread on TRUNCATE, VACUUM, and ANALYZE privileges:


I'm not seeing much else. As far as I can see, the only demand for using more privilege bits is for VACUUM, ANALYZE, REFRESH, CLUSTER, and REINDEX.

So revised proposal: instead of calling it REFRESH, call it MAINTAIN. Anybody with MAINTAIN permission can do any of those 5 different maintenance actions as if they were the owner of the relation in question.

This uses just 1 bit, leaving 3 more for future expansion, and satisfying all the outstanding requests to allocate privilege bits. I think. That seems like a pretty good deal to me. Also, if a future proposal comes along, it may be appropriate to re-use this new permission at that time, as long as the word "maintain" makes even a little sense as the name in the new context.

We would probably have to apply the same rule to all of these, that the owner can always perform these actions, because of the issue with dumps restored from older databases.


On 28 March 2018 at 21:56, David G. Johnston <david.g.johnston@gmail.com> wrote:
On Wed, Mar 28, 2018 at 6:38 PM, Isaac Morland <isaac.morland@gmail.com> wrote:
​​
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. 

​I made an argument for an "ANALYZE" grant a little while back, and it kinda leads one to want one for VACUUM as well.


​David J.​


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Robert Haas
Date:
On Wed, Mar 28, 2018 at 9:56 PM, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> On Wed, Mar 28, 2018 at 6:38 PM, Isaac Morland <isaac.morland@gmail.com>
> wrote:
>> 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.
>
> I made an argument for an "ANALYZE" grant a little while back, and it kinda
> leads one to want one for VACUUM as well.

Yeah, and FWIW, I think that's a totally reasonable request, as is
this one.  The problem is that our authentication model seems to have
been designed under the assumption that there weren't all that many
different things you might want to separately GRANT, and the requests
we've had over the years show that this isn't the case.  So the
request is reasonable; it's just hard to implement.  I think we should
somehow move to a system where there's a set of "core" permissions
that are identified by bits for efficiency, and a set of "extended"
permissions which are identified by names for extensibility.  Things
like VACUUM and ANALYZE and REFRESH could be extended permissions.

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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 28, 2018 at 9:56 PM, David G. Johnston
> <david.g.johnston@gmail.com> wrote:
>> I made an argument for an "ANALYZE" grant a little while back, and it kinda
>> leads one to want one for VACUUM as well.

> Yeah, and FWIW, I think that's a totally reasonable request, as is
> this one.  The problem is that our authentication model seems to have
> been designed under the assumption that there weren't all that many
> different things you might want to separately GRANT, and the requests
> we've had over the years show that this isn't the case.  So the
> request is reasonable; it's just hard to implement.  I think we should
> somehow move to a system where there's a set of "core" permissions
> that are identified by bits for efficiency, and a set of "extended"
> permissions which are identified by names for extensibility.  Things
> like VACUUM and ANALYZE and REFRESH could be extended permissions.

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

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

            regards, tom lane


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Robert Haas
Date:
On Tue, May 15, 2018 at 6:07 PM, Tom Lane <tgl@sss.pgh.pa.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.

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

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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Stephen Frost
Date:
Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, May 15, 2018 at 6:07 PM, Tom Lane <tgl@sss.pgh.pa.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

Attachment

Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Robert Haas
Date:
On Fri, May 18, 2018 at 8:13 PM, Stephen Frost <sfrost@snowman.net> wrote:
> 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.

That is a fair point, but if we want to do things like that then it's
really not a good idea to limit ourselves to a fixed number of bits,
even if it's 2x or 4x more than what we have today.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Greg Stark
Date:
On 19 May 2018 at 01:13, Stephen Frost <sfrost@snowman.net> wrote:
>
> 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.

I don't actually understand the reason y'all are talking about
varlena. The aclitem type is already not passbyvalue so there's no
particular limit on the size and specifically no reason it can't be
larger than 64 bytes. As Tom mentioned it's only used in catalogs so
there's no on-disk version compatibility issue either. It can be
defined to have a bitmask exactly large enough to hold all the acl
privileges needed for the specific version and still not be a varlena.

The only downsides are:

1. At some point it gets large enough that adding rarely used
privileges is imposing a large storage and cache efficiency cost
that's amortized over all the acl lookups even when it's not used. I
doubt we're anywhere close to that currently but if we started having
hundreds of privileges...

2. The current macros just do a bitshift to look up bits and they
would get a bit more complex. But it's not like it isn't arithmetic we
haven't tackled repeatedly in other places that do bitmasks such as
varbit, numeric, bitmapset, and probably more.

Fwiw I don't understand why the current AclMode uses a single uint32
to pass around two 16-bit bitmasks and uses bitshifting to extract the
two. Why not just make it a struct with two uint16s in it instead?
That would mean we would have a factor of four available before the
macros even get the slight added complication.

-- 
greg


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Robert Haas
Date:
On Sat, May 19, 2018 at 12:59 PM, Greg Stark <stark@mit.edu> wrote:
> On 19 May 2018 at 01:13, Stephen Frost <sfrost@snowman.net> wrote:
>> 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.
>
> I don't actually understand the reason y'all are talking about
> varlena.

Because aclitem's typlen value in pg_type is currently "12".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Dmitry Dolgov
Date:
> On Mon, 21 May 2018 at 15:46, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sat, May 19, 2018 at 12:59 PM, Greg Stark <stark@mit.edu> wrote:
> > On 19 May 2018 at 01:13, Stephen Frost <sfrost@snowman.net> wrote:
> >> 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.
> >
> > I don't actually understand the reason y'all are talking about
> > varlena.
>
> Because aclitem's typlen value in pg_type is currently "12".

This patch went through the last two commit fests without any noticeable
activity. As far as I can see, judging from the discussion, there isn't a
single opinion everyone would agree with, except that simply introducing a new
permission is probably not enough and we need to address how to do this
in an extendable way.

> On Sun, 18 Mar 2018 at 22:05, Isaac Morland <isaac.morland@gmail.com> wrote:
>
> Right now I'm really looking for whether anybody observes any problems with
> the basic idea. If it's considered to be at least in principle a good idea
> then I'll go and make a more complete patch.

There were at least two options suggested about how to address the questions
from the discussion (widening AclMode and introducing another type similar to
aclitem, but more flexible, to store an "extended" permissions). Maybe the
constructive approach here would be to try them out and propose a draft
implementation for one of them?


Re: Flexible permissions for REFRESH MATERIALIZED VIEW

From
Dmitry Dolgov
Date:
On Mon, Nov 5, 2018 at 4:19 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> This patch went through the last two commit fests without any noticeable
> activity. As far as I can see, judging from the discussion, there isn't a
> single opinion everyone would agree with, except that simply introducing a new
> permission is probably not enough and we need to address how to do this
> in an extendable way.
>
> > On Sun, 18 Mar 2018 at 22:05, Isaac Morland <isaac.morland@gmail.com> wrote:
> >
> > Right now I'm really looking for whether anybody observes any problems with
> > the basic idea. If it's considered to be at least in principle a good idea
> > then I'll go and make a more complete patch.
>
> There were at least two options suggested about how to address the questions
> from the discussion (widening AclMode and introducing another type similar to
> aclitem, but more flexible, to store an "extended" permissions). Maybe the
> constructive approach here would be to try them out and propose a draft
> implementation for one of them?

Due to lack of response I'm marking it as "Returned with feedback". Feel free
to resubmit a new version though.