Обсуждение: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Add new predefined role pg_maintenance, which can issue VACUUM, ANALYZE, CHECKPOINT. Patch attached. Regards, Jeff Davis
Вложения
On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis <pgsql@j-davis.com> wrote: > > Add new predefined role pg_maintenance, which can issue VACUUM, > ANALYZE, CHECKPOINT. > > Patch attached. At this point, the idea of having a new role for maintenance work looks good. With this patch and Mark Dilger's patch introducing a bunch of new predefined roles, one concern is that we might reach to a state where we will have patches being proposed for new predefined roles for every database activity and the superuser eventually will have nothing to do in the database, it just becomes dummy? I'm not sure if Mark Dilger's patch on new predefined roles has a suitable/same role that we can use here. Are there any other database activities that fall under the "maintenance" category? How about CLUSTER, REINDEX? I didn't check the code for their permissions. Regards, Bharath Rupireddy.
On Sun, Oct 24, 2021 at 7:49 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> Add new predefined role pg_maintenance, which can issue VACUUM,
> ANALYZE, CHECKPOINT.
Are there any other database activities that fall under the
"maintenance" category? How about CLUSTER, REINDEX? I didn't check the
code for their permissions.
I would not lump the I/O intensive cluster and reindexing commands, and vacuum full, into the same permission bucket as vacuum and analyze. Checkpoint fits in the middle of that continuum. However, given that both vacuum and analyze are run to ensure good planner statistics during normal usage of the database, while the others, including checkpoint, either are non-normal usage or don't influence the planner, I would shift checkpoint to the same permission that covers cluster and reindex.
David J.
On Sun, 2021-10-24 at 20:19 +0530, Bharath Rupireddy wrote: > At this point, the idea of having a new role for maintenance work > looks good. With this patch and Mark Dilger's patch introducing a > bunch of new predefined roles, one concern is that we might reach to > a > state where we will have patches being proposed for new predefined > roles for every database activity and the superuser eventually will > have nothing to do in the database, it just becomes dummy? The idea is that, in different environments, the notion of an "administrator" should have different capabilities and different risks. By making the privileges more fine-grained, we enable those different use cases. I don't see it as necessarily a problem if superuser doesn't have much left to do. > I'm not sure if Mark Dilger's patch on new predefined roles has a > suitable/same role that we can use here. I didn't see one. I think one of the most common reasons to do manual checkpoints and vacuums is for performance testing, so another potential name might be "pg_performance". But "pg_maintenance" seemed a slightly better fit. > Are there any other database activities that fall under the > "maintenance" category? How about CLUSTER, REINDEX? I didn't check > the > code for their permissions. I looked around and didn't see much else to fit into this category. CLUSTER and REINDEX are a little too specific for a generic maintenance operation -- it's unlikely that you'd want to perform those expensive operations just to tidy up. But if you think something else should fit, let me know. Thank you, Jeff Davis
On 10/24/21, 10:20 AM, "Jeff Davis" <pgsql@j-davis.com> wrote: > On Sun, 2021-10-24 at 20:19 +0530, Bharath Rupireddy wrote: >> Are there any other database activities that fall under the >> "maintenance" category? How about CLUSTER, REINDEX? I didn't check >> the >> code for their permissions. > > I looked around and didn't see much else to fit into this category. > CLUSTER and REINDEX are a little too specific for a generic maintenance > operation -- it's unlikely that you'd want to perform those expensive > operations just to tidy up. But if you think something else should fit, > let me know. My initial reaction was that members of pg_maintenance should be able to do all of these things (VACUUM, ANALYZE, CLUSTER, REINDEX, and CHECKPOINT). It's true that some of these are more expensive or disruptive than others, but how else could we divvy it up? Maybe one option is to have two separate roles, one for commands that require lower lock levels (i.e., ANALYZE and VACUUM without TRUNCATE and FULL), and another for all of the maintenance commands. Nathan
On Sun, 2021-10-24 at 21:32 +0000, Bossart, Nathan wrote: > My initial reaction was that members of pg_maintenance should be able > to do all of these things (VACUUM, ANALYZE, CLUSTER, REINDEX, and > CHECKPOINT). What about REFRESH MATERIALIZED VIEW? That seems more specific to a workload, but it's hard to draw a clear line between that and CLUSTER. > Maybe one > option is to have two separate roles, one for commands that require > lower lock levels (i.e., ANALYZE and VACUUM without TRUNCATE and > FULL), and another for all of the maintenance commands. My main motivation is CHECKPOINT and database-wide VACUUM and ANALYZE. I'm fine extending it if others think it would be worthwhile, but it goes beyond my use case. Regards, Jeff Davis
On 10/24/21, 11:13 PM, "Jeff Davis" <pgsql@j-davis.com> wrote: > On Sun, 2021-10-24 at 21:32 +0000, Bossart, Nathan wrote: >> My initial reaction was that members of pg_maintenance should be able >> to do all of these things (VACUUM, ANALYZE, CLUSTER, REINDEX, and >> CHECKPOINT). > > What about REFRESH MATERIALIZED VIEW? That seems more specific to a > workload, but it's hard to draw a clear line between that and CLUSTER. Hm. CLUSTER reorders the content of a table but does not change it. REFRESH MATERIALIZED VIEW, on the other hand, does replace the content. I think that's the sort of line I'd draw between REFRESH MATERIALIZED VIEW and the other commands as well, so I'd leave it out of pg_maintenance. Nathan
Greetings, * Bharath Rupireddy (bharath.rupireddyforpostgres@gmail.com) wrote: > On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis <pgsql@j-davis.com> wrote: > > Add new predefined role pg_maintenance, which can issue VACUUM, > > ANALYZE, CHECKPOINT. > > > > Patch attached. > > At this point, the idea of having a new role for maintenance work > looks good. With this patch and Mark Dilger's patch introducing a > bunch of new predefined roles, one concern is that we might reach to a > state where we will have patches being proposed for new predefined > roles for every database activity and the superuser eventually will > have nothing to do in the database, it just becomes dummy? Independent of other things, getting to the point where everything can be done in the database without the need for superuser is absolutely a good goal to be striving for, not something to be avoiding. I don't think that makes superuser become 'dummy', but perhaps the only explicit superuser check we end up needing is "superuser is a member of all roles". That would be a very cool end state. Thanks, Stephen
Вложения
Stephen Frost <sfrost@snowman.net> writes: > Independent of other things, getting to the point where everything can > be done in the database without the need for superuser is absolutely a > good goal to be striving for, not something to be avoiding. > I don't think that makes superuser become 'dummy', but perhaps the > only explicit superuser check we end up needing is "superuser is a > member of all roles". That would be a very cool end state. I'm not entirely following how that's going to work. It implies that there is some allegedly-not-superuser role that has the ability to become superuser -- either within SQL or by breaking out to the OS -- because certainly a superuser can do those things. I don't think we're serving any good purpose by giving people the impression that roles with such permissions are somehow not superuser-equivalent. Certainly, the providers who don't want to give users superuser are just going to need a longer list of roles they won't give access to (and they probably won't be pleased about having to vet every predefined role carefully). regards, tom lane
Greetings, * Jeff Davis (pgsql@j-davis.com) wrote: > On Sun, 2021-10-24 at 21:32 +0000, Bossart, Nathan wrote: > > My initial reaction was that members of pg_maintenance should be able > > to do all of these things (VACUUM, ANALYZE, CLUSTER, REINDEX, and > > CHECKPOINT). > > What about REFRESH MATERIALIZED VIEW? That seems more specific to a > workload, but it's hard to draw a clear line between that and CLUSTER. Let's not forget that there are already existing non-superusers who can run things like REFRESH MATERIALIZED VIEW- the owner. > > Maybe one > > option is to have two separate roles, one for commands that require > > lower lock levels (i.e., ANALYZE and VACUUM without TRUNCATE and > > FULL), and another for all of the maintenance commands. > > My main motivation is CHECKPOINT and database-wide VACUUM and ANALYZE. > I'm fine extending it if others think it would be worthwhile, but it > goes beyond my use case. I've been wondering what the actual use-case here is. DB-wide VACUUM and ANALYZE are already able to be run by the database owner, but probably more relevant is that DB-wide VACUUMs and ANALYZEs shouldn't really be necessary given autovacuum, so why are we adding predefined roles which will encourage users to do that? I was also contemplating a different angle on this- allowing users to request autovacuum to run vacuum/analyze on a particular table. This would have the advantage that you get the vacuum/analyze behavior that autovacuum has (giving up an attempted truncate lock if another process wants a lock on the table, going at a slower pace rather than going all out and sucking up lots of I/O, etc). I'm not completely against this approach but just would like a bit better understanding of why it makes sense and what things we'll be able to say about what this role can/cannot do. Lastly though- I dislike the name, it seems far too general. I get that naming things is hard but maybe we could find something better than 'pg_maintenance' for this. Thanks, Stephen
Вложения
Greetings, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > Independent of other things, getting to the point where everything can > > be done in the database without the need for superuser is absolutely a > > good goal to be striving for, not something to be avoiding. > > I don't think that makes superuser become 'dummy', but perhaps the > > only explicit superuser check we end up needing is "superuser is a > > member of all roles". That would be a very cool end state. > > I'm not entirely following how that's going to work. It implies that > there is some allegedly-not-superuser role that has the ability to > become superuser -- either within SQL or by breaking out to the OS -- > because certainly a superuser can do those things. I don't think it implies that at all. Either that, or I'm not following what you're saying here. We have predefined roles today which aren't superusers and yet they're able to break out to the OS. Of course they can become superusers if they put the effort in. None of that gets away from the more general idea that we could get to a point where all of a superuser's capabilities come from roles which the superuser is automatically a member of such that we need have only one explicit superuser() check. > I don't think we're serving any good purpose by giving people the > impression that roles with such permissions are somehow not > superuser-equivalent. Certainly, the providers who don't want to > give users superuser are just going to need a longer list of roles > they won't give access to (and they probably won't be pleased about > having to vet every predefined role carefully). I agree that we need to be clear through the documentation about which predefined roles are able to "break outside the box" and become superuser, but that's independent from the question of if we will get to a point where every capability the superuser has can also be given through membership in some predefined role or not. That providers have to figure out what makes sense for them in terms of what they'll allow their users to do or not do doesn't seem entirely relevant here- different providers are by definition different and some might be fine with given out certain rights that others don't want to give out. That's actually kind of the point of breaking out all of these capabilities into more fine-grained ways of granting capabilities out. We already have roles today which are ones that can break outside the box and get to the OS and are able to do things that a superuser can do, or become a superuser themselves, and that's been generally a positive thing. We also have roles which are able to do things that only superusers used to be able to do but which are not able to become superusers themselves and aren't able to break out of the box. I don't see any reason why we can't continue with this and eventually eliminate the explicit superuser() checks except from the one where a superuser is a member of all roles. Sure, some of those roles give capabilities which can be used to become superuser, while others don't, but I hardly see that as an argument against the general idea that each is able to be independently GRANT'd. If nothing else, if we could eventually get to the point where there's only one such explicit check then maybe we'd stop creating *new* places where capabilities are locked behind a superuser check. I did an audit once upon a time of all superuser checks and rather than that number going down, as I had hoped at the time, it's been going up instead across new releases. I view that as unfortunate. Thanks, Stephen
Вложения
On Mon, 2021-10-25 at 13:54 -0400, Stephen Frost wrote: > Let's not forget that there are already existing non-superusers who > can > run things like REFRESH MATERIALIZED VIEW- the owner. Right, that's one reason why I don't see a particular use case there. But CHECKPOINT right now has an explicit superuser check, and it would be nice to be able to avoid that. It's pretty normal to issue a CHECKPOINT right after a data load and before running a performance test, right? Shouldn't there be some way to do that without superuser? Regards, Jeff Davis
On 2021-Oct-25, Jeff Davis wrote: > But CHECKPOINT right now has an explicit superuser check, and it would > be nice to be able to avoid that. > > It's pretty normal to issue a CHECKPOINT right after a data load and > before running a performance test, right? Shouldn't there be some way > to do that without superuser? Maybe you just need pg_checkpointer. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
> On Oct 24, 2021, at 7:49 AM, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > At this point, the idea of having a new role for maintenance work > looks good. With this patch and Mark Dilger's patch introducing a > bunch of new predefined roles, one concern is that we might reach to a > state where we will have patches being proposed for new predefined > roles for every database activity and the superuser eventually will > have nothing to do in the database, it just becomes dummy? > > I'm not sure if Mark Dilger's patch on new predefined roles has a > suitable/same role that we can use here. If you refer to the ALTER SYSTEM SET patches, which I agree introduce a number of new predefined roles, it may interest youthat Andrew has requested that I rework that patch set. In particular, he would like me to implement a new system ofgrants whereby the authority to ALTER SYSTEM SET can be granted per GUC rather than having predefined roles which hardcodedprivileges. I have not withdrawn the ALTER SYSTEM SET patches yet, as I don't know if Andrew's proposal can be made to work, but I wouldn'trecommend tying this pg_maintenance idea to that set. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, 2021-10-25 at 17:55 -0300, Alvaro Herrera wrote: > Maybe you just need pg_checkpointer. Fair enough. Attached simpler patch that only covers checkpoint, and calls the role pg_checkpointer. Regards, Jeff Davis
Вложения
On 10/25/21, 4:40 PM, "Jeff Davis" <pgsql@j-davis.com> wrote: > On Mon, 2021-10-25 at 17:55 -0300, Alvaro Herrera wrote: >> Maybe you just need pg_checkpointer. > > Fair enough. Attached simpler patch that only covers checkpoint, and > calls the role pg_checkpointer. It feels a bit excessive to introduce a new predefined role just for this. Perhaps this could be accomplished with a new function that could be granted. Nathan
On Tue, 2021-10-26 at 00:07 +0000, Bossart, Nathan wrote: > It feels a bit excessive to introduce a new predefined role just for > this. Perhaps this could be accomplished with a new function that > could be granted. It would be nice if the syntax could be used, since it's pretty widespread. I guess it does feel excessive to have its own predefined role, but at the same time it's hard to group a command like CHECKPOINT into a category. Maybe if we named it something like pg_performance or something we could make a larger group? Regards, Jeff Davis
On 10/25/21, 6:48 PM, "Jeff Davis" <pgsql@j-davis.com> wrote: > On Tue, 2021-10-26 at 00:07 +0000, Bossart, Nathan wrote: >> It feels a bit excessive to introduce a new predefined role just for >> this. Perhaps this could be accomplished with a new function that >> could be granted. > > It would be nice if the syntax could be used, since it's pretty > widespread. I guess it does feel excessive to have its own predefined > role, but at the same time it's hard to group a command like CHECKPOINT > into a category. Maybe if we named it something like pg_performance or > something we could make a larger group? I do think a larger group is desirable, but as is evidenced by this thread, it may be some time until we can figure out exactly how that would look. I feel like there's general support for being able to allow non-superusers to CHECKPOINT and do other maintenance/performance tasks, though. I think my main concern with pg_checkpointer is that it could set a precedent for new predefined roles, and we'd end up with dozens or more. But as long as new predefined roles aren't terribly expensive, maybe that's not all that bad. The advantage of having a pg_checkpointer role is that it enables users to grant just CHECKPOINT and nothing else. If we wanted a larger "pg_performance" group in the future, we could introduce that role and make it a member of pg_checkpointer and others (similar to how pg_monitor works). Nathan
Greetings, * Jeff Davis (pgsql@j-davis.com) wrote: > On Tue, 2021-10-26 at 00:07 +0000, Bossart, Nathan wrote: > > It feels a bit excessive to introduce a new predefined role just for > > this. Perhaps this could be accomplished with a new function that > > could be granted. > > It would be nice if the syntax could be used, since it's pretty > widespread. I guess it does feel excessive to have its own predefined > role, but at the same time it's hard to group a command like CHECKPOINT > into a category. Maybe if we named it something like pg_performance or > something we could make a larger group? For the use-case presented, I don't really buy off on this argument. We're talking about benchmarking tools, surely they can be and likely already are updated with some regularity for new major versions of PG. I wonder also if there aren't other things besides this that would need to be changed for them to work as a non-superuser anyway too, meaning this would be just one change among others that they'd need to make. In this specific case, I'd be more inclined to provide a function rather than an explicit predefined role for this one thing. Thanks, Stephen
Вложения
On Tue, 2021-10-26 at 16:02 -0400, Stephen Frost wrote: > We're talking about benchmarking tools What I had in mind was something much less formal, like a self- contained repro case of a performance problem. ... simple schema ... data load ... maybe build some indexes ... maybe set hints VACUUM ANALYZE; CHECKPOINT; I'm not saying it's a very strong use case, but at least for me, it's kind of a habit to throw in a CHECKPOINT after a quick data load for a test, even if it might not matter for whatever I'm testing. I guess I can change my habit to use a function instead, but then what's the point of the syntax? Should we just add a builtin function pg_checkpoint(), and deprecate the syntax? Regards, Jeff Davis
On 10/26/21, 2:04 PM, "Jeff Davis" <pgsql@j-davis.com> wrote: > Should we just add a builtin function pg_checkpoint(), and deprecate > the syntax? That seems reasonable to me. Nathan
On Wed, Oct 27, 2021 at 3:18 AM Bossart, Nathan <bossartn@amazon.com> wrote: > > On 10/26/21, 2:04 PM, "Jeff Davis" <pgsql@j-davis.com> wrote: > > Should we just add a builtin function pg_checkpoint(), and deprecate > > the syntax? > > That seems reasonable to me. IMHO, moving away from SQL command "CHECKPOINT" to function "pg_checkpoint()" isn't nice as the SQL command has been there for a long time and all the applications or services that were/are being built around the postgres ecosystem would have to adapt someday to the new function (if at all we deprecate the command and onboard the function). This isn't good at all given the CHECKPOINT is one of the mostly used commands in the apps or services layer. Moreover, if we go with the function pg_checkpoint(), we might see patches coming in for pg_vacuum(), pg_reindex(), pg_cluster() and so on. I suggest having a predefined role (pg_maintenance or pg_checkpoint(although I'm not sure convinced to have a separate role just for checkpoint) or some other) and let superuser and the users with this new predefined role do checkpoint. Regards, Bharath Rupireddy.
On Sat, 2021-10-30 at 13:24 +0530, Bharath Rupireddy wrote: > IMHO, moving away from SQL command "CHECKPOINT" to function > "pg_checkpoint()" isn't nice as the SQL command has been there for a > long time and all the applications or services that were/are being > built around the postgres ecosystem would have to adapt someday to > the > new function (if at all we deprecate the command and onboard the > function). This isn't good at all given the CHECKPOINT is one of the > mostly used commands in the apps or services layer. Moreover, if we > go > with the function pg_checkpoint(), we might see patches coming in for > pg_vacuum(), pg_reindex(), pg_cluster() and so on. I tend to agree with all of this. The CHECKPOINT command is already there and people already use it. If we are already chipping away at the need for superuser elsewhere, we should offer a way to use CHECKPOINT without being superuser. If the purpose[0] of predefined roles is that they allow you to do things that can't be expressed by GRANT, a predefined role pg_checkpointer seems to fit the bill. The main argument against[1] having a pg_checkpointer predefined role is that it creates a clutter of predefined roles. But it seems like just another part of the clutter of having a special SQL command merely for requesting a checkpoint. The alternative of creating a new function doesn't seem to alleviate the clutter. Some people will use the function and some will use the command, creating inconsistency in examples and scripts, and people will wonder which one is the "right" one. Regards, Jeff Davis [0] https://postgr.es/m/CA+TgmobQoWZn62qWRX+OOFjBPhdubxYTBeO-GSNPcLvBHh4ZvA@mail.gmail.com [1] https://postgr.es/m/8C661979-AF85-4AE1-9E2B-2A091DA3DB22@amazon.com
On 2021-Oct-30, Jeff Davis wrote: > I tend to agree with all of this. The CHECKPOINT command is already > there and people already use it. If we are already chipping away at the > need for superuser elsewhere, we should offer a way to use CHECKPOINT > without being superuser. +1 > If the purpose[0] of predefined roles is that they allow you to do > things that can't be expressed by GRANT, a predefined role > pg_checkpointer seems to fit the bill. +1 > The main argument against[1] having a pg_checkpointer predefined role > is that it creates a clutter of predefined roles. But it seems like > just another part of the clutter of having a special SQL command merely > for requesting a checkpoint. +1 -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Y una voz del caos me habló y me dijo "Sonríe y sé feliz, podría ser peor". Y sonreí. Y fui feliz. Y fue peor.
On 10/30/21, 11:14 AM, "Jeff Davis" <pgsql@j-davis.com> wrote: > On Sat, 2021-10-30 at 13:24 +0530, Bharath Rupireddy wrote: >> IMHO, moving away from SQL command "CHECKPOINT" to function >> "pg_checkpoint()" isn't nice as the SQL command has been there for a >> long time and all the applications or services that were/are being >> built around the postgres ecosystem would have to adapt someday to >> the >> new function (if at all we deprecate the command and onboard the >> function). This isn't good at all given the CHECKPOINT is one of the >> mostly used commands in the apps or services layer. Moreover, if we >> go >> with the function pg_checkpoint(), we might see patches coming in for >> pg_vacuum(), pg_reindex(), pg_cluster() and so on. > > I tend to agree with all of this. The CHECKPOINT command is already > there and people already use it. If we are already chipping away at the > need for superuser elsewhere, we should offer a way to use CHECKPOINT > without being superuser. I think Bharath brings up some good points. The simple fact is that CHECKPOINT has been around for a while, and creating functions for maintenance tasks would add just as much or more clutter than adding a predefined role for each one. I do wonder what we would've done if CHECKPOINT didn't already exist. Based on the goal of this thread, I get the feeling that we might've seriously considered introducing it as a function so that you can just GRANT EXECUTE as needed. Nathan
Greetings, * Bossart, Nathan (bossartn@amazon.com) wrote: > On 10/30/21, 11:14 AM, "Jeff Davis" <pgsql@j-davis.com> wrote: > > On Sat, 2021-10-30 at 13:24 +0530, Bharath Rupireddy wrote: > >> IMHO, moving away from SQL command "CHECKPOINT" to function > >> "pg_checkpoint()" isn't nice as the SQL command has been there for a > >> long time and all the applications or services that were/are being > >> built around the postgres ecosystem would have to adapt someday to > >> the > >> new function (if at all we deprecate the command and onboard the > >> function). This isn't good at all given the CHECKPOINT is one of the > >> mostly used commands in the apps or services layer. Moreover, if we > >> go > >> with the function pg_checkpoint(), we might see patches coming in for > >> pg_vacuum(), pg_reindex(), pg_cluster() and so on. > > > > I tend to agree with all of this. The CHECKPOINT command is already > > there and people already use it. If we are already chipping away at the > > need for superuser elsewhere, we should offer a way to use CHECKPOINT > > without being superuser. > > I think Bharath brings up some good points. The simple fact is that > CHECKPOINT has been around for a while, and creating functions for > maintenance tasks would add just as much or more clutter than adding a > predefined role for each one. I do wonder what we would've done if > CHECKPOINT didn't already exist. Based on the goal of this thread, I > get the feeling that we might've seriously considered introducing it > as a function so that you can just GRANT EXECUTE as needed. I don't really buy off on the "because it's been around a long time" as a reason to invent a predefined role for an individual command that doesn't take any options and could certainly just be a function. Applications developed to run as a superuser aren't likely to magically start working because they were GRANT'd this one additional predefined role either but likely would need other changes anyway. All that said, I wonder if we can have our cake and eat it too. I haven't looked into this at all yet and perhaps it's foolish on its face, but, could we make CHECKPOINT; basically turn around and just run select pg_checkpoint(); with the regular privilege checking happening? Then we'd keep the existing syntax working, but if the user is allowed to run the command would depend on if they've been GRANT'd EXECUTE rights on the function or not. Thanks, Stephen
Вложения
On 11/1/21, 9:51 AM, "Stephen Frost" <sfrost@snowman.net> wrote: > I don't really buy off on the "because it's been around a long time" as > a reason to invent a predefined role for an individual command that > doesn't take any options and could certainly just be a function. > Applications developed to run as a superuser aren't likely to magically > start working because they were GRANT'd this one additional predefined > role either but likely would need other changes anyway. I suspect the CHECKPOINT command wouldn't be removed anytime soon, either. I definitely understand the desire to avoid changing something that's been around a long time, but I think a function fits better in this case. > All that said, I wonder if we can have our cake and eat it too. I > haven't looked into this at all yet and perhaps it's foolish on its > face, but, could we make CHECKPOINT; basically turn around and just run > select pg_checkpoint(); with the regular privilege checking happening? > Then we'd keep the existing syntax working, but if the user is allowed > to run the command would depend on if they've been GRANT'd EXECUTE > rights on the function or not. I'd be worried about the behavior of CHECKPOINT changing because someone messed with the function. Nathan
Greetings, * Bossart, Nathan (bossartn@amazon.com) wrote: > On 11/1/21, 9:51 AM, "Stephen Frost" <sfrost@snowman.net> wrote: > > All that said, I wonder if we can have our cake and eat it too. I > > haven't looked into this at all yet and perhaps it's foolish on its > > face, but, could we make CHECKPOINT; basically turn around and just run > > select pg_checkpoint(); with the regular privilege checking happening? > > Then we'd keep the existing syntax working, but if the user is allowed > > to run the command would depend on if they've been GRANT'd EXECUTE > > rights on the function or not. > > I'd be worried about the behavior of CHECKPOINT changing because > someone messed with the function. Folks playing around in the catalog can break lots of things, I don't really see this as an argument against the idea. I do wonder if we should put a bit more effort into preventing people from messing with functions and such in pg_catalog. Being able to do something like: create or replace function xpath ( text, xml ) returns xml[] as $$ begin return 'xml'; end; $$ language plpgsql; (or with much worse functions..) strikes me as just a bit too easy to mistakenly cause problems as a superuser. Still, that's really an independent issue from this discussion. It's not like someone breaking CHECKPOINT; would actually impact normal checkpoints anyway. Thanks, Stephen
Вложения
On 11/1/21, 10:43 AM, "Stephen Frost" <sfrost@snowman.net> wrote: > Folks playing around in the catalog can break lots of things, I don't > really see this as an argument against the idea. > > I do wonder if we should put a bit more effort into preventing people > from messing with functions and such in pg_catalog. Being able to do > something like: > > create or replace function xpath ( text, xml ) returns xml[] > as $$ begin return 'xml'; end; $$ language plpgsql; > > (or with much worse functions..) strikes me as just a bit too easy to > mistakenly cause problems as a superuser. Still, that's really an > independent issue from this discussion. It's not like someone breaking > CHECKPOINT; would actually impact normal checkpoints anyway. Yeah, I see your point. Nathan
On Sat, Oct 23, 2021 at 5:45 PM Jeff Davis <pgsql@j-davis.com> wrote: > Add new predefined role pg_maintenance, which can issue VACUUM, > ANALYZE, CHECKPOINT. Just as a sort of general comment on this endeavor, I suspect that any attempt to lump things together that seem closely related is doomed to backfire. There's bound to be somebody who wants to grant some of these permissions and not others, or who wants to grant the ability to run those commands on some tables but not others. That's kind of unfortunate because it makes it more complicated to implement stuff like this ... but I've more or less given up hope on getting away with anything else. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, 2021-11-01 at 12:50 -0400, Stephen Frost wrote: > All that said, I wonder if we can have our cake and eat it too. I > haven't looked into this at all yet and perhaps it's foolish on its > face, but, could we make CHECKPOINT; basically turn around and just > run > select pg_checkpoint(); with the regular privilege checking > happening? > Then we'd keep the existing syntax working, but if the user is > allowed > to run the command would depend on if they've been GRANT'd EXECUTE > rights on the function or not. Great idea! Patch attached. This feels like a good pattern that we might want to use elsewhere, if the need arises. Regards, Jeff Davis
Вложения
On Tue, 2021-11-02 at 11:06 -0400, Robert Haas wrote: > Just as a sort of general comment on this endeavor, I suspect that > any > attempt to lump things together that seem closely related is doomed > to > backfire. Agreed, I think that is apparent from the different opinions in this thread. Robert had a good idea over here though: https://postgr.es/m/20211101165025.GS20998@tamriel.snowman.net which gives fine-grained control without the "clutter" of extra predefined roles. Regards, Jeff Davis
On 11/2/21, 10:29 AM, "Jeff Davis" <pgsql@j-davis.com> wrote: > Great idea! Patch attached. > > This feels like a good pattern that we might want to use elsewhere, if > the need arises. The approach in the patch looks alright to me, but another one could be to build a SelectStmt when parsing CHECKPOINT. I think that'd simplify the standard_ProcessUtility() changes. Otherwise, I see a couple of warnings when compiling: xlogfuncs.c:54: warning: implicit declaration of function ‘RequestCheckpoint’ xlogfuncs.c:56: warning: control reaches end of non-void function Nathan
Greetings, * Jeff Davis (pgsql@j-davis.com) wrote: > On Tue, 2021-11-02 at 11:06 -0400, Robert Haas wrote: > > Just as a sort of general comment on this endeavor, I suspect that > > any > > attempt to lump things together that seem closely related is doomed > > to > > backfire. > > Agreed, I think that is apparent from the different opinions in this > thread. > > Robert had a good idea over here though: Think you meant 'Stephen' there. ;) > https://postgr.es/m/20211101165025.GS20998@tamriel.snowman.net > > which gives fine-grained control without the "clutter" of extra > predefined roles. Right. * Bossart, Nathan (bossartn@amazon.com) wrote: > On 11/2/21, 10:29 AM, "Jeff Davis" <pgsql@j-davis.com> wrote: > > Great idea! Patch attached. > > > > This feels like a good pattern that we might want to use elsewhere, if > > the need arises. > > The approach in the patch looks alright to me, but another one could > be to build a SelectStmt when parsing CHECKPOINT. I think that'd > simplify the standard_ProcessUtility() changes. For my 2c, at least, I'm not really partial to either approach, though I'd want to see what error messages end up looking like. Seems like we might want to exercise a bit more control than we'd be able to if we transformed it directly into a SelectStmt (that is, we might add a HINT: roles with execute rights on pg_checkpoint() can run this command, or something; maybe not too tho). > Otherwise, I see a couple of warnings when compiling: > xlogfuncs.c:54: warning: implicit declaration of function ‘RequestCheckpoint’ > xlogfuncs.c:56: warning: control reaches end of non-void function Yeah, such things would need to be cleaned up, of course. Thanks! Stephen
Вложения
On 11/2/21, 11:27 AM, "Stephen Frost" <sfrost@snowman.net> wrote: > * Bossart, Nathan (bossartn@amazon.com) wrote: >> The approach in the patch looks alright to me, but another one could >> be to build a SelectStmt when parsing CHECKPOINT. I think that'd >> simplify the standard_ProcessUtility() changes. > > For my 2c, at least, I'm not really partial to either approach, though > I'd want to see what error messages end up looking like. Seems like we > might want to exercise a bit more control than we'd be able to if we > transformed it directly into a SelectStmt (that is, we might add a HINT: > roles with execute rights on pg_checkpoint() can run this command, or > something; maybe not too tho). I don't feel strongly one way or the other as well, but you have a good point about extra control over the error messages. The latest patch just does a standard aclcheck_error(), so you'd probably see "permission denied for function" if you didn't have privileges for CHECKPOINT. That could be confusing. Nathan
On 11/2/21 4:06 PM, Robert Haas wrote: > There's bound to be somebody who wants to grant some of > these permissions and not others, or who wants to grant the ability to > run those commands on some tables but not others. Is there anything stopping us from adding syntax like this? GRANT VACUUM, ANALYZE ON TABLE foo TO bar; That doesn't fix the CHECKPOINT issue, but surely vacuum and analyze can be done that way. I would much prefer that over new predefined roles. This would be nice, but there is nothing to hang our hat on: GRANT CHECKPOINT TO username; -- Vik Fearing
On Tue, Nov 2, 2021 at 3:14 PM Vik Fearing <vik@postgresfriends.org> wrote:
On 11/2/21 4:06 PM, Robert Haas wrote:
> There's bound to be somebody who wants to grant some of
> these permissions and not others, or who wants to grant the ability to
> run those commands on some tables but not others.
Is there anything stopping us from adding syntax like this?
GRANT VACUUM, ANALYZE ON TABLE foo TO bar;
That doesn't fix the CHECKPOINT issue, but surely vacuum and analyze can
be done that way. I would much prefer that over new predefined roles.
This would be nice, but there is nothing to hang our hat on:
GRANT CHECKPOINT TO username;
Here is the thread when I last brought up this idea five years ago:
I do not believe we've actually consumed any of the then available permission bits in the meanwhile.
David J.
On Tue, 2 Nov 2021 at 18:14, Vik Fearing <vik@postgresfriends.org> wrote:
On 11/2/21 4:06 PM, Robert Haas wrote:
> There's bound to be somebody who wants to grant some of
> these permissions and not others, or who wants to grant the ability to
> run those commands on some tables but not others.
Is there anything stopping us from adding syntax like this?
GRANT VACUUM, ANALYZE ON TABLE foo TO bar;
There is a limited number of bits available in the way privileges are stored. I investigated this in 2018 in connection with an idea I had to allow granting the ability to refresh a materialized view; after consideration and discussion I came to the idea of having a "MAINTAIN" permission which would allow refreshing materialized views and would also cover clustering, reindexing, vacuuming, and analyzing on objects to which those actions are applicable.
This message from me summarizes the history of usage of the available privilege bits:
If you dig into the replies you will find the revised proposal.
That doesn't fix the CHECKPOINT issue, but surely vacuum and analyze can
be done that way. I would much prefer that over new predefined roles.
This would be nice, but there is nothing to hang our hat on:
GRANT CHECKPOINT TO username;
On 11/2/21 11:14 PM, Vik Fearing wrote: > This would be nice, but there is nothing to hang our hat on: > > GRANT CHECKPOINT TO username; Thinking about this more, why don't we just add CHECKPOINT and NOCHECKPOINT attributes to roles? ALTER ROLE username WITH CHECKPOINT; -- Vik Fearing
On Tue, 2 Nov 2021 at 19:00, Vik Fearing <vik@postgresfriends.org> wrote:
On 11/2/21 11:14 PM, Vik Fearing wrote:
> This would be nice, but there is nothing to hang our hat on:
>
> GRANT CHECKPOINT TO username;
Thinking about this more, why don't we just add CHECKPOINT and
NOCHECKPOINT attributes to roles?
ALTER ROLE username WITH CHECKPOINT;
At present, this would require adding a field to pg_authid. This isn't very scalable; but we're already creating new pg_* roles which give access to various actions so I don't know why a role attribute would be a better approach. If anything, I think it would be more likely to move in the other direction: replace role attributes that in effect grant privileges with predefined roles. I think this has already been discussed here in the context of CREATEROLE.
> On 2 Nov 2021, at 19:26, Stephen Frost <sfrost@snowman.net> wrote: >> Otherwise, I see a couple of warnings when compiling: >> xlogfuncs.c:54: warning: implicit declaration of function ‘RequestCheckpoint’ >> xlogfuncs.c:56: warning: control reaches end of non-void function > > Yeah, such things would need to be cleaned up, of course. The Commitfest CI has -Werror,-Wimplicit-function-declaration on some platforms in which this patch breaks, so I think we should apply the below (or something similar) to ensure this is tested everywhere: diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 7ecaca4788..c9e1df39c1 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -26,6 +26,7 @@ #include "funcapi.h" #include "miscadmin.h" #include "pgstat.h" +#include "postmaster/bgwriter.h" #include "replication/walreceiver.h" #include "storage/fd.h" #include "storage/ipc.h" @@ -53,6 +54,7 @@ pg_checkpoint(PG_FUNCTION_ARGS) { RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT | (RecoveryInProgress() ? 0 : CHECKPOINT_FORCE)); + PG_RETURN_VOID(); } -- Daniel Gustafsson https://vmware.com/
Greetings, * Bossart, Nathan (bossartn@amazon.com) wrote: > On 11/2/21, 11:27 AM, "Stephen Frost" <sfrost@snowman.net> wrote: > > * Bossart, Nathan (bossartn@amazon.com) wrote: > >> The approach in the patch looks alright to me, but another one could > >> be to build a SelectStmt when parsing CHECKPOINT. I think that'd > >> simplify the standard_ProcessUtility() changes. > > > > For my 2c, at least, I'm not really partial to either approach, though > > I'd want to see what error messages end up looking like. Seems like we > > might want to exercise a bit more control than we'd be able to if we > > transformed it directly into a SelectStmt (that is, we might add a HINT: > > roles with execute rights on pg_checkpoint() can run this command, or > > something; maybe not too tho). > > I don't feel strongly one way or the other as well, but you have a > good point about extra control over the error messages. The latest > patch just does a standard aclcheck_error(), so you'd probably see > "permission denied for function" if you didn't have privileges for > CHECKPOINT. That could be confusing. Yeah, that's exactly the thing I was thinking about that might seem odd. I don't think it's a huge deal but I do think it'd be good for us to at least think about if we're ok with that or if we want to try and do something a bit better. Thanks, Stephen
Вложения
On Tue, 2021-11-02 at 14:26 -0400, Stephen Frost wrote: > Think you meant 'Stephen' there. ;) Yes ;-) > > The approach in the patch looks alright to me, but another one > > could > > be to build a SelectStmt when parsing CHECKPOINT. I think that'd > > simplify the standard_ProcessUtility() changes. Nathan, if I understand correctly, that would mean no CheckPointStmt at all. So it would either lack the right command tag, or we would have to hack it in somewhere. The utility changes in the existing patch are fairly minor, so I'll stick with that approach unless I'm missing something. > For my 2c, at least, I'm not really partial to either approach, > though > I'd want to see what error messages end up looking like. Seems like > we > might want to exercise a bit more control than we'd be able to if we > transformed it directly into a SelectStmt (that is, we might add a > HINT: > roles with execute rights on pg_checkpoint() can run this command, or > something; maybe not too tho). I changed the error message to: ERROR: permission denied for command CHECKPOINT HINT: The CHECKPOINT command requires the EXECUTE privilege on the function pg_checkpoint(). New version attached. Andres suggested that I also consider a new form of the GRANT clause that works on the CHECKPOINT command directly. I looked into that briefly, but in every other case it seems that GRANT works on an object (like a function). It doesn't feel like grating on a command is the right solution. The approach of using a function's ACL to represent the ACL of a higher-level command (as in this patch) does feel right to me. It feels like something we might extend to similar situations in the future; and even if we don't, it seems like a clean solution in isolation. Regards, Jeff Davis
Вложения
On Thu, Nov 4, 2021 at 12:03 PM Jeff Davis <pgsql@j-davis.com> wrote: > The approach of using a function's ACL to represent the ACL of a > higher-level command (as in this patch) does feel right to me. It feels > like something we might extend to similar situations in the future; and > even if we don't, it seems like a clean solution in isolation. It feels wrong to me. I realize that it's convenient to be able to re-use the existing GRANT and REVOKE commands that we have for functions, but actually DDL interfaces are better than SQL functions, because the syntax can be richer and you can avoid things like needing to take a snapshot. This particular patch dodges that problem, which is both a good thing and also clever, but it doesn't really make me feel any better about the concept in general. I think that the ongoing pressure to reduce as many things as possible to function permissions checks is ultimately going to turn out to be an unpleasant dead end. But by the time we reach that dead end we'll have put so much effort into making it work that it will be hard to change course, for backward-compatibility reasons among others. I don't have anything specific to propose, which I realize is kind of unhelpful ... but I don't like this, either. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, 2021-11-04 at 12:37 -0400, Robert Haas wrote: > I don't have anything specific to propose, which I realize is kind of > unhelpful ... but I don't like this, either. We can go back to having a pg_checkpoint predefined role that is only used for the CHECKPOINT command. The only real argument against that was a general sense of clutter, but I wasn't entirely convinced of that. If we have a specialized command, we have all kinds of clutter associated with that; a predefined role doesn't add much additional clutter. Regards, Jeff Davis
Hi, On 2021-11-02 10:28:39 -0700, Jeff Davis wrote: > On Mon, 2021-11-01 at 12:50 -0400, Stephen Frost wrote: > > All that said, I wonder if we can have our cake and eat it too. I > > haven't looked into this at all yet and perhaps it's foolish on its > > face, but, could we make CHECKPOINT; basically turn around and just > > run > > select pg_checkpoint(); with the regular privilege checking > > happening? > > Then we'd keep the existing syntax working, but if the user is > > allowed > > to run the command would depend on if they've been GRANT'd EXECUTE > > rights on the function or not. > > Great idea! Patch attached. > > This feels like a good pattern that we might want to use elsewhere, if > the need arises. > case T_CheckPointStmt: > - if (!superuser()) > - ereport(ERROR, > - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > - errmsg("must be superuser to do CHECKPOINT"))); > + { > + /* > + * Invoke pg_checkpoint(). Implementing the CHECKPOINT command > + * with a function allows administrators to grant privileges > + * on the CHECKPOINT command by granting privileges on the > + * pg_checkpoint() function. It also calls the function > + * execute hook, if present. > + */ > + AclResult aclresult; > + FmgrInfo flinfo; > + > + aclresult = pg_proc_aclcheck(F_PG_CHECKPOINT, GetUserId(), > + ACL_EXECUTE); > + if (aclresult != ACLCHECK_OK) > + aclcheck_error(aclresult, OBJECT_FUNCTION, > + get_func_name(F_PG_CHECKPOINT)); > > - RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT | > - (RecoveryInProgress() ? 0 : CHECKPOINT_FORCE)); > + InvokeFunctionExecuteHook(F_PG_CHECKPOINT); > + > + fmgr_info(F_PG_CHECKPOINT, &flinfo); > + > + (void) FunctionCall0Coll(&flinfo, InvalidOid); > + } > break; I don't like this. This turns the checkpoint command which previously didn't rely on the catalog in the happy path etc into something that requires most of the backend to be happily up to work. Greetings, Andres Freund
Hi, On 2021-11-04 14:25:54 -0700, Jeff Davis wrote: > On Thu, 2021-11-04 at 12:37 -0400, Robert Haas wrote: > > I don't have anything specific to propose, which I realize is kind of > > unhelpful ... but I don't like this, either. > > We can go back to having a pg_checkpoint predefined role that is only > used for the CHECKPOINT command. What about extending GRANT to allow to grant rights on commands? Yes, it'd be a bit of work to make that work in the catalogs, but it doesn't seem too hard to tackle. Greetings, Andres Freund
On Thu, 2021-11-04 at 15:46 -0700, Andres Freund wrote: > What about extending GRANT to allow to grant rights on commands? Yes, > it'd be > a bit of work to make that work in the catalogs, but it doesn't seem > too hard > to tackle. You mean for the CHECKPOINT command specifically, or for many commands? If it only applies to CHECKPOINT, it seems like more net clutter than a new predefined role. But I don't see it generalizing to a lot of commands, either. I looked at the list, and it's taking some creativity to think of more than a couple other commands where it makes sense. Maybe LISTEN/NOTIFY? But even then, there are three related commands: LISTEN, UNLISTEN, and NOTIFY. Are those one privilege representing them all, two (LISTEN/UNLISTEN, and NOTIFY), or three separate privileges? Regards, Jeff Davis
On Thu, 2021-11-04 at 15:42 -0700, Andres Freund wrote: > I don't like this. This turns the checkpoint command which previously > didn't > rely on the catalog in the happy path etc into something that > requires most of > the backend to be happily up to work. It seems like this specific approach has been mostly shot down already. But out of curiosity, are you intending to run CHECKPOINT during bootstrap or something? Regards, Jeff Davis
On Thu, Nov 4, 2021 at 5:25 PM Jeff Davis <pgsql@j-davis.com> wrote: > On Thu, 2021-11-04 at 12:37 -0400, Robert Haas wrote: > > I don't have anything specific to propose, which I realize is kind of > > unhelpful ... but I don't like this, either. > > We can go back to having a pg_checkpoint predefined role that is only > used for the CHECKPOINT command. I would prefer that approach. Other people may prefer other things, but if I got to pick, I'd say do it that way. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Nov 4, 2021 at 7:38 PM Jeff Davis <pgsql@j-davis.com> wrote: > It seems like this specific approach has been mostly shot down already. > But out of curiosity, are you intending to run CHECKPOINT during > bootstrap or something? Imagine a system with corruption in pg_proc. Right now, that won't prevent you from successfully executing a checkpoint. With this approach, it might. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Nov 4, 2021 at 6:46 PM Andres Freund <andres@anarazel.de> wrote: > What about extending GRANT to allow to grant rights on commands? Yes, it'd be > a bit of work to make that work in the catalogs, but it doesn't seem too hard > to tackle. I think that there aren't too many commands where the question is just whether you can execute the command or not. CHECKPOINT is one that does work that way, but if it's VACUUM or ANALYZE the question will be whether you can run it on a particular table; if it's ALTER SYSTEM it will be whether you can run it for that GUC; and so on. CHECKPOINT is one of the few commands that has no target. -- Robert Haas EDB: http://www.enterprisedb.com
On 2021-Nov-04, Jeff Davis wrote: > But I don't see it generalizing to a lot of commands, either. I looked > at the list, and it's taking some creativity to think of more than a > couple other commands where it makes sense. Maybe LISTEN/NOTIFY? But > even then, there are three related commands: LISTEN, UNLISTEN, and > NOTIFY. Are those one privilege representing them all, two > (LISTEN/UNLISTEN, and NOTIFY), or three separate privileges? What about things like CREATE SUBSCRIPTION/PUBLICATION? Sounds like it would be useful to allow non-superusers do those, too. That said, if the list is short, then additional predefined roles seem preferrable to having a ton of infrastructure code that might be much more clutter than what seems a short list of additional predefined roles. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "We're here to devour each other alive" (Hobbes)
On 2021-11-05 08:42:58 -0400, Robert Haas wrote: > On Thu, Nov 4, 2021 at 7:38 PM Jeff Davis <pgsql@j-davis.com> wrote: > > It seems like this specific approach has been mostly shot down already. > > But out of curiosity, are you intending to run CHECKPOINT during > > bootstrap or something? > > Imagine a system with corruption in pg_proc. Right now, that won't > prevent you from successfully executing a checkpoint. With this > approach, it might. Exactly. It wouldn't matter if checkpoints weren't something needed to potentially bring the system back into a sane state, but ...
Hi, On 2021-11-05 08:54:37 -0400, Robert Haas wrote: > On Thu, Nov 4, 2021 at 6:46 PM Andres Freund <andres@anarazel.de> wrote: > > What about extending GRANT to allow to grant rights on commands? Yes, it'd be > > a bit of work to make that work in the catalogs, but it doesn't seem too hard > > to tackle. > > I think that there aren't too many commands where the question is just > whether you can execute the command or not. CHECKPOINT is one that > does work that way, but if it's VACUUM or ANALYZE the question will be > whether you can run it on a particular table; if it's ALTER SYSTEM it > will be whether you can run it for that GUC; and so on. CHECKPOINT is > one of the few commands that has no target. I don't know if that's really such a big deal. It's useful to be able to grant the right to do a system wide ANALYZE etc to a role that can't otherwise do anything with the table. Even for ALTER SYSTEM etc it seems like it'd be helpful, because it allows to constrain an admin tool to "legitimate" admin paths, without allowing, say, UPDATE pg_proc. Greetings, Andres Freund
Greetings, * Andres Freund (andres@anarazel.de) wrote: > On 2021-11-05 08:42:58 -0400, Robert Haas wrote: > > On Thu, Nov 4, 2021 at 7:38 PM Jeff Davis <pgsql@j-davis.com> wrote: > > > It seems like this specific approach has been mostly shot down already. > > > But out of curiosity, are you intending to run CHECKPOINT during > > > bootstrap or something? > > > > Imagine a system with corruption in pg_proc. Right now, that won't > > prevent you from successfully executing a checkpoint. With this > > approach, it might. > > Exactly. It wouldn't matter if checkpoints weren't something needed to > potentially bring the system back into a sane state, but ... This really isn't that hard to address- do a superuser check, if it passes then just call the checkpoint function like CHECKPOINT; does today. Otherwise, check the perms on the function or just call the function in a manner which would check privileges, or maybe have another predefined role, though I continue to feel like the function based approach is better. If we're actually worried about catalog corruption (and, frankly, I've got some serious doubts that jumping in and running CHECKPOINT; by hand is a great idea if there's such active corruption) then we must use such an approach no matter how we allow non-superusers to run the command because any approach to that necessarily involves some amount of catalog access. Any concern leveraged against pg_proc applies equally to pg_auth_members after all, so having it be something role-based vs. function privilege is really just moving deck chairs around on the titanic at that point. Thanks, Stephen
Вложения
Greetings, * Andres Freund (andres@anarazel.de) wrote: > On 2021-11-05 08:54:37 -0400, Robert Haas wrote: > > On Thu, Nov 4, 2021 at 6:46 PM Andres Freund <andres@anarazel.de> wrote: > > > What about extending GRANT to allow to grant rights on commands? Yes, it'd be > > > a bit of work to make that work in the catalogs, but it doesn't seem too hard > > > to tackle. > > > > I think that there aren't too many commands where the question is just > > whether you can execute the command or not. CHECKPOINT is one that > > does work that way, but if it's VACUUM or ANALYZE the question will be > > whether you can run it on a particular table; if it's ALTER SYSTEM it > > will be whether you can run it for that GUC; and so on. CHECKPOINT is > > one of the few commands that has no target. > > I don't know if that's really such a big deal. It's useful to be able to grant > the right to do a system wide ANALYZE etc to a role that can't otherwise do > anything with the table. Even for ALTER SYSTEM etc it seems like it'd be > helpful, because it allows to constrain an admin tool to "legitimate" admin > paths, without allowing, say, UPDATE pg_proc. Note that it's already possible to have a non-superuser who can run VACUUM and ANALYZE on all non-shared tables in a database but who otherwise isn't able to mess with the tables owned by other users- that is something the database owner can do. Perhaps it's useful to break that out into a separately grantable permission but the argument should really by why you'd want to GRANT someone the ability to VACUUM/ANALYZE an entire database while *not* having them be the database owner. That's a much more narrow use-case vs. having them not be a superuser or be able to do things like UPDATE pg_proc. Thanks, Stephen
Вложения
Hi, On 2021-11-08 12:23:18 -0500, Stephen Frost wrote: > If we're actually worried about catalog corruption (and, frankly, I've > got some serious doubts that jumping in and running CHECKPOINT; by hand > is a great idea if there's such active corruption) I've been there when recovering from corruption. > though I continue to feel like the function based approach is better. I think it's a somewhat ugly hack. > then we must use such an approach no matter how we allow non-superusers to > run the command because any approach to that necessarily involves some > amount of catalog access. As long as there's no additional catalog access when the user is known to be a superuser, then I think it's fine. There's a difference between doing one pg_authid read for superuser - with a fallback to automatically assuming a user if one couldn't be found - and doing a full pg_proc read with several subsidiary pg_type reads etc. Greetings, Andres Freund
Greetings, * Alvaro Herrera (alvherre@alvh.no-ip.org) wrote: > On 2021-Nov-04, Jeff Davis wrote: > > But I don't see it generalizing to a lot of commands, either. I looked > > at the list, and it's taking some creativity to think of more than a > > couple other commands where it makes sense. Maybe LISTEN/NOTIFY? But > > even then, there are three related commands: LISTEN, UNLISTEN, and > > NOTIFY. Are those one privilege representing them all, two > > (LISTEN/UNLISTEN, and NOTIFY), or three separate privileges? > > What about things like CREATE SUBSCRIPTION/PUBLICATION? Sounds like it > would be useful to allow non-superusers do those, too. Agreed. Having these be limited to superusers is unfortunate, though at the time probably made sense as otherwise it would have made it that much more difficult to get logical replication in. Now is a great time to try and improve on that situation though. This is a bit tricky though since creating a subscription means that you'll be able to cause some code to be executed with higher privileges today, as I recall, and we'd need to make sure to address that. If we can make sure that a subscription isn't able to be used to execute code as effectively a superuser then I would think the other permission needed to create one, for tables which you own, would be just a "network access" kind of capability. In other words, I'm not 100% sure we need to have 'create subscription' require different privileges from 'create a foreign server'. Then again, having additional predefined rules isn't a huge cost and perhaps it would be better to avoid the confusion that introducing a separate 'capabilities' kind of system would involve where those capabilities cross multiple commands. > That said, if the list is short, then additional predefined roles seem > preferrable to having a ton of infrastructure code that might be much > more clutter than what seems a short list of additional predefined roles. None of this strikes me as a 'ton of infrastructure code' and so I'm not quite sure I'm following the argument being made here. Thanks, Stephen
Вложения
On 2021-Nov-08, Stephen Frost wrote: > * Alvaro Herrera (alvherre@alvh.no-ip.org) wrote: > > That said, if the list is short, then additional predefined roles seem > > preferrable to having a ton of infrastructure code that might be much > > more clutter than what seems a short list of additional predefined roles. > > None of this strikes me as a 'ton of infrastructure code' and so I'm not > quite sure I'm following the argument being made here. I was referring specifically to Andres' idea of having additional DDL commands handled as special GRANTable privileges, https://postgr.es/m/20211104224636.5qg6cfyjkw52rh4d@alap3.anarazel.de -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)
Greetings, * Andres Freund (andres@anarazel.de) wrote: > On 2021-11-08 12:23:18 -0500, Stephen Frost wrote: > > though I continue to feel like the function based approach is better. > > I think it's a somewhat ugly hack. I suppose we'll just have to disagree on that. :) I don't feel as strongly as others apparently do on this point though, and I'd rather have non-superusers able to run CHECKPOINT *somehow* than not, so if the others feel like a predefined role is a better approach then I'm alright with that. Seems a bit overkill to me but it's also not like it's actually all that much code or work to do. > > then we must use such an approach no matter how we allow non-superusers to > > run the command because any approach to that necessarily involves some > > amount of catalog access. > > As long as there's no additional catalog access when the user is known to be a > superuser, then I think it's fine. There's a difference between doing one > pg_authid read for superuser - with a fallback to automatically assuming a > user if one couldn't be found - and doing a full pg_proc read with several > subsidiary pg_type reads etc. Yes, the approach I'm suggesting would make the superuser-run CHECKPOINT be exactly the same as it is today, while a non-superuser trying to run a CHECKPOINT would end up doing additional catalog accesses. Thanks, Stephen
Вложения
Greetings, * Alvaro Herrera (alvherre@alvh.no-ip.org) wrote: > On 2021-Nov-08, Stephen Frost wrote: > > > * Alvaro Herrera (alvherre@alvh.no-ip.org) wrote: > > > > That said, if the list is short, then additional predefined roles seem > > > preferrable to having a ton of infrastructure code that might be much > > > more clutter than what seems a short list of additional predefined roles. > > > > None of this strikes me as a 'ton of infrastructure code' and so I'm not > > quite sure I'm following the argument being made here. > > I was referring specifically to Andres' idea of having additional DDL > commands handled as special GRANTable privileges, > https://postgr.es/m/20211104224636.5qg6cfyjkw52rh4d@alap3.anarazel.de Ah, thanks, I had seen that but didn't quite associate it to this comment. Perhaps not a surprise, but I tend to favor predefined roles for these kinds of things. If we do want to revamp how GRANT works, I'd argue for first splitting up the way we handle privileges to be on a per-object-type basis and once we did that then we could extend that to allow GRANT on commands more easily (and with more variety as to what privileges a GRANT on a command could be). It's kind of cute to have one bitmap covering all objects but it puts us into a place where extending what can be GRANT'd on one kind of object necessarily impacts our ability to GRANT on other kinds (eg: we have a bit reserved for TRUNCATE in the same bitmask for a schema as we do for a table, but we don't allow TRUNCATE on schemas and probably never will). Thanks, Stephen
Вложения
Greetings, * Isaac Morland (isaac.morland@gmail.com) wrote: > On Tue, 2 Nov 2021 at 19:00, Vik Fearing <vik@postgresfriends.org> wrote: > > On 11/2/21 11:14 PM, Vik Fearing wrote: > > > > > This would be nice, but there is nothing to hang our hat on: > > > > > > GRANT CHECKPOINT TO username; > > > > Thinking about this more, why don't we just add CHECKPOINT and > > NOCHECKPOINT attributes to roles? > > > > ALTER ROLE username WITH CHECKPOINT; > > At present, this would require adding a field to pg_authid. This isn't very > scalable; but we're already creating new pg_* roles which give access to > various actions so I don't know why a role attribute would be a better > approach. If anything, I think it would be more likely to move in the other > direction: replace role attributes that in effect grant privileges with > predefined roles. I think this has already been discussed here in the > context of CREATEROLE. Yes, much better to create predefined roles for this kind of thing and, ideally, move explicitly away from role attributes. Thanks, Stephen
Вложения
On Mon, 2021-11-08 at 12:47 -0500, Stephen Frost wrote: > > I don't feel as strongly as others apparently do on this point > though, > and I'd rather have non-superusers able to run CHECKPOINT *somehow* > than not, so if the others feel like a predefined role is a better > approach then I'm alright with that. Seems a bit overkill to me but > it's also not like it's actually all that much code or work to do. +1. It seems like the pg_checkpointer predefined role is the most acceptable to everyone (even if not universally liked). Attached a rebased version of that patch. Regards, Jeff Davis
Вложения
Greetings, * Jeff Davis (pgsql@j-davis.com) wrote: > On Mon, 2021-11-08 at 12:47 -0500, Stephen Frost wrote: > > > > I don't feel as strongly as others apparently do on this point > > though, > > and I'd rather have non-superusers able to run CHECKPOINT *somehow* > > than not, so if the others feel like a predefined role is a better > > approach then I'm alright with that. Seems a bit overkill to me but > > it's also not like it's actually all that much code or work to do. > > +1. It seems like the pg_checkpointer predefined role is the most > acceptable to everyone (even if not universally liked). > > Attached a rebased version of that patch. Thanks. Quick review- > diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c > index bf085aa93b2..0ff832a62c2 100644 > --- a/src/backend/tcop/utility.c > +++ b/src/backend/tcop/utility.c > @@ -24,6 +24,7 @@ > #include "catalog/catalog.h" > #include "catalog/index.h" > #include "catalog/namespace.h" > +#include "catalog/pg_authid.h" > #include "catalog/pg_inherits.h" > #include "catalog/toasting.h" > #include "commands/alter.h" > @@ -939,10 +940,10 @@ standard_ProcessUtility(PlannedStmt *pstmt, > break; > > case T_CheckPointStmt: > - if (!superuser()) > + if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINTER)) > ereport(ERROR, > (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > - errmsg("must be superuser to do CHECKPOINT"))); > + errmsg("must be member of pg_checkpointer to do CHECKPOINT"))); Most such error messages say 'superuser or '... Also, note the recent thread about trying to ensure that places are using has_privs_of_role() as you're doing here but also say that in the error message consistently, rather than 'member of' it should really be 'has privileges of' as the two are not necessarily always the same. You can be a member of a role but not actively have the privileges of that role. Otherwise, looks pretty good to me. I'll note that has_privs_of_role() will first call superuser_arg(member), just the same as the prior superuser() check did, so this doesn't change the catalog accesses in that case from today. Thanks, Stephen