Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.

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