Greetings, * Martín Marqués (mar...@2ndquadrant.com) wrote: > > > $ git diff > > > diff --git a/src/backend/catalog/system_views.sql > > > b/src/backend/catalog/system_views.sql > > > index c16061f8f00..97ee72a9cfc 100644 > > > --- a/src/backend/catalog/system_views.sql > > > +++ b/src/backend/catalog/system_views.sql > > > @@ -1494,9 +1494,6 @@ GRANT EXECUTE ON FUNCTION > > > pg_ls_archive_statusdir() TO pg_monitor; > > > GRANT EXECUTE ON FUNCTION pg_ls_tmpdir() TO pg_monitor; > > > GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) TO pg_monitor; > > > > > > -GRANT EXECUTE ON FUNCTION pg_replication_origin_progress(text, > > > boolean) TO pg_monitor; > > > -GRANT EXECUTE ON FUNCTION > > > pg_replication_origin_session_progress(boolean) TO pg_monitor; > > > - > > > GRANT pg_read_all_settings TO pg_monitor; > > > GRANT pg_read_all_stats TO pg_monitor; > > > GRANT pg_stat_scan_tables TO pg_monitor; > > > > > > Agreed on this part. The two functions aren't needed to be granted. > > > > But, pg_show_replication_origin_status() should be allowed > > pg_read_all_stats, not pg_monitor. pg_monitor is just a union role of > > actual privileges. > > I placed that GRANT on purpose to `pg_monitor`, separated from the > `pg_read_all_stats` role, because it doesn't match the description for > that role. > > ``` > Read all pg_stat_* views and use various statistics related > extensions, even those normally visible only to superusers. > ``` > > I have no problem adding it to this ROLE, but we'd have to amend the > doc for default-roles to reflect that SELECT for this view is also > granted to `pg_read_all_stats`.
I agree in general that pg_monitor shouldn't have privileges granted directly to it. If this needs a new default role, that's an option, but it seems like it'd make sense to be part of pg_read_all_stats to me, so amending the docs looks reasonable from here. > > Another issue would be how to control output of > > pg_show_replication_origin_status(). Most of functions that needs > > pg_read_all_stats privileges are filtering sensitive columns in each > > row, instead of hiding the existence of rows. Maybe the view > > pg_replication_origin_status should show only local_id and hide other > > columns from non-pg_read_all_stats users. > > I think that the output from `pg_show_replication_origin_status()` > doesn't expose any data that `pg_read_all_stats` or `pg_monitor` > shouldn't be able to read. Removing or obfuscating `external_id` > and/or `remote_lsn` would make the view somehow pointless, in > particular for monitoring and diagnostic tools. Yeah, pg_read_all_stats is a rather privileged role when it comes to reading data, consider that it can see basically everything in pg_stat_activity, for example. Thanks, Stephen
signature.asc
Description: PGP signature