Re: predefined role(s) for VACUUM and ANALYZE

At Mon, 25 Jul 2022 09:40:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Mon, Jul 25, 2022 at 12:58:36PM +0530, Bharath Rupireddy wrote:
> > Thanks. I'm personally happy with more granular levels of control (as
> > we don't have to give full superuser access to just run a few commands
> > or maintenance operations) for various postgres commands. The only
> > concern is that we might eventually end up with many predefined roles
> > (perhaps one predefined role per command), spreading all around the
> > code base and it might be difficult for the users to digest all of the
> > roles in. It will be great if we can have some sort of rules or
> > methods to define a separate role for a command.
> 
> Yeah, in the future, I could see this growing to a couple dozen predefined
> roles.  Given they are relatively inexpensive and there are already 12 of
> them, I'm personally not too worried about the list becoming too unwieldy.
> Another way to help users might be to create additional aggregate
> predefined roles (like pg_monitor) for common combinations.

I agree to the necessity of that execution control, but I fear a
little how many similar roles will come in future (but it doesn't seem
so much?).  I didn't think so when pg_checkpoint was introdueced,
though.  That being said, since we're going to control
maintenance'ish-command execution via predefined roles so it is fine
in that criteria.

One arguable point would be whether we will need to put restriction
the target relations that Bob can vacuum/analyze. If we need that, the
new predeefined role is not sufficient then need a new syntax for that.

GRANT EXECUTION COMMAND VACUUM ON TABLE rel1 TO bob.
GRANT EXECUTION COMMAND VACUUM ON TABLES OWNED BY alice TO bob.
GRANT EXECUTION COMMAND VACUUM ON ALL TABLES OWNED BY alice TO bob.

However, one problem of these syntaxes is they cannot do something to
future relations.

So, considering that aspect, I would finally agree to the proposal
here. (In short +1 for what the patch does.)


About the patch, it seems fine as the whole except the change in error
messages.

-          (errmsg("skipping \"%s\" --- only superuser can analyze it",
+          (errmsg("skipping \"%s\" --- only superusers and roles with "
+              "privileges of pg_vacuum_analyze can analyze it",

The message looks a bit too verbose or lengty especially when many
relations are rejected.

WARNING:  skipping "pg_statistic" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database
ownercan vacuum it
 
WARNING:  skipping "pg_type" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner can
vacuumit
 
<snip many lines>
WARNING:  skipping "user_mappings" --- only table or database owner can vacuum it
VACUUM

Couldn't we simplify the message?  For example "skipping \"%s\" ---
insufficient priviledges".  We could add a detailed (not a DETAIL:)
message at the end to cover all of the skipped relations, but it may
be too much.


By the way the patch splits an error message into several parts but
that later makes it harder to search for the message in the tree.  *I*
would suggest not splitting message strings.


# I refrain from suggesing removing parens surrounding errmsg() :p

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center