From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Dave Page <dpage(at)pgadmin(dot)org> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Mark Dilger <hornschnorter(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Monitoring roles patch |
Date: | 2017-03-28 18:22:46 |
Message-ID: | 20170328182246.GG9812@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dave,
* Dave Page (dpage(at)pgadmin(dot)org) wrote:
> OK, so before I start hacking again, here's a proposal based on my
> understanding of folks comments, and so open questions. If I can get
> agreement and answers, I'll be able to break out vi again without
> (hopefully) too many more revisions:
>
> pg_read_all_stats: Will have C-coded access to pg_stats views and
> pg_*_size that are currently hard-coded to superuser
Not quite sure what you mean here by 'C-coded access to pg_stats
*views*', but I'm guessing that isn't exactly what you meant since I'm
sure we aren't going to change how view permissions are done in this
patch. I take it you mean access to the functions under the views? If
so, I believe this is correct.
> pg_read_all_settings: Will have C-coded access to read GUCs that are
> currently hard-coded to the superuser
Right.
> pg_monitor: Will include pg_read_all_stats and pg_read_all_settings,
> and all explicitly GRANTable rights, e.g. in contrib modules.
Right.
> Patch to be rebased on Simon's updated version.
Right.
> Questions:
>
> - pg_stat_statements has a hard-coded superuser check. Shall I remove
> that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor?
pg_stat_statements shouldn't have ever had that superuser check and it
shouldn't have ever used '==' for the user check, it should have been
using 'has_privs_of_role()' from the start, which means that the
superuser check isn't necessary.
I don't think we should remove that check, nor should we REVOKE EXECUTE
from public for the function. We *should* add a hard-coded role check
to allow another role which isn't a member of the role whose query it is
to view the query. That shouldn't be pg_monitor, of course (as
discussed). I don't think pg_read_all_stats or pg_read_all_settings
really covers this case either- this is more like pg_read_all_queries
and should also be used for pg_stat_activity.
That would then be granted to pg_monitor.
> - pgrowlocks has hard-coded access to superuser and users with SELECT
> on the table being examined. How should this be handled?
I don't see any hard-coded superuser check? There is a
pg_class_aclcheck() for SELECT rights on the table. I like the idea of
having another way to grant access to run this function on a table
besides giving SELECT rights on the entire table to the user. This
would fall under the mandate of the role described in your next bullet,
in my view.
> - Stephen suggested a separate role for functions that can lock
> tables. Is this still desired, or shall we just grant access to
> pg_monitor (I think the latter is fine)?
Right, I was thinking something like pg_stat_all_tables or
pg_stat_scan_tables or similar. We would add that (perhaps also with a
SELECT check like pgrowlocks has) for the other functions like
pgstattuple and pg_freespacemap and pg_visibility.
> - Based on Peter's concerns, is pg_read_all_stats the right name?
> Maybe pg_read_monitoring_stats?
I'd rather not get too wrapped up in the naming.. The documentation is
the important part here, imv.
Thanks!
Stephen
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2017-03-28 18:29:09 | Re: Monitoring roles patch |
Previous Message | Jim Nasby | 2017-03-28 18:15:06 | Missing increment of vacrelstats->pinskipped_pages |