Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings,
* Jeff Davis (pg...@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
signature.asc
Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
diff --git a/doc/src/sgml/ref/checkpoint.sgml b/doc/src/sgml/ref/checkpoint.sgml
index 2afee6d7b59..aebe8f8cd77 100644
--- a/doc/src/sgml/ref/checkpoint.sgml
+++ b/doc/src/sgml/ref/checkpoint.sgml
@@ -52,7 +52,8 @@ CHECKPOINT
- Only superusers can call CHECKPOINT.
+ Only superusers or members of the pg_checkpointer role
+ can call CHECKPOINT.
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index afbf67c28cf..d5482cd5fc6 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -582,6 +582,10 @@ DROP ROLE doomed_role;
Allow executing programs on the database server as the user the database runs as with
COPY and other functions which allow executing a server-side program.
+
+ pg_checkpointer
+ Allow executing the CHECKPOINT command.
+
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")));
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
(RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 9faf017457a..f0707678037 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 202110272
+#define CATALOG_VERSION_NO 202111081
#endif
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 3da68016b61..9c65174f3c6 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -79,5 +79,10 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '4544', oid_symbol => 'ROLE_PG_CHECKPOINTER',
+ rolname => 'pg_checkpointer', rolsuper => 'f', rolinherit => 't',
+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+ rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+ rolpassword => '_null_', rolvaliduntil => '_null_' },
]
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Isaac Morland (isaac.morl...@gmail.com) wrote: > On Tue, 2 Nov 2021 at 19:00, Vik Fearing 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Alvaro Herrera (alvhe...@alvh.no-ip.org) wrote: > On 2021-Nov-08, Stephen Frost wrote: > > > * Alvaro Herrera (alvhe...@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.5qg6cfyjkw52r...@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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Andres Freund (and...@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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 2021-Nov-08, Stephen Frost wrote: > * Alvaro Herrera (alvhe...@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.5qg6cfyjkw52r...@alap3.anarazel.de -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Alvaro Herrera (alvhe...@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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Andres Freund (and...@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 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Andres Freund (and...@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 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Hi, On 2021-11-05 08:54:37 -0400, Robert Haas wrote: > On Thu, Nov 4, 2021 at 6:46 PM 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. > > 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 2021-11-05 08:42:58 -0400, Robert Haas wrote: > On Thu, Nov 4, 2021 at 7:38 PM Jeff Davis 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 ...
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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)
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Thu, Nov 4, 2021 at 6:46 PM 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. 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Thu, Nov 4, 2021 at 7:38 PM Jeff Davis 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Thu, Nov 4, 2021 at 5:25 PM 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. 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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;
> + FmgrInfoflinfo;
> +
> + 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, );
> +
> + (void) FunctionCall0Coll(, 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Thu, Nov 4, 2021 at 12:03 PM Jeff Davis 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4b49dff2ffc..3558044221a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25487,6 +25487,24 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560
+
+
+
+ pg_checkpoint
+
+pg_checkpoint ()
+void
+
+
+Request an immediate checkpoint. This function implements the command, so the behavior is identical. This
+function is restricted to superusers by default, but other users can
+be granted EXECUTE to run the function. If a user has permission to
+execute this function, they also have permission to issue a
+CHECKPOINT command.
+
+
+
diff --git a/doc/src/sgml/ref/checkpoint.sgml b/doc/src/sgml/ref/checkpoint.sgml
index 2afee6d7b59..7c6bea05fa0 100644
--- a/doc/src/sgml/ref/checkpoint.sgml
+++ b/doc/src/sgml/ref/checkpoint.sgml
@@ -52,7 +52,11 @@ CHECKPOINT
- Only superusers can call CHECKPOINT.
+ The CHECKPOINT is equivalent to calling the function
+ pg_checkpoint(). By
+ default, only superusers can this function, but if other users are granted
+ privileges on it, they may also call the CHECKPOINT
+ command.
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index b98deb72ec6..c9e1df39c11 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"
@@ -44,6 +45,18 @@
static StringInfo label_file;
static StringInfo tblspc_map_file;
+/*
+ * Implements the CHECKPOINT command. To allow non-superusers to perform the
+ * CHECKPOINT command, grant privileges on this function.
+ */
+Datum
+pg_checkpoint(PG_FUNCTION_ARGS)
+{
+ RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
+ (RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
+ PG_RETURN_VOID();
+}
+
/*
* pg_start_backup: set up for taking an on-line backup dump
*
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 54c93b16c4c..4437eb3010b 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -603,6 +603,8 @@ AS 'unicode_is_normalized';
-- available to superuser / cluster owner, if they choose.
--
+REVOKE EXECUTE ON FUNCTION pg_checkpoint() FROM public;
+
REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public;
REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index bf085aa93b2..dcd822075f5 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/objectaccess.h"
#include "catalog/pg_inherits.h"
#include "catalog/toasting.h"
#include "commands/alter.h"
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Bossart, Nathan (bossa...@amazon.com) wrote: > On 11/2/21, 11:27 AM, "Stephen Frost" wrote: > > * Bossart, Nathan (bossa...@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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
> On 2 Nov 2021, at 19:26, Stephen Frost 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/
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Tue, 2 Nov 2021 at 19:00, Vik Fearing 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.
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Tue, 2 Nov 2021 at 18:14, Vik Fearing 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: https://www.postgresql.org/message-id/CAMsGm5c4DycKBYZCypfV02s-SC8GwF%2BKeTt%3D%3DvbWrFn%2Bdz%3DKeg%40mail.gmail.com 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; >
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Tue, Nov 2, 2021 at 3:14 PM Vik Fearing 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: https://www.postgresql.org/message-id/CAKFQuwaAhVt6audf92Q1VrELfJ%2BPz%3DuDfNb8%3D1_bqAmyDpnDmA%40mail.gmail.com I do not believe we've actually consumed any of the then available permission bits in the meanwhile. David J.
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 11/2/21, 11:27 AM, "Stephen Frost" wrote: > * Bossart, Nathan (bossa...@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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Jeff Davis (pg...@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.gs20...@tamriel.snowman.net > > which gives fine-grained control without the "clutter" of extra > predefined roles. Right. * Bossart, Nathan (bossa...@amazon.com) wrote: > On 11/2/21, 10:29 AM, "Jeff Davis" 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 11/2/21, 10:29 AM, "Jeff Davis" 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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.gs20...@tamriel.snowman.net which gives fine-grained control without the "clutter" of extra predefined roles. Regards, Jeff Davis
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4b49dff2ffc..1e3152c39b1 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25487,6 +25487,23 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560
+
+
+
+ pg_checkpoint
+
+pg_checkpoint ()
+
+
+Request an immediate checkpoint. This function implements the command, so the behavior is identical. This
+function is restricted to superusers by default, but other users can
+be granted EXECUTE to run the function. If a user has permission to
+execute this function, they also have permission to issue a
+CHECKPOINT command.
+
+
+
diff --git a/doc/src/sgml/ref/checkpoint.sgml b/doc/src/sgml/ref/checkpoint.sgml
index 2afee6d7b59..ec2c1a62050 100644
--- a/doc/src/sgml/ref/checkpoint.sgml
+++ b/doc/src/sgml/ref/checkpoint.sgml
@@ -52,7 +52,10 @@ CHECKPOINT
- Only superusers can call CHECKPOINT.
+ By default, only superusers can call CHECKPOINT, but
+ permission can be granted to other users by granting privileges on the
+ pg_checkpoint()
+ function.
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index b98deb72ec6..7ecaca47885 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -44,6 +44,17 @@
static StringInfo label_file;
static StringInfo tblspc_map_file;
+/*
+ * Implements the CHECKPOINT command. To allow non-superusers to perform the
+ * CHECKPOINT command, grant privileges on this function.
+ */
+Datum
+pg_checkpoint(PG_FUNCTION_ARGS)
+{
+ RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
+ (RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
+}
+
/*
* pg_start_backup: set up for taking an on-line backup dump
*
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 54c93b16c4c..4437eb3010b 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -603,6 +603,8 @@ AS 'unicode_is_normalized';
-- available to superuser / cluster owner, if they choose.
--
+REVOKE EXECUTE ON FUNCTION pg_checkpoint() FROM public;
+
REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public;
REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index bf085aa93b2..cb55544d7be 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/objectaccess.h"
#include "catalog/pg_inherits.h"
#include "catalog/toasting.h"
#include "commands/alter.h"
@@ -67,6 +68,7 @@
#include "tcop/pquery.h"
#include "tcop/utility.h"
#include "utils/acl.h"
+#include "utils/fmgroids.h"
#include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
@@ -939,13 +941,29 @@ standard_ProcessUtility(PlannedStmt *pstmt,
break;
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, );
+
+(void) FunctionCall0Coll(, InvalidOid);
+ }
break;
case T_ReindexStmt:
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sat, Oct 23, 2021 at 5:45 PM Jeff Davis 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 11/1/21, 10:43 AM, "Stephen Frost" 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Bossart, Nathan (bossa...@amazon.com) wrote: > On 11/1/21, 9:51 AM, "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. > > 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 11/1/21, 9:51 AM, "Stephen Frost" 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Bossart, Nathan (bossa...@amazon.com) wrote: > On 10/30/21, 11:14 AM, "Jeff Davis" 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/30/21, 11:14 AM, "Jeff Davis" 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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.
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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-gsnpclvbhh4...@mail.gmail.com [1] https://postgr.es/m/8c661979-af85-4ae1-9e2b-2a091da3d...@amazon.com
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Wed, Oct 27, 2021 at 3:18 AM Bossart, Nathan wrote: > > On 10/26/21, 2:04 PM, "Jeff Davis" 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.
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/26/21, 2:04 PM, "Jeff Davis" wrote: > Should we just add a builtin function pg_checkpoint(), and deprecate > the syntax? That seems reasonable to me. Nathan
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Jeff Davis (pg...@j-davis.com) wrote: > On Tue, 2021-10-26 at 00:07 +, 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/25/21, 6:48 PM, "Jeff Davis" wrote: > On Tue, 2021-10-26 at 00:07 +, 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Tue, 2021-10-26 at 00:07 +, 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/25/21, 4:40 PM, "Jeff Davis" 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
diff --git a/doc/src/sgml/ref/checkpoint.sgml b/doc/src/sgml/ref/checkpoint.sgml
index 2afee6d7b59..aebe8f8cd77 100644
--- a/doc/src/sgml/ref/checkpoint.sgml
+++ b/doc/src/sgml/ref/checkpoint.sgml
@@ -52,7 +52,8 @@ CHECKPOINT
- Only superusers can call CHECKPOINT.
+ Only superusers or members of the pg_checkpointer role
+ can call CHECKPOINT.
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")));
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
(RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3253b8751b1..1bc331a9a39 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 202109101
+#define CATALOG_VERSION_NO 202110231
#endif
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 3da68016b61..9c65174f3c6 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -79,5 +79,10 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '4544', oid_symbol => 'ROLE_PG_CHECKPOINTER',
+ rolname => 'pg_checkpointer', rolsuper => 'f', rolinherit => 't',
+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+ rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+ rolpassword => '_null_', rolvaliduntil => '_null_' },
]
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
> On Oct 24, 2021, at 7:49 AM, 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? > > 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 you that Andrew has requested that I rework that patch set. In particular, he would like me to implement a new system of grants whereby the authority to ALTER SYSTEM SET can be granted per GUC rather than having predefined roles which hardcoded privileges. 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't recommend tying this pg_maintenance idea to that set. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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/
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Jeff Davis (pg...@j-davis.com) wrote: > On Sun, 2021-10-24 at 21:32 +, 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Stephen Frost 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
Greetings, * Bharath Rupireddy (bharath.rupireddyforpostg...@gmail.com) wrote: > On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis 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 signature.asc Description: PGP signature
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/24/21, 11:13 PM, "Jeff Davis" wrote: > On Sun, 2021-10-24 at 21:32 +, 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sun, 2021-10-24 at 21:32 +, 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/24/21, 10:20 AM, "Jeff Davis" 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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
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
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sun, Oct 24, 2021 at 7:49 AM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis 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.
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis 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.
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
From e615ad4b4a8c390a4b937dbb5721de5daad5 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Sat, 23 Oct 2021 13:41:41 -0700
Subject: [PATCH] Predefined role pg_maintenance for VACUUM, ANALYZE,
CHECKPOINT.
---
doc/src/sgml/ref/analyze.sgml | 14 +++---
doc/src/sgml/ref/checkpoint.sgml | 3 ++-
doc/src/sgml/ref/vacuum.sgml | 14 +++---
src/backend/commands/vacuum.c | 12 +++-
src/backend/tcop/utility.c| 5 +++--
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_authid.dat | 5 +
7 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index c8fcebc1612..38e3d8916f0 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -148,13 +148,13 @@ ANALYZE [ VERBOSE ] [ table_and_columnsNotes
- To analyze a table, one must ordinarily be the table's owner or a
- superuser. However, database owners are allowed to
- analyze all tables in their databases, except shared catalogs.
- (The restriction for shared catalogs means that a true database-wide
- ANALYZE can only be performed by a superuser.)
- ANALYZE will skip over any tables that the calling user
- does not have permission to analyze.
+ To analyze a table, one must ordinarily be the table's owner, a superuser,
+ or a member of the pg_maintenance role. However,
+ database owners are allowed to analyze all tables in their databases,
+ except shared catalogs. (The restriction for shared catalogs means that a
+ true database-wide ANALYZE can only be performed by a
+ superuser.) ANALYZE will skip over any tables that the
+ calling user does not have permission to analyze.
diff --git a/doc/src/sgml/ref/checkpoint.sgml b/doc/src/sgml/ref/checkpoint.sgml
index 2afee6d7b59..461ccd603e5 100644
--- a/doc/src/sgml/ref/checkpoint.sgml
+++ b/doc/src/sgml/ref/checkpoint.sgml
@@ -52,7 +52,8 @@ CHECKPOINT
- Only superusers can call CHECKPOINT.
+ Only superusers or members of pg_maintenance can call
+ CHECKPOINT.
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 3df32b58ee6..72d5b56feb5 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -356,13 +356,13 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ relisshared))
return true;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index bf085aa93b2..43607eb84af 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_MAINTENANCE))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to do CHECKPOINT")));
+ errmsg("must be member of pg_maintenance to do CHECKPOINT")));
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
(RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3253b8751b1..1bc331a9a39 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 202109101
+#define CATALOG_VERSION_NO 202110231
#endif
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 3da68016b61..886969dc646 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -79,5 +79,10 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '4544', oid_symbol => 'ROLE_PG_MAINTENANCE',
+ rolname => 'pg_maintenance', rolsuper => 'f', rolinherit => 't',
+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+ rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+ rolpassword => '_null_', rolvaliduntil => '_null_' },
]
--
2.17.1
