Thread: predefined role(s) for VACUUM and ANALYZE
Hi hackers, The previous attempt to add a predefined role for VACUUM and ANALYZE [0] resulted in the new pg_checkpoint role in v15. I'd like to try again to add a new role (or multiple new roles) for VACUUM and ANALYZE. The primary motivation for this is to continue chipping away at things that require special privileges or even superuser. VACUUM and ANALYZE typically require table ownership, database ownership, or superuser. And only superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these operations would allow delegating such tasks (e.g., a nightly VACUUM scheduled with pg_cron) to a role with fewer privileges. The attached patch adds a pg_vacuum_analyze role that allows VACUUM and ANALYZE commands on all relations. I started by trying to introduce separate pg_vacuum and pg_analyze roles, but that quickly became complicated because the VACUUM and ANALYZE code is intertwined. To initiate the discussion, here's the simplest thing I could think of. An alternate approach might be to allow using GRANT to manage these privileges, as suggested in the previous thread [1]. Thoughts? [0] https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com [1] https://postgr.es/m/20211104224636.5qg6cfyjkw52rh4d@alap3.anarazel.de -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Jul 22, 2022 at 01:37:35PM -0700, Nathan Bossart wrote: > The attached patch adds a pg_vacuum_analyze role that allows VACUUM and > ANALYZE commands on all relations. I started by trying to introduce > separate pg_vacuum and pg_analyze roles, but that quickly became > complicated because the VACUUM and ANALYZE code is intertwined. To > initiate the discussion, here's the simplest thing I could think of. And here's the same patch, but with docs that actually build. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Sat, Jul 23, 2022 at 2:07 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > Hi hackers, > > The previous attempt to add a predefined role for VACUUM and ANALYZE [0] > resulted in the new pg_checkpoint role in v15. I'd like to try again to > add a new role (or multiple new roles) for VACUUM and ANALYZE. > > The primary motivation for this is to continue chipping away at things that > require special privileges or even superuser. VACUUM and ANALYZE typically > require table ownership, database ownership, or superuser. And only > superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these > operations would allow delegating such tasks (e.g., a nightly VACUUM > scheduled with pg_cron) to a role with fewer privileges. Thanks. I'm personally happy with more granular levels of control (as we don't have to give full superuser access to just run a few commands or maintenance operations) for various postgres commands. The only concern is that we might eventually end up with many predefined roles (perhaps one predefined role per command), spreading all around the code base and it might be difficult for the users to digest all of the roles in. It will be great if we can have some sort of rules or methods to define a separate role for a command. > The attached patch adds a pg_vacuum_analyze role that allows VACUUM and > ANALYZE commands on all relations. I started by trying to introduce > separate pg_vacuum and pg_analyze roles, but that quickly became > complicated because the VACUUM and ANALYZE code is intertwined. To > initiate the discussion, here's the simplest thing I could think of. pg_vacuum_analyze, immediately, makes me think if we need to have a predefined role for CLUSTER command and maybe for other commands as well such as EXECUTE, CALL, ALTER SYSTEM SET, LOAD, COPY and so on. > An alternate approach might be to allow using GRANT to manage these > privileges, as suggested in the previous thread [1]. > > Thoughts? > > [0] https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com > [1] https://postgr.es/m/20211104224636.5qg6cfyjkw52rh4d@alap3.anarazel.de I think GRANT approach [1] is worth considering or at least discussing its pros and cons might give us a better idea as to why we need separate predefined roles. Regards, Bharath Rupireddy.
On Mon, Jul 25, 2022 at 12:58:36PM +0530, Bharath Rupireddy wrote: > Thanks. I'm personally happy with more granular levels of control (as > we don't have to give full superuser access to just run a few commands > or maintenance operations) for various postgres commands. The only > concern is that we might eventually end up with many predefined roles > (perhaps one predefined role per command), spreading all around the > code base and it might be difficult for the users to digest all of the > roles in. It will be great if we can have some sort of rules or > methods to define a separate role for a command. Yeah, in the future, I could see this growing to a couple dozen predefined roles. Given they are relatively inexpensive and there are already 12 of them, I'm personally not too worried about the list becoming too unwieldy. Another way to help users might be to create additional aggregate predefined roles (like pg_monitor) for common combinations. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
At Mon, 25 Jul 2022 09:40:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> On Mon, Jul 25, 2022 at 12:58:36PM +0530, Bharath Rupireddy wrote:
> > Thanks. I'm personally happy with more granular levels of control (as
> > we don't have to give full superuser access to just run a few commands
> > or maintenance operations) for various postgres commands. The only
> > concern is that we might eventually end up with many predefined roles
> > (perhaps one predefined role per command), spreading all around the
> > code base and it might be difficult for the users to digest all of the
> > roles in. It will be great if we can have some sort of rules or
> > methods to define a separate role for a command.
>
> Yeah, in the future, I could see this growing to a couple dozen predefined
> roles. Given they are relatively inexpensive and there are already 12 of
> them, I'm personally not too worried about the list becoming too unwieldy.
> Another way to help users might be to create additional aggregate
> predefined roles (like pg_monitor) for common combinations.
I agree to the necessity of that execution control, but I fear a
little how many similar roles will come in future (but it doesn't seem
so much?). I didn't think so when pg_checkpoint was introdueced,
though. That being said, since we're going to control
maintenance'ish-command execution via predefined roles so it is fine
in that criteria.
One arguable point would be whether we will need to put restriction
the target relations that Bob can vacuum/analyze. If we need that, the
new predeefined role is not sufficient then need a new syntax for that.
GRANT EXECUTION COMMAND VACUUM ON TABLE rel1 TO bob.
GRANT EXECUTION COMMAND VACUUM ON TABLES OWNED BY alice TO bob.
GRANT EXECUTION COMMAND VACUUM ON ALL TABLES OWNED BY alice TO bob.
However, one problem of these syntaxes is they cannot do something to
future relations.
So, considering that aspect, I would finally agree to the proposal
here. (In short +1 for what the patch does.)
About the patch, it seems fine as the whole except the change in error
messages.
- (errmsg("skipping \"%s\" --- only superuser can analyze it",
+ (errmsg("skipping \"%s\" --- only superusers and roles with "
+ "privileges of pg_vacuum_analyze can analyze it",
The message looks a bit too verbose or lengty especially when many
relations are rejected.
WARNING: skipping "pg_statistic" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database
ownercan vacuum it
WARNING: skipping "pg_type" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner can
vacuumit
<snip many lines>
WARNING: skipping "user_mappings" --- only table or database owner can vacuum it
VACUUM
Couldn't we simplify the message? For example "skipping \"%s\" ---
insufficient priviledges". We could add a detailed (not a DETAIL:)
message at the end to cover all of the skipped relations, but it may
be too much.
By the way the patch splits an error message into several parts but
that later makes it harder to search for the message in the tree. *I*
would suggest not splitting message strings.
# I refrain from suggesing removing parens surrounding errmsg() :p
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Tue, 26 Jul 2022 10:47:12 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > WARNING: skipping "pg_statistic" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database ownercan vacuum it > WARNING: skipping "pg_type" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner canvacuum it > <snip many lines> > WARNING: skipping "user_mappings" --- only table or database owner can vacuum it By the way, the last error above dissapears by granting pg_vacuum_analyze to the role. Is there a reason the message is left alone? And If I specified the view directly, I would get the following message. postgres=> vacuum information_schema.user_mappings; WARNING: skipping "user_mappings" --- cannot vacuum non-tables or special system tables So, "VACUUM;" does something wrong? Or is it the designed behavior? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, Jul 25, 2022 at 9:47 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > One arguable point would be whether we will need to put restriction > the target relations that Bob can vacuum/analyze. Yeah. pg_checkpoint makes sense because you can either CHECKPOINT or you can't. But for a command with a target, you really ought to have a permission on the object, not just a general permission. On the other hand, we do have things like pg_read_all_tables, so we could have pg_vacuum_all_tables too. Still, it seems somewhat appealing to give people fine-grained control over this, rather than just "on" or "off". -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, Jul 26, 2022 at 10:37 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jul 25, 2022 at 9:47 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> One arguable point would be whether we will need to put restriction
> the target relations that Bob can vacuum/analyze.
But for a command with a target, you really ought to have a
permission on the object, not just a general permission. On the other
hand, we do have things like pg_read_all_tables, so we could have
pg_vacuum_all_tables too.
I'm still more likely to create a specific security definer function owned by the relevant table owner to give out ANALYZE (and maybe VACUUM) permission to ETL-performing roles.
Still, it seems somewhat appealing to give
people fine-grained control over this, rather than just "on" or "off".
Appealing enough to consume a couple of permission bits?
David J.
On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston <david.g.johnston@gmail.com> wrote: >> Still, it seems somewhat appealing to give >> people fine-grained control over this, rather than just "on" or "off". > Appealing enough to consume a couple of permission bits? > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com I think we're down to 0 remaining now, so it'd be hard to justify consuming 2 of 0 remaining bits. However, I maintain that the solution to this is either (1) change the aclitem representation to get another 32 bits or (2) invent a different system for less-commonly used permission bits. Checking permissions for SELECT or UPDATE has to be really fast, because most queries will need to do that sort of thing. If we represented VACUUM or ANALYZE in some other way in the catalogs that was more scalable but less efficient, it wouldn't be a big deal (although there's the issue of code duplication to consider). -- Robert Haas EDB: http://www.enterprisedb.com
At Tue, 26 Jul 2022 13:54:38 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston > <david.g.johnston@gmail.com> wrote: > >> Still, it seems somewhat appealing to give > >> people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? > > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com > > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). I guess that we can use the last bit for ACL_SLOW_PATH or something like. Furthermore we could move some existing ACL modeds to that slow path to vacate some fast-ACL bits. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Jul 26, 2022 at 01:54:38PM -0400, Robert Haas wrote: > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. AFAICT there are 2 remaining. N_ACL_RIGHTS is only 14. > However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). Perhaps we could add something like a relacl_ext column to affected catalogs with many more than 32 privilege bits. However, if we actually do have 2 bits remaining, we wouldn't need to do that work now unless someone else uses them first. That being said, it's certainly worth thinking about the future of this stuff. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Greetings, * Robert Haas (robertmhaas@gmail.com) wrote: > On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston > <david.g.johnston@gmail.com> wrote: > >> Still, it seems somewhat appealing to give > >> people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? > > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com > > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). I've long felt that we should redefine the way the ACLs work to have a distinct set of bits for each object type. We don't need to support a CONNECT bit on a table, yet we do today and we expend quite a few bits in that way. Having that handled on a per-object-type basis instead would allow us to get quite a bit more mileage out of the existing 32bit field before having to introduce more complicated storage methods like using a bit to tell us to go look up more ACLs somewhere else. Thanks, Stephen
Attachment
Here is a first attempt at allowing users to grant VACUUM or ANALYZE per-relation. Overall, this seems pretty straightforward. I needed to adjust the permissions logic for VACUUM/ANALYZE a bit, which causes some extra WARNING messages for VACUUM (ANALYZE) in some cases, but this didn't seem particularly worrisome. It may be desirable to allow granting ANALYZE on specific columns or to allow granting VACUUM/ANALYZE at the schema or database level, but that is left as a future exercise. On Tue, Aug 23, 2022 at 07:46:47PM -0400, Stephen Frost wrote: > I've long felt that we should redefine the way the ACLs work to have a > distinct set of bits for each object type. We don't need to support a > CONNECT bit on a table, yet we do today and we expend quite a few bits > in that way. Having that handled on a per-object-type basis instead > would allow us to get quite a bit more mileage out of the existing 32bit > field before having to introduce more complicated storage methods like > using a bit to tell us to go look up more ACLs somewhere else. There are 2 bits remaining at the moment, so I didn't redesign the ACL system in the attached patch. However, I did some research on a couple options. Using a distinct set of bits for each catalog table should free up a handful of bits, which should indeed kick the can down the road a little. Another easy option is to simply make AclMode a uint64, which would immediately free up another 16 privilege bits. I was able to get this approach building and passing tests in a few minutes, but there might be performance/space concerns. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Sep 5, 2022 at 2:56 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > There are 2 bits remaining at the moment, so I didn't redesign the ACL > system in the attached patch. However, I did some research on a couple > options. Using a distinct set of bits for each catalog table should free > up a handful of bits, which should indeed kick the can down the road a > little. Another easy option is to simply make AclMode a uint64, which > would immediately free up another 16 privilege bits. I was able to get > this approach building and passing tests in a few minutes, but there might > be performance/space concerns. I believe Tom has expressed such concerns in the past, but it is not clear to me that they are well-founded. I don't think we have much code that manipulates large numbers of aclitems, so I can't quite see where the larger size would be an issue. There may well be some places, so I'm not saying that Tom or anyone else with concerns is wrong, but I'm just having a hard time thinking of where it would be a real issue. -- Robert Haas EDB: http://www.enterprisedb.com
Greetings, * Nathan Bossart (nathandbossart@gmail.com) wrote: > On Tue, Aug 23, 2022 at 07:46:47PM -0400, Stephen Frost wrote: > > I've long felt that we should redefine the way the ACLs work to have a > > distinct set of bits for each object type. We don't need to support a > > CONNECT bit on a table, yet we do today and we expend quite a few bits > > in that way. Having that handled on a per-object-type basis instead > > would allow us to get quite a bit more mileage out of the existing 32bit > > field before having to introduce more complicated storage methods like > > using a bit to tell us to go look up more ACLs somewhere else. > > There are 2 bits remaining at the moment, so I didn't redesign the ACL > system in the attached patch. However, I did some research on a couple > options. Using a distinct set of bits for each catalog table should free > up a handful of bits, which should indeed kick the can down the road a > little. Another easy option is to simply make AclMode a uint64, which > would immediately free up another 16 privilege bits. I was able to get > this approach building and passing tests in a few minutes, but there might > be performance/space concerns. Considering our burn rate of ACL bits is really rather slow (2 this year, but prior to that was TRUNCATE in 2008 and CONNECT in 2006), I'd argue that moving away from the current one-size-fits-all situation would kick the can down the road more than just 'a little' and wouldn't have any performance or space concerns. Once we actually get to the point where we've burned through all of those after the next few decades then we can move to a uint64 or something else more complicated, perhaps. If we were to make the specific bits depend on the object type as I'm suggesting, then we'd have 8 bits used for relations (10 with the vacuum and analyze bits), leaving us with 6 remaining inside the existing uint32, or more bits available than we've ever used since the original implementation from what I can tell, or at least 15+ years. That seems like pretty darn good future-proofing without a lot of complication or any change in physical size. We would also be able to get rid of the question of "well, is it more valuable to add the ability to GRANT TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather odd debates between ultimately very different things. Thanks, Stephen
Attachment
On Tue, Sep 6, 2022 at 11:11 AM Stephen Frost <sfrost@snowman.net> wrote: > Considering our burn rate of ACL bits is really rather slow (2 this > year, but prior to that was TRUNCATE in 2008 and CONNECT in 2006), I'd > argue that moving away from the current one-size-fits-all situation > would kick the can down the road more than just 'a little' and wouldn't > have any performance or space concerns. Once we actually get to the > point where we've burned through all of those after the next few decades > then we can move to a uint64 or something else more complicated, > perhaps. Our burn rate is slow because there's been a lot of pushback - mostly from Tom - about consuming the remaining bits. It's not because people haven't had ideas about how to use them up. > If we were to make the specific bits depend on the object type as I'm > suggesting, then we'd have 8 bits used for relations (10 with the vacuum > and analyze bits), leaving us with 6 remaining inside the existing > uint32, or more bits available than we've ever used since the original > implementation from what I can tell, or at least 15+ years. That seems > like pretty darn good future-proofing without a lot of complication or > any change in physical size. We would also be able to get rid of the > question of "well, is it more valuable to add the ability to GRANT > TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather > odd debates between ultimately very different things. I mostly agree with this. I don't think it's entirely clear how we should try to get more bits going forward, but it's clear that we cannot just forever hold our breath and refuse to find any more bits. And of the possible ways of doing it, this seems like the one with the lowest impact, so I think it likely makes sense to do this one first. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, Sep 06, 2022 at 11:24:18AM -0400, Robert Haas wrote: > On Tue, Sep 6, 2022 at 11:11 AM Stephen Frost <sfrost@snowman.net> wrote: >> If we were to make the specific bits depend on the object type as I'm >> suggesting, then we'd have 8 bits used for relations (10 with the vacuum >> and analyze bits), leaving us with 6 remaining inside the existing >> uint32, or more bits available than we've ever used since the original >> implementation from what I can tell, or at least 15+ years. That seems >> like pretty darn good future-proofing without a lot of complication or >> any change in physical size. We would also be able to get rid of the >> question of "well, is it more valuable to add the ability to GRANT >> TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather >> odd debates between ultimately very different things. > > I mostly agree with this. I don't think it's entirely clear how we > should try to get more bits going forward, but it's clear that we > cannot just forever hold our breath and refuse to find any more bits. > And of the possible ways of doing it, this seems like the one with the > lowest impact, so I think it likely makes sense to do this one first. +1. My earlier note wasn't intended to suggest that one approach was better than the other, merely that there are a couple of options to choose from once we run out of bits. I don't think this work needs to be tied to the VACUUM/ANALYZE stuff, but I am interested in it and hope to take it on at some point. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Mon, Sep 05, 2022 at 11:56:30AM -0700, Nathan Bossart wrote: > Here is a first attempt at allowing users to grant VACUUM or ANALYZE > per-relation. Overall, this seems pretty straightforward. I needed to > adjust the permissions logic for VACUUM/ANALYZE a bit, which causes some > extra WARNING messages for VACUUM (ANALYZE) in some cases, but this didn't > seem particularly worrisome. It may be desirable to allow granting ANALYZE > on specific columns or to allow granting VACUUM/ANALYZE at the schema or > database level, but that is left as a future exercise. Here is a new patch set with some follow-up patches to implement $SUBJECT. 0001 is the same as v3. 0002 simplifies some WARNING messages as suggested upthread [0]. 0003 adds the new pg_vacuum_all_tables and pg_analyze_all_tables predefined roles. Instead of adjusting the permissions logic in vacuum.c, I modified pg_class_aclmask_ext() to return the ACL_VACUUM and/or ACL_ANALYZE bits as appropriate. [0] https://postgr.es/m/20220726.104712.912995710251150228.horikyota.ntt%40gmail.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
Greetings, * Nathan Bossart (nathandbossart@gmail.com) wrote: > On Tue, Sep 06, 2022 at 11:24:18AM -0400, Robert Haas wrote: > > On Tue, Sep 6, 2022 at 11:11 AM Stephen Frost <sfrost@snowman.net> wrote: > >> If we were to make the specific bits depend on the object type as I'm > >> suggesting, then we'd have 8 bits used for relations (10 with the vacuum > >> and analyze bits), leaving us with 6 remaining inside the existing > >> uint32, or more bits available than we've ever used since the original > >> implementation from what I can tell, or at least 15+ years. That seems > >> like pretty darn good future-proofing without a lot of complication or > >> any change in physical size. We would also be able to get rid of the > >> question of "well, is it more valuable to add the ability to GRANT > >> TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather > >> odd debates between ultimately very different things. > > > > I mostly agree with this. I don't think it's entirely clear how we > > should try to get more bits going forward, but it's clear that we > > cannot just forever hold our breath and refuse to find any more bits. > > And of the possible ways of doing it, this seems like the one with the > > lowest impact, so I think it likely makes sense to do this one first. > > +1. My earlier note wasn't intended to suggest that one approach was > better than the other, merely that there are a couple of options to choose > from once we run out of bits. I don't think this work needs to be tied to > the VACUUM/ANALYZE stuff, but I am interested in it and hope to take it on > at some point. I disagree that we should put the onus for addressing this on the next person who wants to add bits and just willfully use up the last of them right now for what strikes me, at least, as a relatively marginal use case. If we had plenty of bits then, sure, let's use a couple of for this, but that isn't currently the case. If you want this feature then the onus is on you to do the legwork to make it such that we have plenty of bits. My 2c anyway. Thanks, Stephen
Attachment
> On Jul 22, 2022, at 1:37 PM, Nathan Bossart <nathandbossart@gmail.com> wrote: > > The primary motivation for this is to continue chipping away at things that > require special privileges or even superuser. VACUUM and ANALYZE typically > require table ownership, database ownership, or superuser. And only > superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these > operations would allow delegating such tasks (e.g., a nightly VACUUM > scheduled with pg_cron) to a role with fewer privileges. > > The attached patch adds a pg_vacuum_analyze role that allows VACUUM and > ANALYZE commands on all relations. Granting membership in a role that can VACUUM and ANALYZE any relation seems to grant a subset of a more general category,the ability to perform modifying administrative operations on a relation without necessarily being able to reador modify the logical contents of that relation. That more general category would seem to also include CLUSTER, REINDEX,REFRESH MATERIALIZED VIEW and more broadly ALTER SUBSCRIPTION ... REFRESH PUBLICATION and ALTER DATABASE ... REFRESHCOLLATION VERSION. These latter operations may be less critical to database maintenance than is VACUUM, but arguablyANALYZE isn't as critical as is VACUUM, either. Assuming for the sake of argument that we should create a role something like you propose, can you explain why we shoulddraw the line around just VACUUM and ANALYZE? I am not arguing for including these other commands, but don't wantto regret having drawn the line in the wrong place when later we decide to add more roles like the one you are proposing. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Sep 07, 2022 at 05:13:44PM -0400, Stephen Frost wrote: > I disagree that we should put the onus for addressing this on the next > person who wants to add bits and just willfully use up the last of them > right now for what strikes me, at least, as a relatively marginal use > case. If we had plenty of bits then, sure, let's use a couple of for > this, but that isn't currently the case. If you want this feature then > the onus is on you to do the legwork to make it such that we have plenty > of bits. FWIW what I really want is the new predefined roles. I received feedback upthread that it might also make sense to give people more fine-grained control, so I implemented that. And now you're telling me that I need to redesign the ACL system. :) I'm happy to give that project a try given there is agreement on the direction and general interest in the patches. From the previous discussion, it sounds like we want to first use a distinct set of bits for each catalog table. Is that what I should proceed with? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Sep 07, 2022 at 02:53:57PM -0700, Mark Dilger wrote: > Assuming for the sake of argument that we should create a role something like you propose, can you explain why we shoulddraw the line around just VACUUM and ANALYZE? I am not arguing for including these other commands, but don't wantto regret having drawn the line in the wrong place when later we decide to add more roles like the one you are proposing. There was some previous discussion around adding a pg_maintenance role that could perform all of these commands [0]. I didn't intend to draw a line around VACUUM and ANALYZE. Those are just the commands I started with. If/when there are many of these roles, it might make sense to create a pg_maintenance role that is a member of pg_vacuum_all_tables, pg_analyze_all_tables, etc. [0] https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
> On Sep 7, 2022, at 3:21 PM, Nathan Bossart <nathandbossart@gmail.com> wrote: > > There was some previous discussion around adding a pg_maintenance role that > could perform all of these commands [0]. I didn't intend to draw a line > around VACUUM and ANALYZE. Those are just the commands I started with. > If/when there are many of these roles, it might make sense to create a > pg_maintenance role that is a member of pg_vacuum_all_tables, > pg_analyze_all_tables, etc. Thank you, that sounds fair enough. It seems you've been pushed to make the patch-set more complicated, and now we're debating privilege bits, which seems prettyfar off topic. I may be preaching to the choir here, but wouldn't it work to commit new roles pg_vacuum_all_tables and pg_analyze_all_tableswith checks like you had in the original patch of this thread? That wouldn't block the later additionof finer grained controls allowing users to grant VACUUM or ANALYZE per-relation, would it? Something like whatStephen is requesting, and what you did with new privilege bits for VACUUM and ANALYZE could still be added, unless I'mmissing something. I'd hate to see your patch set get further delayed by things that aren't logically prerequisites. The conversation upthreadwas useful to determine that they aren't prerequisites, but if anybody wants to explain to me why they are.... — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Greetings,
On Wed, Sep 7, 2022 at 18:11 Nathan Bossart <nathandbossart@gmail.com> wrote:
On Wed, Sep 07, 2022 at 05:13:44PM -0400, Stephen Frost wrote:
> I disagree that we should put the onus for addressing this on the next
> person who wants to add bits and just willfully use up the last of them
> right now for what strikes me, at least, as a relatively marginal use
> case. If we had plenty of bits then, sure, let's use a couple of for
> this, but that isn't currently the case. If you want this feature then
> the onus is on you to do the legwork to make it such that we have plenty
> of bits.
FWIW what I really want is the new predefined roles. I received feedback
upthread that it might also make sense to give people more fine-grained
control, so I implemented that. And now you're telling me that I need to
redesign the ACL system. :)
Calling this a redesign is over-stating things, imv … and I’d much rather have the per-relation granularity than predefined roles for this, so there is that to consider too, perhaps.
I'm happy to give that project a try given there is agreement on the
direction and general interest in the patches. From the previous
discussion, it sounds like we want to first use a distinct set of bits for
each catalog table. Is that what I should proceed with?
Yes, that seems to be the consensus among those involved in this thread thus far. Basically, I imagine this involves passing around the object type along with the acl info and then using that to check the bits and such. I doubt it’s worth inventing a new structure to combine the two … but that’s just gut feeling and you may find it does make sense to once you get into it.
Thanks!
Stephen
> On Sep 7, 2022, at 4:09 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Calling this a redesign is over-stating things, imv … and I’d much rather have the per-relation granularity than predefinedroles for this, so there is that to consider too, perhaps. Ok, now I'm a bit lost. If I want to use Nathan's feature to create a role to vacuum and analyze my database on a regularbasis, how does per-relation granularity help me? If somebody creates a new table and doesn't grant those privilegesto the role, doesn't that break the usage case? To me, per-relation granularity sounds useful, but orthogonal,to this feature. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Sep 07, 2022 at 07:09:05PM -0400, Stephen Frost wrote: > Yes, that seems to be the consensus among those involved in this thread > thus far. Basically, I imagine this involves passing around the object > type along with the acl info and then using that to check the bits and > such. I doubt it’s worth inventing a new structure to combine the two … > but that’s just gut feeling and you may find it does make sense to once you > get into it. I've done some preliminary research for this approach, and I've found some interesting challenges. * aclparse() will need to handle ambiguous strings. For example, USAGE is available for most catalogs, so which ACL bit should be chosen? One possible solution would be to make sure the common privilege types always use the same bit. * When comparing ACLs, there probably should be some way to differentiate overloaded privilege bits, else ACLs for different catalogs that have nothing in common could evaluate as equal. Such comparisons may be unlikely, but this still doesn't strike me as acceptable. * aclitemout() needs some way to determine what privilege an ACL bit actually refers to. I can think of a couple of ways to do this: 1) we could create different aclitem types for each catalog (or maybe just one for pg_class and another for everything else), or 2) we could include the type in AclItem, perhaps by adding a uint8 field. I noticed that Tom called out this particular challenge back in 2018 [0]. Am I overlooking an easier way to handle these things? From my admittedly brief analysis thus far, I'm worried this could devolve into something overly complex or magical, especially when simply moving to a uint64 might be a reasonable way to significantly extend AclItem's life span. Robert suggested upthread that Tom might have concerns with adding another 32 bits to AclItem, but the archives indicate he has previously proposed exactly that [1]. Of course, I don't know how everyone feels about the uint64 idea today, but ISTM like it might be the path of least resistance. So, here is a new patch set. 0001 expands AclMode to a uint64. 0002 simplifies some WARNING messages for VACUUM/ANALYZE. 0003 introduces privilege bits for VACUUM and ANALYZE on relations. And 0004 introduces the pg_vacuum/analyze_all_tables predefined roles. [0] https://postgr.es/m/18391.1521419120%40sss.pgh.pa.us [1] https://postgr.es/m/11414.1526422062%40sss.pgh.pa.us -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, Sep 7, 2022 at 7:09 PM Stephen Frost <sfrost@snowman.net> wrote: > Calling this a redesign is over-stating things, imv … and I’d much rather have the per-relation granularity than predefinedroles for this, so there is that to consider too, perhaps. I also prefer the finer granularity. On the question of whether freeing up more privilege bits is a prerequisite for this patch, I'm a bit on the fence about that. If I look at the amount of extra work that your review comments have caused me to do over, let's say, the last three years, and I compare that to the amount of extra work that the review comments of other people have caused me to do in the same period of time, you win. In fact, you win against all of them added together and doubled. I think that as a general matter you are far too willing to argue vigorously for people to do work that isn't closely related to their original goals, and which is at times even opposed to their original goals, and I think the project would be better off if you tempered that urge. Now on the other hand, I also do think we need more privilege bits. You're not alone in making the case that this is a problem which needs to be solved, and the set of other people who are also making that argument includes me. At the same time, there is certainly a double standard here. When Andrew and Tom committed d11e84ea466b4e3855d7bd5142fb68f51c273567 and a0ffa885e478f5eeacc4e250e35ce25a4740c487 respectively, we used up 2 of the remaining 4 bits, bits which other people would have liked to have used up years ago and they were told "no you can't." I don't believe I would have been willing to commit those patches without doing something to solve this problem, because I would have been worried about getting yelled at by Tom. But now here we are with only 2 bits left instead of 4, and we're telling the next patch author - who is not Tom - that he's on the hook to solve the problem. Well, we do need to solve the problem. But we're not necessarily being fair about how the work involved gets distributed. It's a heck of a lot easier for a committer to get something committed to address this issue than a non-committer, and it's a heck of a lot easier for a committer to ignore the fact that the problem hasn't been solved and press ahead anyway, and yet somehow we're trying to dump a problem that's a decade in the making on Nathan. I'm not exactly sure what to propose as an alternative, but that doesn't seem quite fair. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Sep 07, 2022 at 04:15:23PM -0700, Mark Dilger wrote: > Ok, now I'm a bit lost. If I want to use Nathan's feature to create a role to vacuum and analyze my database on a regularbasis, how does per-relation granularity help me? If somebody creates a new table and doesn't grant those privilegesto the role, doesn't that break the usage case? To me, per-relation granularity sounds useful, but orthogonal,to this feature. I think there is room for both per-relation privileges and new predefined roles. My latest patch set [0] introduces both. [0] https://postgr.es/m/20220908055035.GA2100193%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, Sep 08, 2022 at 09:41:20AM -0400, Robert Haas wrote: > Now on the other hand, I also do think we need more privilege bits. > You're not alone in making the case that this is a problem which needs > to be solved, and the set of other people who are also making that > argument includes me. At the same time, there is certainly a double > standard here. When Andrew and Tom committed > d11e84ea466b4e3855d7bd5142fb68f51c273567 and > a0ffa885e478f5eeacc4e250e35ce25a4740c487 respectively, we used up 2 of > the remaining 4 bits, bits which other people would have liked to have > used up years ago and they were told "no you can't." I don't believe I > would have been willing to commit those patches without doing > something to solve this problem, because I would have been worried > about getting yelled at by Tom. But now here we are with only 2 bits > left instead of 4, and we're telling the next patch author - who is > not Tom - that he's on the hook to solve the problem. > > Well, we do need to solve the problem. But we're not necessarily being > fair about how the work involved gets distributed. It's a heck of a > lot easier for a committer to get something committed to address this > issue than a non-committer, and it's a heck of a lot easier for a > committer to ignore the fact that the problem hasn't been solved and > press ahead anyway, and yet somehow we're trying to dump a problem > that's a decade in the making on Nathan. I'm not exactly sure what to > propose as an alternative, but that doesn't seem quite fair. Are there any concerns with simply expanding AclMode to 64 bits, as done in v5 [0]? [0] https://postgr.es/m/20220908055035.GA2100193%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Mon, Sep 19, 2022 at 08:51:47PM -0700, Nathan Bossart wrote: > Are there any concerns with simply expanding AclMode to 64 bits, as done in > v5 [0]? > > [0] https://postgr.es/m/20220908055035.GA2100193%40nathanxps13 I have gone through the thread, and I'd agree with getting more granularity when it comes to assigning ACLs to relations rather than just an on/off switch for the objects of a given type would be nice. I've been looking at the whole use of AclMode and AclItem in the code, and I don't quite see why a larger size could have a noticeable impact. There are a few things that could handle a large number of AclItems, though, say for array operations like aclupdate(). These could be easily checked with some micro-benchmarking or some SQL queries that emulate a large number of items in aclitem[] arrays. Any impact for the column sizes of the catalogs holding ACL information? Just asking while browsing the patch set. Some comments in utils/acl.h need a refresh as the number of lower and upper bits looked at from ai_privs changes. -- Michael
Attachment
On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: > I have gone through the thread, and I'd agree with getting more > granularity when it comes to assigning ACLs to relations rather than > just an on/off switch for the objects of a given type would be nice. > I've been looking at the whole use of AclMode and AclItem in the code, > and I don't quite see why a larger size could have a noticeable > impact. There are a few things that could handle a large number of > AclItems, though, say for array operations like aclupdate(). These > could be easily checked with some micro-benchmarking or some SQL > queries that emulate a large number of items in aclitem[] arrays. I performed a few quick tests with a couple thousand ACLs on my laptop, and I'm consistently seeing a 4.3% regression. > Any impact for the column sizes of the catalogs holding ACL > information? Just asking while browsing the patch set. Since each aclitem requires 16 bytes instead of 12, I assume so. However, in my testing, I hit a "row is too big" error with the same number of aclitems in a pg_class row before and after the change. I might be missing something in my patch, or maybe I am misunderstanding how arrays of aclitems are stored on disk. > Some comments in utils/acl.h need a refresh as the number of lower and > upper bits looked at from ai_privs changes. Oops, I missed that one. I fixed it in the attached patch set. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Sep 20, 2022 at 11:05:33AM -0700, Nathan Bossart wrote: > On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: >> Any impact for the column sizes of the catalogs holding ACL >> information? Just asking while browsing the patch set. > > Since each aclitem requires 16 bytes instead of 12, I assume so. However, > in my testing, I hit a "row is too big" error with the same number of > aclitems in a pg_class row before and after the change. I might be missing > something in my patch, or maybe I am misunderstanding how arrays of > aclitems are stored on disk. Ah, it looks like relacl is compressed. The column is marked "extended," but pg_class doesn't appear to have a TOAST table, so presumably no out-of-line storage can be used. I found a couple of threads about this [0] [1] [2]. [0] https://postgr.es/m/17245.964897719%40sss.pgh.pa.us [1] https://postgr.es/m/200309040531.h845ViP05881%40candle.pha.pa.us [2] https://postgr.es/m/29061.1265327626%40sss.pgh.pa.us -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Sep 20, 2022 at 04:31:17PM -0700, Nathan Bossart wrote: > On Tue, Sep 20, 2022 at 11:05:33AM -0700, Nathan Bossart wrote: >> On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: >>> Any impact for the column sizes of the catalogs holding ACL >>> information? Just asking while browsing the patch set. >> >> Since each aclitem requires 16 bytes instead of 12, I assume so. However, >> in my testing, I hit a "row is too big" error with the same number of >> aclitems in a pg_class row before and after the change. I might be missing >> something in my patch, or maybe I am misunderstanding how arrays of >> aclitems are stored on disk. > > Ah, it looks like relacl is compressed. The column is marked "extended," > but pg_class doesn't appear to have a TOAST table, so presumably no > out-of-line storage can be used. I found a couple of threads about this > [0] [1] [2]. I suppose there is some risk that folks with really long aclitem arrays might be unable to pg_upgrade to a version with uint64 AclModes, but I suspect that risk is limited to extreme cases (i.e., multiple thousands of aclitems). I'm not sure whether that's worth worrying about too much. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Sep 20, 2022 at 04:50:10PM -0700, Nathan Bossart wrote: > On Tue, Sep 20, 2022 at 04:31:17PM -0700, Nathan Bossart wrote: >> On Tue, Sep 20, 2022 at 11:05:33AM -0700, Nathan Bossart wrote: >>> On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: >>>> Any impact for the column sizes of the catalogs holding ACL >>>> information? Just asking while browsing the patch set. >>> >>> Since each aclitem requires 16 bytes instead of 12, I assume so. However, >>> in my testing, I hit a "row is too big" error with the same number of >>> aclitems in a pg_class row before and after the change. I might be missing >>> something in my patch, or maybe I am misunderstanding how arrays of >>> aclitems are stored on disk. >> >> Ah, it looks like relacl is compressed. The column is marked "extended," >> but pg_class doesn't appear to have a TOAST table, so presumably no >> out-of-line storage can be used. I found a couple of threads about this >> [0] [1] [2]. Adding a toast table to pg_class has been a sensitive topic over the years. Based on my recollection of the events, there were worries about the potential cross-dependencies with pg_class and pg_attribute that this would create. > I suppose there is some risk that folks with really long aclitem arrays > might be unable to pg_upgrade to a version with uint64 AclModes, but I > suspect that risk is limited to extreme cases (i.e., multiple thousands of > aclitems). I'm not sure whether that's worth worrying about too much. Did you just run an aclupdate()? 4% for aclitem[] sounds like quite a number to me :/ It may be worth looking at if these operations could be locally optimized more, as well. I'd like to think that we could live with that to free up enough bits in AclItems for the next 20 years, anyway. Any opinions? For the column sizes of the catalogs, I was wondering about how pg_column_size() changes when they hold ACL information. Unoptimized alignment could cause an unnecessary increase in the structure sizes, so the addition of new fields or changes in object size could have unexpected side effects. -- Michael
Attachment
On Wed, Sep 21, 2022 at 10:31:47AM +0900, Michael Paquier wrote: > Did you just run an aclupdate()? 4% for aclitem[] sounds like quite a > number to me :/ It may be worth looking at if these operations could > be locally optimized more, as well. I'd like to think that we could > live with that to free up enough bits in AclItems for the next 20 > years, anyway. Any opinions? Yes, the test was mostly for aclupdate(). Looking at that function, I bet most of its time is spent in palloc0() and memcpy(). It might be possible to replace the linear search if the array was sorted, but I'm skeptical that will help much. In the end, I'm not it's worth worrying too much about 2,000 calls to aclupdate() with an array of 2,000 ACLs taking 5.3 seconds instead of 5.1 seconds. I bet a more pressing concern is the calls to aclmask() since checking privileges is probably done more frequently than updating them. That appears to use a linear search, too, so maybe sorting the aclitem arrays is actually worth exploring. I still doubt there will be much noticeable impact from expanding AclMode outside of the most extreme cases. > For the column sizes of the catalogs, I was wondering about how > pg_column_size() changes when they hold ACL information. Unoptimized > alignment could cause an unnecessary increase in the structure sizes, > so the addition of new fields or changes in object size could have > unexpected side effects. After a few tests, I haven't discovered any changes to the output of pg_column_size(). -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Sep 20, 2022 at 09:31:26PM -0700, Nathan Bossart wrote: > I bet a more pressing concern is the calls to aclmask() since checking > privileges is probably done more frequently than updating them. That > appears to use a linear search, too, so maybe sorting the aclitem arrays is > actually worth exploring. I still doubt there will be much noticeable > impact from expanding AclMode outside of the most extreme cases. I've been testing aclmask() with long aclitem arrays (2,000 entries is close to the limit for pg_class entries), and I haven't found any significant impact from bumping AclMode to 64 bits. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Greetings,
On Wed, Sep 28, 2022 at 14:50 Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Sep 20, 2022 at 09:31:26PM -0700, Nathan Bossart wrote:
> I bet a more pressing concern is the calls to aclmask() since checking
> privileges is probably done more frequently than updating them. That
> appears to use a linear search, too, so maybe sorting the aclitem arrays is
> actually worth exploring. I still doubt there will be much noticeable
> impact from expanding AclMode outside of the most extreme cases.
I've been testing aclmask() with long aclitem arrays (2,000 entries is
close to the limit for pg_class entries), and I haven't found any
significant impact from bumping AclMode to 64 bits.
The max is the same regardless of the size..? Considering the size is capped since pg_class doesn’t (and isn’t likely to..) have a toast table, that seems unlikely, so I’m asking for clarification on that. We may be able to get consensus that the difference isn’t material since no one is likely to have such long lists, but we should at least be aware.
Thanks,
Stephen
On Wed, Sep 28, 2022 at 03:09:46PM -0400, Stephen Frost wrote: > On Wed, Sep 28, 2022 at 14:50 Nathan Bossart <nathandbossart@gmail.com> > wrote: >> I've been testing aclmask() with long aclitem arrays (2,000 entries is >> close to the limit for pg_class entries), and I haven't found any >> significant impact from bumping AclMode to 64 bits. > > The max is the same regardless of the size..? Considering the size is > capped since pg_class doesn’t (and isn’t likely to..) have a toast table, > that seems unlikely, so I’m asking for clarification on that. We may be > able to get consensus that the difference isn’t material since no one is > likely to have such long lists, but we should at least be aware. While pg_class doesn't have a TOAST table, that column is marked as "extended," so I believe it is still compressed, and the maximum aclitem array length for pg_class.relacl would depend on how well the array compresses. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Sep 28, 2022 at 01:12:22PM -0700, Nathan Bossart wrote: > On Wed, Sep 28, 2022 at 03:09:46PM -0400, Stephen Frost wrote: >> The max is the same regardless of the size..? Considering the size is >> capped since pg_class doesn’t (and isn’t likely to..) have a toast table, >> that seems unlikely, so I’m asking for clarification on that. We may be >> able to get consensus that the difference isn’t material since no one is >> likely to have such long lists, but we should at least be aware. > > While pg_class doesn't have a TOAST table, that column is marked as > "extended," so I believe it is still compressed, and the maximum aclitem > array length for pg_class.relacl would depend on how well the array > compresses. Are there any remaining concerns about this approach? I'm happy to do any testing that folks deem necessary, or anything else really that might help move this patch set forward. If we don't want to extend AclMode right away, we could also keep it in our back pocket for the next time someone (which may very well be me) wants to add privileges. That is, 0001 is not fundamentally a prerequisite for 0002-0004, but I recognize that freeing up some extra bits would be the most courteous. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> Are there any remaining concerns about this approach? I'm happy to do any
> testing that folks deem necessary, or anything else really that might help
> move this patch set forward. If we don't want to extend AclMode right
> away, we could also keep it in our back pocket for the next time someone
> (which may very well be me) wants to add privileges. That is, 0001 is not
> fundamentally a prerequisite for 0002-0004, but I recognize that freeing up
> some extra bits would be the most courteous.
In view of the recent mess around bigint relfilenodes, it seems to me
that we shouldn't move forward with widening AclMode unless somebody
runs down which structs will get wider (or more aligned) and how much
that'll cost us. Maybe it's not a problem, but it could do with an
explicit look at the point.
I do agree with the position that these features are not where to
spend our last remaining privilege bits.
regards, tom lane
On Fri, Sep 30, 2022 at 04:15:24PM -0400, Tom Lane wrote: > In view of the recent mess around bigint relfilenodes, it seems to me > that we shouldn't move forward with widening AclMode unless somebody > runs down which structs will get wider (or more aligned) and how much > that'll cost us. Maybe it's not a problem, but it could do with an > explicit look at the point. The main one I see is AclItem, which increases from 12 bytes to 16 bytes. AFAICT all of the catalogs that store aclitem arrays have the aclitem[] column marked extended, so they are compressed or moved out-of-line as needed, too. The only other structs I've spotted that make use of AclMode are InternalGrant and InternalDefaultACL. I haven't identified anything that leads me to believe there are alignment problems or anything else comparable to the issues listed in the relfilenode thread [0], but I could be missing something. Did you have something else in mind you think ought to be checked? I'm not sure my brief analysis here suffices. [0] https://postgr.es/m/CA%2BTgmoaa9Yc9O-FP4vS_xTKf8Wgy8TzHpjnjN56_ShKE%3DjrP-Q%40mail.gmail.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> The main one I see is AclItem, which increases from 12 bytes to 16 bytes.
... and now requires double alignment ... did you fix its typalign?
We could conceivably dodge the alignment increase by splitting the 64-bit
field into two 32-bit fields, one for base privileges and one for grant
options. That'd be rather invasive, so unless it leads to pleasant
improvements in readability (which it might, perhaps) I wouldn't advocate
for it.
regards, tom lane
On Fri, Sep 30, 2022 at 06:00:53PM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> The main one I see is AclItem, which increases from 12 bytes to 16 bytes. > > ... and now requires double alignment ... did you fix its typalign? Nope, I missed that, thanks for pointing it out. Should we move ai_privs to the beginning of the struct, too? The only other similar example I see is TimeTzADT, but that only consists of an int64 and an int32, while AclItem starts with 2 uint32s. While it might not be strictly necessary, it seems like there is a small chance it could become necessary in the future. > We could conceivably dodge the alignment increase by splitting the 64-bit > field into two 32-bit fields, one for base privileges and one for grant > options. That'd be rather invasive, so unless it leads to pleasant > improvements in readability (which it might, perhaps) I wouldn't advocate > for it. Yeah, the invasiveness is the main reason I haven't tried this yet, but it does seem like it'd improve readability. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Sep 30, 2022 at 06:00:53PM -0400, Tom Lane wrote:
>> ... and now requires double alignment ... did you fix its typalign?
> Nope, I missed that, thanks for pointing it out. Should we move ai_privs
> to the beginning of the struct, too?
Don't see any point, there won't be any padding. If we ever change the
sizeof(Oid), or add more fields, we can consider what to do then.
regards, tom lane
On Fri, Sep 30, 2022 at 07:05:38PM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> On Fri, Sep 30, 2022 at 06:00:53PM -0400, Tom Lane wrote: >>> ... and now requires double alignment ... did you fix its typalign? > >> Nope, I missed that, thanks for pointing it out. Should we move ai_privs >> to the beginning of the struct, too? > > Don't see any point, there won't be any padding. If we ever change the > sizeof(Oid), or add more fields, we can consider what to do then. Sounds good. Here's a new patch set with aclitem's typalign fixed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
Sounds good. Here's a new patch set with aclitem's typalign fixed.
Patch applies.
Passes make check and make check-world.
Test coverage seems adequate.
Passes make check and make check-world.
Test coverage seems adequate.
Coding is very clear and very much in the style of the existing code. Any quibbles I have with the coding style are ones I have with the overall pg-style, and this isn't the forum for that.
I haven't done any benchmarking yet, but it seems that the main question will be the impact on ordinary DML statements.
I have no opinion about the design debate earlier in this thread, but I do think that this patch is ready and adds something concrete to the ongoing discussion.
On Fri, Oct 14, 2022 at 07:37:38PM -0400, Corey Huinker wrote: > Patch applies. > Passes make check and make check-world. > Test coverage seems adequate. > > Coding is very clear and very much in the style of the existing code. Any > quibbles I have with the coding style are ones I have with the overall > pg-style, and this isn't the forum for that. > > I haven't done any benchmarking yet, but it seems that the main question > will be the impact on ordinary DML statements. > > I have no opinion about the design debate earlier in this thread, but I do > think that this patch is ready and adds something concrete to the ongoing > discussion. Thanks for taking a look! Here is a rebased version of the patch set. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Nov 14, 2022 at 03:40:04PM -0800, Nathan Bossart wrote: > Thanks for taking a look! Here is a rebased version of the patch set. Oops, apparently object_aclcheck() cannot be used for pg_class. Here is another version that uses pg_class_aclcheck() instead. I'm not sure how I missed this earlier. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On 2022-11-15 Tu 00:08, Nathan Bossart wrote: > On Mon, Nov 14, 2022 at 03:40:04PM -0800, Nathan Bossart wrote: >> Thanks for taking a look! Here is a rebased version of the patch set. > Oops, apparently object_aclcheck() cannot be used for pg_class. Here is > another version that uses pg_class_aclcheck() instead. I'm not sure how I > missed this earlier. > OK, reading the history I think everyone is on board with expanding AclMode from uint32 to uint64. Is that right? If so I'm intending to commit at least the first two of these patches fairly soon. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, Nov 16, 2022 at 03:09:47PM -0500, Andrew Dunstan wrote: > OK, reading the history I think everyone is on board with expanding > AclMode from uint32 to uint64. Is that right? I skimmed through this thread again, and AFAICT folks are okay with this approach. I'm not aware of any remaining concerns. > If so I'm intending to > commit at least the first two of these patches fairly soon. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
rebased -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Nov 18, 2022 at 09:05:04AM -0800, Nathan Bossart wrote: > rebased another rebase -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Sat, Nov 19, 2022 at 10:50:04AM -0800, Nathan Bossart wrote: > another rebase Another rebase for cfbot. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On 2022-11-20 Su 11:57, Nathan Bossart wrote: > On Sat, Nov 19, 2022 at 10:50:04AM -0800, Nathan Bossart wrote: >> another rebase > Another rebase for cfbot. > I have committed the first couple of these to get them out of the way. But I think we need a bit of cleanup in the next patch. vacuum_is_relation_owner() looks like it's now rather misnamed. Maybe vacuum_is_permitted_for_relation()? Also I think we need a more thorough reworking of the comments around line 566. And I think we need a more detailed explanation of why the change in vacuum_rel is ok, and if it is OK we should adjust the head comment on the function. In any case I think this comment would be better English with "might" instead of "may": /* user may have the ANALYZE privilege */ cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, Nov 23, 2022 at 02:56:28PM -0500, Andrew Dunstan wrote: > I have committed the first couple of these to get them out of the way. Thanks! > But I think we need a bit of cleanup in the next patch. > vacuum_is_relation_owner() looks like it's now rather misnamed. Maybe > vacuum_is_permitted_for_relation()? Also I think we need a more thorough > reworking of the comments around line 566. And I think we need a more > detailed explanation of why the change in vacuum_rel is ok, and if it is > OK we should adjust the head comment on the function. > > In any case I think this comment would be better English with "might" > instead of "may": > > /* user may have the ANALYZE privilege */ I've attempted to address all your feedback in v13. Please let me know if anything needs further reworking. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On 2022-11-23 We 18:54, Nathan Bossart wrote: > On Wed, Nov 23, 2022 at 02:56:28PM -0500, Andrew Dunstan wrote: >> I have committed the first couple of these to get them out of the way. > Thanks! > >> But I think we need a bit of cleanup in the next patch. >> vacuum_is_relation_owner() looks like it's now rather misnamed. Maybe >> vacuum_is_permitted_for_relation()? Also I think we need a more thorough >> reworking of the comments around line 566. And I think we need a more >> detailed explanation of why the change in vacuum_rel is ok, and if it is >> OK we should adjust the head comment on the function. >> >> In any case I think this comment would be better English with "might" >> instead of "may": >> >> /* user may have the ANALYZE privilege */ > I've attempted to address all your feedback in v13. Please let me know if > anything needs further reworking. Thanks, pushed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Mon, Nov 28, 2022 at 12:13:13PM -0500, Andrew Dunstan wrote: > pushed. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Hello,
While looking into the new feature, I found the following situation with
the \dp command displaying privileges on the system tables:
GRANT VACUUM, ANALYZE ON TABLE pg_type TO alice;
SELECT relacl FROM pg_class WHERE oid = 'pg_type'::regclass;
relacl
-------------------------------------------------------------
{=r/postgres,postgres=arwdDxtvz/postgres,alice=vz/postgres}
(1 row)
But the \dp command does not show the granted privileges:
\dp pg_type
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+------+-------------------+-------------------+----------
(0 rows)
The comment in src/bin/psql/describe.c explains the situation:
/*
* Unless a schema pattern is specified, we suppress system and temp
* tables, since they normally aren't very interesting from a
permissions
* point of view. You can see 'em by explicit request though, eg
with \z
* pg_catalog.*
*/
So to see the privileges you have to explicitly specify the schema name:
\dp pg_catalog.pg_type
Access privileges
Schema | Name | Type | Access privileges | Column
privileges | Policies
------------+---------+-------+-----------------------------+-------------------+----------
pg_catalog | pg_type | table | =r/postgres +| |
| | |
postgres=arwdDxtvz/postgres+| |
| | | alice=vz/postgres | |
(1 row)
But perhaps this behavior should be reviewed or at least documented?
-----
Pavel Luzanov
On Mon, Dec 05, 2022 at 11:21:08PM +0300, Pavel Luzanov wrote: > But perhaps this behavior should be reviewed or at least documented? I wonder why \dpS wasn't added. I wrote up a patch to add it and the corresponding documentation that other meta-commands already have. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
Nathan Bossart <nathandbossart@gmail.com> writes:
> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> index 3b5ea3c137..bd967eaa78 100644
> --- a/src/backend/catalog/aclchk.c
> +++ b/src/backend/catalog/aclchk.c
> @@ -4202,6 +4202,26 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
> has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA))
> result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE));
>
> + /*
> + * Check if ACL_VACUUM is being checked and, if so, and not already set as
> + * part of the result, then check if the user is a member of the
> + * pg_vacuum_all_tables role, which allows VACUUM on all relations.
> + */
> + if (mask & ACL_VACUUM &&
> + !(result & ACL_VACUUM) &&
> + has_privs_of_role(roleid, ROLE_PG_VACUUM_ALL_TABLES))
> + result |= ACL_VACUUM;
> +
> + /*
> + * Check if ACL_ANALYZE is being checked and, if so, and not already set as
> + * part of the result, then check if the user is a member of the
> + * pg_analyze_all_tables role, which allows ANALYZE on all relations.
> + */
> + if (mask & ACL_ANALYZE &&
> + !(result & ACL_ANALYZE) &&
> + has_privs_of_role(roleid, ROLE_PG_ANALYZE_ALL_TABLES))
> + result |= ACL_ANALYZE;
> +
> return result;
> }
These checks are getting rather repetitive, how about a data-driven
approach, along the lines of the below patch? I'm not quite happy with
the naming of the struct and its members (and maybe it should be in a
header?), suggestions welcome.
- ilmari
From 34bac3aced60931b2e995c5e1e6269f40c0828f5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 1 Dec 2022 11:49:14 +0000
Subject: [PATCH] Make built-in role permission checking data-driven
---
src/backend/catalog/aclchk.c | 64 +++++++++++++-------------------
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 27 insertions(+), 38 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index bd967eaa78..434bd39124 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4084,6 +4084,22 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL);
}
+/*
+ * Actions that built-in roles can perform unconditionally
+ */
+typedef struct RoleAcl
+{
+ Oid role;
+ AclMode mode;
+} RoleAcl;
+
+static const RoleAcl builtin_role_acls[] = {
+ {ROLE_PG_READ_ALL_DATA, ACL_SELECT},
+ {ROLE_PG_WRITE_ALL_DATA, ACL_INSERT | ACL_UPDATE | ACL_DELETE},
+ {ROLE_PG_VACUUM_ALL_TABLES, ACL_VACUUM},
+ {ROLE_PG_ANALYZE_ALL_TABLES, ACL_ANALYZE},
+};
+
/*
* Routine for examining a user's privileges for a table
*
@@ -4182,45 +4198,17 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
ReleaseSysCache(tuple);
/*
- * Check if ACL_SELECT is being checked and, if so, and not set already as
- * part of the result, then check if the user is a member of the
- * pg_read_all_data role, which allows read access to all relations.
+ * For each built-in role, check if its permissions are being checked and,
+ * if so, and not set already as part of the result, then check if the
+ * user is a member of the role, and allow the action if so.
*/
- if (mask & ACL_SELECT && !(result & ACL_SELECT) &&
- has_privs_of_role(roleid, ROLE_PG_READ_ALL_DATA))
- result |= ACL_SELECT;
-
- /*
- * Check if ACL_INSERT, ACL_UPDATE, or ACL_DELETE is being checked and, if
- * so, and not set already as part of the result, then check if the user
- * is a member of the pg_write_all_data role, which allows
- * INSERT/UPDATE/DELETE access to all relations (except system catalogs,
- * which requires superuser, see above).
- */
- if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE) &&
- !(result & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&
- has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA))
- result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE));
-
- /*
- * Check if ACL_VACUUM is being checked and, if so, and not already set as
- * part of the result, then check if the user is a member of the
- * pg_vacuum_all_tables role, which allows VACUUM on all relations.
- */
- if (mask & ACL_VACUUM &&
- !(result & ACL_VACUUM) &&
- has_privs_of_role(roleid, ROLE_PG_VACUUM_ALL_TABLES))
- result |= ACL_VACUUM;
-
- /*
- * Check if ACL_ANALYZE is being checked and, if so, and not already set as
- * part of the result, then check if the user is a member of the
- * pg_analyze_all_tables role, which allows ANALYZE on all relations.
- */
- if (mask & ACL_ANALYZE &&
- !(result & ACL_ANALYZE) &&
- has_privs_of_role(roleid, ROLE_PG_ANALYZE_ALL_TABLES))
- result |= ACL_ANALYZE;
+ for (int i = 0; i < lengthof(builtin_role_acls); i++)
+ {
+ const RoleAcl *const builtin = &builtin_role_acls[i];
+ if (mask & builtin->mode && !(result & builtin->mode) &&
+ has_privs_of_role(roleid, builtin->role))
+ result |= (mask & builtin->mode);
+ }
return result;
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 58daeca831..1c36d241db 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2340,6 +2340,7 @@ RewriteState
RmgrData
RmgrDescData
RmgrId
+RoleAcl
RoleNameItem
RoleSpec
RoleSpecType
--
2.34.1
On 06.12.2022 03:04, Nathan Bossart wrote: > I wonder why \dpS wasn't added. I wrote up a patch to add it and the > corresponding documentation that other meta-commands already have. Yes, \dpS command and clarification in the documentation is exactly what is needed. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
On Tue, Dec 06, 2022 at 04:57:37PM +0300, Pavel Luzanov wrote:
> On 06.12.2022 03:04, Nathan Bossart wrote:
>> I wonder why \dpS wasn't added. I wrote up a patch to add it and the
>> corresponding documentation that other meta-commands already have.
>
> Yes, \dpS command and clarification in the documentation is exactly what is
> needed.
I created a new thread for this:
https://postgr.es/m/20221206193606.GB3078082%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Dec 06, 2022 at 11:47:50AM +0000, Dagfinn Ilmari Mannsåker wrote:
> These checks are getting rather repetitive, how about a data-driven
> approach, along the lines of the below patch? I'm not quite happy with
> the naming of the struct and its members (and maybe it should be in a
> header?), suggestions welcome.
+1. I wonder if we should also consider checking all the bits at once
before we start checking for the predefined roles. I'm thinking of
something a bit like this:
role_mask = ACL_SELECT | ACL_INSERT | ACL_UPDATE |
ACL_DELETE | ACL_VACUUM | ACL_ANALYZE;
if (mask & role_mask != result & role_mask)
{
... existing checks here ...
}
I'm skeptical this actually produces any measurable benefit, but presumably
the predefined roles list will continue to grow, so maybe it's still worth
adding a fast path.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com