At Mon, 25 Jul 2022 09:40:49 -0700, Nathan Bossart <nathandboss...@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 owner can vacuum it WARNING: skipping "pg_type" --- only superusers, roles with privileges of pg_vacuum_analyze, or the database owner can vacuum it <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