Hi

On Thu, Mar 23, 2017 at 12:23 PM, Stephen Frost <sfr...@snowman.net> wrote:
>
>> diff --git a/contrib/pg_stat_statements/pg_stat_statements.c 
>> b/contrib/pg_stat_statements/pg_stat_statements.c
>> index 221ac98d4a..cec94d5896 100644
>> --- a/contrib/pg_stat_statements/pg_stat_statements.c
>> +++ b/contrib/pg_stat_statements/pg_stat_statements.c
>> @@ -1386,7 +1387,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
>>       MemoryContext per_query_ctx;
>>       MemoryContext oldcontext;
>>       Oid                     userid = GetUserId();
>> -     bool            is_superuser = superuser();
>> +     bool            is_superuser = false;
>>       char       *qbuffer = NULL;
>>       Size            qbuffer_size = 0;
>>       Size            extent = 0;
>> @@ -1394,6 +1395,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
>>       HASH_SEQ_STATUS hash_seq;
>>       pgssEntry  *entry;
>>
>> +     /* For the purposes of this module, we consider pg_read_all_stats 
>> members to be superusers */
>> +     is_superuser = (superuser() || is_member_of_role(GetUserId(), 
>> DEFAULT_ROLE_READ_ALL_STATS));
>
> Whoahhhh, let's *not* set an 'is_superuser' flag for a case where the
> user is not, actually, a superuser.  This should be 'allowed_role' or
> something along those lines, I'm thinking.

Fixed.

>> diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
>> index bc42944426..21d787ddf7 100644
>> --- a/contrib/pg_visibility/Makefile
>> +++ b/contrib/pg_visibility/Makefile
>> @@ -4,7 +4,8 @@ MODULE_big = pg_visibility
>>  OBJS = pg_visibility.o $(WIN32RES)
>>
>>  EXTENSION = pg_visibility
>> -DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
>> +DATA = pg_visibility--1.1.sql pg_visibility--1.1--1.2.sql \
>> +     pg_visibility--1.0--1.1.sql
>>  PGFILEDESC = "pg_visibility - page visibility information"
>>
>>  REGRESS = pg_visibility
>
> Missing file here also.

Yeah, I forgot to add this and the others. Sorry about - fixed now.

>> diff --git a/contrib/pgrowlocks/pgrowlocks.c 
>> b/contrib/pgrowlocks/pgrowlocks.c
>> index db9e0349a0..c9194d8c0d 100644
>> --- a/contrib/pgrowlocks/pgrowlocks.c
>> +++ b/contrib/pgrowlocks/pgrowlocks.c
>> @@ -28,6 +28,7 @@
>>  #include "access/relscan.h"
>>  #include "access/xact.h"
>>  #include "catalog/namespace.h"
>> +#include "catalog/pg_authid.h"
>>  #include "funcapi.h"
>>  #include "miscadmin.h"
>>  #include "storage/bufmgr.h"
>> @@ -98,9 +99,11 @@ pgrowlocks(PG_FUNCTION_ARGS)
>>               relrv = 
>> makeRangeVarFromNameList(textToQualifiedNameList(relname));
>>               rel = heap_openrv(relrv, AccessShareLock);
>>
>> -             /* check permissions: must have SELECT on table */
>> -             aclresult = pg_class_aclcheck(RelationGetRelid(rel), 
>> GetUserId(),
>> -                                                                       
>> ACL_SELECT);
>> +             /* check permissions: must have SELECT on table or be in 
>> pg_read_all_stats */
>> +             aclresult = (pg_class_aclcheck(RelationGetRelid(rel), 
>> GetUserId(),
>> +                                                                       
>> ACL_SELECT) ||
>> +                     is_member_of_role(GetUserId(), 
>> DEFAULT_ROLE_READ_ALL_STATS));
>> +
>>               if (aclresult != ACLCHECK_OK)
>>                       aclcheck_error(aclresult, ACL_KIND_CLASS,
>>                                                  
>> RelationGetRelationName(rel));
>
> Doesn't this go beyond just pg_stat_X, but actually allows running
> pgrowlocks against any relation?  That seems like it should be
> independent, though it actually fits the "global-read-metadata" role
> case that has been discussed previously as being what pg_visibility,
> pgstattuple, and other similar contrib modules need.  I don't have a
> great name for that role, but it seems different to me from
> 'pg_read_all_stats', though I don't hold that position very strongly as
> I can see an argument that these kind of are stats.

That was the way I was leaning, on the grounds that the distinction
between two separate roles might end up being confusing for users.
It's simple enough to change if that's the consencus, and we can come
up with a suitable name.

> I see you did change pgstattuple too, and perhaps these are the changes
> you intended for pg_visibility too?  In any case, if so, we need to make
> it clear in the docs that pg_read_all_stats grants access to more than
> just pg_stat_X.

The pg_read_all_stats role docs have:

Read all pg_stat_* views and use various statistics related
extensions, even those normally visible only to superusers.

And then I updated the docs for each contrib module to note where
pg_read_all_stats can use functionality.

>> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
>> index df0435c3f0..5472ff76a7 100644
>> --- a/doc/src/sgml/catalogs.sgml
>> +++ b/doc/src/sgml/catalogs.sgml
>> @@ -10027,15 +10027,17 @@ SELECT * FROM pg_locks pl LEFT JOIN 
>> pg_prepared_xacts ppx
>>        <entry><type>text</type></entry>
>>        <entry>Configuration file the current value was set in (null for
>>        values set from sources other than configuration files, or when
>> -      examined by a non-superuser);
>> -      helpful when using <literal>include</> directives in configuration 
>> files</entry>
>> +      examined by a user who is neither a superuser or a member of
>> +      <literal>pg_read_all_settings</literal>); helpful when using
>> +      <literal>include</> directives in configuration files</entry>
>>       </row>
>>       <row>
>>        <entry><structfield>sourceline</structfield></entry>
>>        <entry><type>integer</type></entry>
>>        <entry>Line number within the configuration file the current value was
>>        set at (null for values set from sources other than configuration 
>> files,
>> -      or when examined by a non-superuser)
>> +      or when examined by a user who is neither a superuser or a member of
>> +      <literal>pg_read_all_settings</literal>).
>>        </entry>
>>       </row>
>>       <row>
>
> We should really figure out a way to link back to the default roles
> section when we refer to roles from other places.  Not sure what the
> best way to do that off-hand is though.

Yeah, I have no idea about that.

>> diff --git a/doc/src/sgml/pgbuffercache.sgml 
>> b/doc/src/sgml/pgbuffercache.sgml
>> index b261a4dbe0..9d1faefdd6 100644
>> --- a/doc/src/sgml/pgbuffercache.sgml
>> +++ b/doc/src/sgml/pgbuffercache.sgml
>> @@ -24,8 +24,9 @@
>>   </para>
>>
>>   <para>
>> -  By default public access is revoked from both of these, just in case there
>> -  are security issues lurking.
>> +  By default use is restricted to superusers and members of the
>> +  <literal>pg_read_all_stats</literal> role, just in case there are security
>> +  issues lurking.
>>   </para>
>>
>>   <sect2>
>> diff --git a/doc/src/sgml/pgfreespacemap.sgml 
>> b/doc/src/sgml/pgfreespacemap.sgml
>> index f2f99d571e..f4f0e08473 100644
>> --- a/doc/src/sgml/pgfreespacemap.sgml
>> +++ b/doc/src/sgml/pgfreespacemap.sgml
>> @@ -16,8 +16,9 @@
>>   </para>
>>
>>   <para>
>> -  By default public access is revoked from the functions, just in case
>> -  there are security issues lurking.
>> +  By default use is restricted to superusers and members of the
>> +  <literal>pg_read_all_stats</literal> role, just in case there are security
>> +  issues lurking.
>>   </para>
>>
>>   <sect2>
>
> These also look like cases where we might want to think about if we
> should have a dedicated role which, essentially, allows locking and
> scanning of all relations.
>
> In particular, I can certainly imagine admins who wish to GRANT out the
> rights to view other queries in pg_stat_activity to another admin and
> maybe even allow them to run pg_terminate_backend, or pg_cancel_query,
> but not allow them to scan every relation in the database.

Easy enough to add - thoughts on the name though? I'm struggling to
come up with anything sane.

>> diff --git a/src/backend/replication/walreceiver.c 
>> b/src/backend/replication/walreceiver.c
>> index 31c567b37e..8e5c8c507c 100644
>> --- a/src/backend/replication/walreceiver.c
>> +++ b/src/backend/replication/walreceiver.c
>> @@ -50,6 +50,7 @@
>>  #include "access/timeline.h"
>>  #include "access/transam.h"
>>  #include "access/xlog_internal.h"
>> +#include "catalog/pg_authid.h"
>>  #include "catalog/pg_type.h"
>>  #include "funcapi.h"
>>  #include "libpq/pqformat.h"
>> @@ -1396,7 +1397,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
>>       /* Fetch values */
>>       values[0] = Int32GetDatum(walrcv->pid);
>>
>> -     if (!superuser())
>> +     if (!superuser() && !is_member_of_role(GetUserId(), 
>> DEFAULT_ROLE_MONITOR))
>>       {
>>               /*
>>                * Only superusers can see details. Other users only get the 
>> pid value
>
> Shouldn't that be DEFAULT_ROLE_READ_ALL_STATS, not MONITOR?

Ooops, fixed.

>> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
>> index 4feb26aa7a..d1da580c9c 100644
>> --- a/src/backend/utils/misc/guc.c
>> +++ b/src/backend/utils/misc/guc.c
>> @@ -34,6 +34,7 @@
>>  #include "access/xact.h"
>>  #include "access/xlog_internal.h"
>>  #include "catalog/namespace.h"
>> +#include "catalog/pg_authid.h"
>>  #include "commands/async.h"
>>  #include "commands/prepare.h"
>>  #include "commands/user.h"
>> @@ -6677,10 +6678,12 @@ GetConfigOption(const char *name, bool missing_ok, 
>> bool restrict_superuser)
>>       }
>>       if (restrict_superuser &&
>>               (record->flags & GUC_SUPERUSER_ONLY) &&
>> -             !superuser())
>> +             !superuser() &&
>> +             !is_member_of_role(GetUserId(), 
>> DEFAULT_ROLE_READ_ALL_SETTINGS))
>
> Seems like having a variable to indicate if the caller should be able to
> read all these settings or not might be nice to have, so we have one
> place that figures out the privileges question.

Those checks are not all identical, and they're in different functions
so a variable wouldn't work (I assume you're not suggesting I add a
global :-p ). I could push part of each check out into a separate
function, but it doesn't seem like it would really add to the
readability to me.

> A few comments where that variable is set wouldn't be bad either.
>
> Also, I'm thinking the pg_monitor documentation really needs to be
> expanded and made very clear.  I'll think a bit more on just how to try
> and phrase that, but the description included certainly seemed
> inadequate.

I've added some additional text below the table.

> That's it for a quick review.  If we can get these changes done and
> there aren't any more questions or concerns, I'm happy to continue
> working to move this forward.  I definitely see the usefulness of these
> changes and it'd be great to have an answer to the oft-asked question of
> how to do proper monitoring with a non-superuser role.

Thanks - updated patch attached.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/pg_buffercache/Makefile b/contrib/pg_buffercache/Makefile
index 497dbeb229..18f7a87452 100644
--- a/contrib/pg_buffercache/Makefile
+++ b/contrib/pg_buffercache/Makefile
@@ -4,8 +4,9 @@ MODULE_big = pg_buffercache
 OBJS = pg_buffercache_pages.o $(WIN32RES)
 
 EXTENSION = pg_buffercache
-DATA = pg_buffercache--1.2.sql pg_buffercache--1.1--1.2.sql \
-       pg_buffercache--1.0--1.1.sql pg_buffercache--unpackaged--1.0.sql
+DATA = pg_buffercache--1.2.sql pg_buffercache--1.2--1.3.sql \
+       pg_buffercache--1.1--1.2.sql pg_buffercache--1.0--1.1.sql \
+       pg_buffercache--unpackaged--1.0.sql
 PGFILEDESC = "pg_buffercache - monitoring of shared buffer cache in real-time"
 
 ifdef USE_PGXS
diff --git a/contrib/pg_buffercache/pg_buffercache.control 
b/contrib/pg_buffercache/pg_buffercache.control
index a4d664f3fa..8c060ae9ab 100644
--- a/contrib/pg_buffercache/pg_buffercache.control
+++ b/contrib/pg_buffercache/pg_buffercache.control
@@ -1,5 +1,5 @@
 # pg_buffercache extension
 comment = 'examine the shared buffer cache'
-default_version = '1.2'
+default_version = '1.3'
 module_pathname = '$libdir/pg_buffercache'
 relocatable = true
diff --git a/contrib/pg_freespacemap/Makefile b/contrib/pg_freespacemap/Makefile
index 7bc0e9555d..0a2f000ec6 100644
--- a/contrib/pg_freespacemap/Makefile
+++ b/contrib/pg_freespacemap/Makefile
@@ -4,8 +4,8 @@ MODULE_big = pg_freespacemap
 OBJS = pg_freespacemap.o $(WIN32RES)
 
 EXTENSION = pg_freespacemap
-DATA = pg_freespacemap--1.1.sql pg_freespacemap--1.0--1.1.sql \
-       pg_freespacemap--unpackaged--1.0.sql
+DATA = pg_freespacemap--1.1.sql pg_freespacemap--1.1--1.2.sql \
+       pg_freespacemap--1.0--1.1.sql pg_freespacemap--unpackaged--1.0.sql
 PGFILEDESC = "pg_freespacemap - monitoring of free space map"
 
 ifdef USE_PGXS
diff --git a/contrib/pg_freespacemap/pg_freespacemap.control 
b/contrib/pg_freespacemap/pg_freespacemap.control
index 764db30d18..ac8fc5050a 100644
--- a/contrib/pg_freespacemap/pg_freespacemap.control
+++ b/contrib/pg_freespacemap/pg_freespacemap.control
@@ -1,5 +1,5 @@
 # pg_freespacemap extension
 comment = 'examine the free space map (FSM)'
-default_version = '1.1'
+default_version = '1.2'
 module_pathname = '$libdir/pg_freespacemap'
 relocatable = true
diff --git a/contrib/pg_stat_statements/Makefile 
b/contrib/pg_stat_statements/Makefile
index 298951a5f5..39b368b70e 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,9 +4,10 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.3--1.4.sql \
-       pg_stat_statements--1.2--1.3.sql pg_stat_statements--1.1--1.2.sql \
-       pg_stat_statements--1.0--1.1.sql pg_stat_statements--unpackaged--1.0.sql
+DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+       pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
+       pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
+       pg_stat_statements--unpackaged--1.0.sql
 PGFILEDESC = "pg_stat_statements - execution statistics of SQL statements"
 
 LDFLAGS_SL += $(filter -lm, $(LIBS))
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c 
b/contrib/pg_stat_statements/pg_stat_statements.c
index 221ac98d4a..3d2dabdce4 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -62,6 +62,7 @@
 #include <unistd.h>
 
 #include "access/hash.h"
+#include "catalog/pg_authid.h"
 #include "executor/instrument.h"
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
@@ -1386,7 +1387,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
        MemoryContext per_query_ctx;
        MemoryContext oldcontext;
        Oid                     userid = GetUserId();
-       bool            is_superuser = superuser();
+       bool            is_allowed_role = false;
        char       *qbuffer = NULL;
        Size            qbuffer_size = 0;
        Size            extent = 0;
@@ -1394,6 +1395,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
        HASH_SEQ_STATUS hash_seq;
        pgssEntry  *entry;
 
+       /* For the purposes of this module, we consider pg_read_all_stats 
members to be superusers */
+       is_allowed_role = (superuser() || is_member_of_role(GetUserId(), 
DEFAULT_ROLE_READ_ALL_STATS));
+
        /* hash table must exist already */
        if (!pgss || !pgss_hash)
                ereport(ERROR,
@@ -1536,7 +1540,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
                values[i++] = ObjectIdGetDatum(entry->key.userid);
                values[i++] = ObjectIdGetDatum(entry->key.dbid);
 
-               if (is_superuser || entry->key.userid == userid)
+               if (is_allowed_role || entry->key.userid == userid)
                {
                        if (api_version >= PGSS_V1_2)
                                values[i++] = Int64GetDatumFast(queryid);
diff --git a/contrib/pg_stat_statements/pg_stat_statements.control 
b/contrib/pg_stat_statements/pg_stat_statements.control
index 24038f56b1..193fcdfafa 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.control
+++ b/contrib/pg_stat_statements/pg_stat_statements.control
@@ -1,5 +1,5 @@
 # pg_stat_statements extension
 comment = 'track execution statistics of all SQL statements executed'
-default_version = '1.4'
+default_version = '1.5'
 module_pathname = '$libdir/pg_stat_statements'
 relocatable = true
diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
index bc42944426..21d787ddf7 100644
--- a/contrib/pg_visibility/Makefile
+++ b/contrib/pg_visibility/Makefile
@@ -4,7 +4,8 @@ MODULE_big = pg_visibility
 OBJS = pg_visibility.o $(WIN32RES)
 
 EXTENSION = pg_visibility
-DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
+DATA = pg_visibility--1.1.sql pg_visibility--1.1--1.2.sql \
+       pg_visibility--1.0--1.1.sql
 PGFILEDESC = "pg_visibility - page visibility information"
 
 REGRESS = pg_visibility
diff --git a/contrib/pg_visibility/pg_visibility.control 
b/contrib/pg_visibility/pg_visibility.control
index f93ed0176e..3cffa08b01 100644
--- a/contrib/pg_visibility/pg_visibility.control
+++ b/contrib/pg_visibility/pg_visibility.control
@@ -1,5 +1,5 @@
 # pg_visibility extension
 comment = 'examine the visibility map (VM) and page-level visibility info'
-default_version = '1.1'
+default_version = '1.2'
 module_pathname = '$libdir/pg_visibility'
 relocatable = true
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index db9e0349a0..c9194d8c0d 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -28,6 +28,7 @@
 #include "access/relscan.h"
 #include "access/xact.h"
 #include "catalog/namespace.h"
+#include "catalog/pg_authid.h"
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
@@ -98,9 +99,11 @@ pgrowlocks(PG_FUNCTION_ARGS)
                relrv = 
makeRangeVarFromNameList(textToQualifiedNameList(relname));
                rel = heap_openrv(relrv, AccessShareLock);
 
-               /* check permissions: must have SELECT on table */
-               aclresult = pg_class_aclcheck(RelationGetRelid(rel), 
GetUserId(),
-                                                                         
ACL_SELECT);
+               /* check permissions: must have SELECT on table or be in 
pg_read_all_stats */
+               aclresult = (pg_class_aclcheck(RelationGetRelid(rel), 
GetUserId(),
+                                                                         
ACL_SELECT) ||
+                       is_member_of_role(GetUserId(), 
DEFAULT_ROLE_READ_ALL_STATS));
+
                if (aclresult != ACLCHECK_OK)
                        aclcheck_error(aclresult, ACL_KIND_CLASS,
                                                   
RelationGetRelationName(rel));
diff --git a/contrib/pgstattuple/pgstattuple--1.4--1.5.sql 
b/contrib/pgstattuple/pgstattuple--1.4--1.5.sql
index 84e112e1c2..a6143fcafe 100644
--- a/contrib/pgstattuple/pgstattuple--1.4--1.5.sql
+++ b/contrib/pgstattuple/pgstattuple--1.4--1.5.sql
@@ -17,6 +17,7 @@ AS 'MODULE_PATHNAME', 'pgstattuple_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pgstattuple(text) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pgstattuple(text) TO pg_read_all_stats;
 
 CREATE OR REPLACE FUNCTION pgstatindex(IN relname text,
     OUT version INT,
@@ -33,6 +34,7 @@ AS 'MODULE_PATHNAME', 'pgstatindex_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pgstatindex(text) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pgstatindex(text) TO pg_read_all_stats;
 
 CREATE OR REPLACE FUNCTION pg_relpages(IN relname text)
 RETURNS BIGINT
@@ -40,6 +42,7 @@ AS 'MODULE_PATHNAME', 'pg_relpages_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pg_relpages(text) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pg_relpages(text) TO pg_read_all_stats;
 
 /* New stuff in 1.1 begins here */
 
@@ -51,6 +54,7 @@ AS 'MODULE_PATHNAME', 'pgstatginindex_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pgstatginindex(regclass) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pgstatginindex(regclass) TO pg_read_all_stats;
 
 /* New stuff in 1.2 begins here */
 
@@ -68,6 +72,7 @@ AS 'MODULE_PATHNAME', 'pgstattuplebyid_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pgstattuple(regclass) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pgstattuple(regclass) TO pg_read_all_stats;
 
 CREATE OR REPLACE FUNCTION pgstatindex(IN relname regclass,
     OUT version INT,
@@ -84,6 +89,7 @@ AS 'MODULE_PATHNAME', 'pgstatindexbyid_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pgstatindex(regclass) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pgstatindex(regclass) TO pg_read_all_stats;
 
 CREATE OR REPLACE FUNCTION pg_relpages(IN relname regclass)
 RETURNS BIGINT
@@ -91,6 +97,7 @@ AS 'MODULE_PATHNAME', 'pg_relpagesbyid_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pg_relpages(regclass) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pg_relpages(regclass) TO pg_read_all_stats;
 
 /* New stuff in 1.3 begins here */
 
@@ -109,6 +116,7 @@ AS 'MODULE_PATHNAME', 'pgstattuple_approx_v1_5'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pgstattuple_approx(regclass) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pgstattuple_approx(regclass) TO pg_read_all_stats;
 
 /* New stuff in 1.5 begins here */
 
@@ -125,3 +133,4 @@ AS 'MODULE_PATHNAME', 'pgstathashindex'
 LANGUAGE C STRICT PARALLEL SAFE;
 
 REVOKE EXECUTE ON FUNCTION pgstathashindex(regclass) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pgstathashindex(regclass) TO pg_read_all_stats;
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index df0435c3f0..5472ff76a7 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10027,15 +10027,17 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts 
ppx
       <entry><type>text</type></entry>
       <entry>Configuration file the current value was set in (null for
       values set from sources other than configuration files, or when
-      examined by a non-superuser);
-      helpful when using <literal>include</> directives in configuration 
files</entry>
+      examined by a user who is neither a superuser or a member of
+      <literal>pg_read_all_settings</literal>); helpful when using
+      <literal>include</> directives in configuration files</entry>
      </row>
      <row>
       <entry><structfield>sourceline</structfield></entry>
       <entry><type>integer</type></entry>
       <entry>Line number within the configuration file the current value was
       set at (null for values set from sources other than configuration files,
-      or when examined by a non-superuser)
+      or when examined by a user who is neither a superuser or a member of
+      <literal>pg_read_all_settings</literal>).
       </entry>
      </row>
      <row>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4dc30caccb..b32d1ce17c 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19370,9 +19370,11 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
     accept the OID or name of a database or tablespace, and return the total
     disk space used therein.  To use <function>pg_database_size</function>,
     you must have <literal>CONNECT</> permission on the specified database
-    (which is granted by default).  To use <function>pg_tablespace_size</>,
-    you must have <literal>CREATE</> permission on the specified tablespace,
-    unless it is the default tablespace for the current database.
+    (which is granted by default), or be a member of the 
<literal>pg_read_all_stats</>
+    role. To use <function>pg_tablespace_size</>, you must have
+    <literal>CREATE</> permission on the specified tablespace, or be a member
+    of the <literal>pg_read_all_stats</> role unless it is the default 
tablespace for
+    the current database.
    </para>
 
    <para>
@@ -19681,7 +19683,8 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
        <entry><type>setof record</type></entry>
        <entry>
         List the name, size, and last modification time of files in the log
-        directory.  Access may be granted to non-superuser roles.
+        directory. Access is granted to members of the <literal>pg_monitor</>
+        role and may be granted to other non-superuser roles.
        </entry>
       </row>
       <row>
@@ -19691,7 +19694,8 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
        <entry><type>setof record</type></entry>
        <entry>
         List the name, size, and last modification time of files in the WAL
-        directory.  Access may be granted to non-superuser roles.
+        directory.Access is granted to members of the <literal>pg_monitor</>
+        role and may be granted to other non-superuser roles.
        </entry>
       </row>
       <row>
@@ -19752,8 +19756,8 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
    <para>
     <function>pg_ls_logdir</> returns the name, size, and last modified time
     (mtime) of each file in the log directory. By default, only superusers
-    can use this function, but access may be granted to others using
-    <command>GRANT</command>.
+    and members of the <literal>pg_monitor</> role can use this function, but
+    access may be granted to others using <command>GRANT</command>.
    </para>
 
    <indexterm>
@@ -19762,8 +19766,9 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
    <para>
     <function>pg_ls_waldir</> returns the name, size, and last modified time
     (mtime) of each file in the write ahead log (WAL) directory. By
-    default only superusers can use this function, but access may be granted
-    to others using <command>GRANT</command>.
+    default only superusers and members of the <literal>pg_monitor</> role
+    can use this function, but access may be granted to others using
+    <command>GRANT</command>.
    </para>
 
    <indexterm>
diff --git a/doc/src/sgml/pgbuffercache.sgml b/doc/src/sgml/pgbuffercache.sgml
index b261a4dbe0..9d1faefdd6 100644
--- a/doc/src/sgml/pgbuffercache.sgml
+++ b/doc/src/sgml/pgbuffercache.sgml
@@ -24,8 +24,9 @@
  </para>
 
  <para>
-  By default public access is revoked from both of these, just in case there
-  are security issues lurking.
+  By default use is restricted to superusers and members of the
+  <literal>pg_read_all_stats</literal> role, just in case there are security
+  issues lurking.
  </para>
 
  <sect2>
diff --git a/doc/src/sgml/pgfreespacemap.sgml b/doc/src/sgml/pgfreespacemap.sgml
index f2f99d571e..f4f0e08473 100644
--- a/doc/src/sgml/pgfreespacemap.sgml
+++ b/doc/src/sgml/pgfreespacemap.sgml
@@ -16,8 +16,9 @@
  </para>
 
  <para>
-  By default public access is revoked from the functions, just in case
-  there are security issues lurking.
+  By default use is restricted to superusers and members of the
+  <literal>pg_read_all_stats</literal> role, just in case there are security
+  issues lurking.
  </para>
 
  <sect2>
diff --git a/doc/src/sgml/pgrowlocks.sgml b/doc/src/sgml/pgrowlocks.sgml
index d73511579c..5f167ebd6a 100644
--- a/doc/src/sgml/pgrowlocks.sgml
+++ b/doc/src/sgml/pgrowlocks.sgml
@@ -12,6 +12,13 @@
   locking information for a specified table.
  </para>
 
+ <para>
+  By default use is restricted to superusers, members of the
+  <literal>pg_read_all_stats</literal> role, and users with
+  <literal>SELECT</literal> permissions on the table.
+ </para>
+
+
  <sect2>
   <title>Overview</title>
 
diff --git a/doc/src/sgml/pgstatstatements.sgml 
b/doc/src/sgml/pgstatstatements.sgml
index d4f09fc2a3..694f3db417 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -226,10 +226,11 @@
   </table>
 
   <para>
-   For security reasons, non-superusers are not allowed to see the SQL
-   text or <structfield>queryid</structfield> of queries executed by other 
users.
-   They can see the statistics, however, if the view has been installed in 
their
-   database.
+   For security reasons, only superusers and members of the
+   <literal>pg_read_all_stats<literal> role are allowed to see the SQL text and
+   <structfield>queryid</structfield> of queries executed by other users.
+   Other users can see the statistics, however, if the view has been installed
+   in their database.
   </para>
 
   <para>
diff --git a/doc/src/sgml/pgstattuple.sgml b/doc/src/sgml/pgstattuple.sgml
index 62b1a6f479..abd4214859 100644
--- a/doc/src/sgml/pgstattuple.sgml
+++ b/doc/src/sgml/pgstattuple.sgml
@@ -16,7 +16,8 @@
   As these functions return detailed page-level information, only the superuser
   has EXECUTE privileges on them upon installation.  After the functions have
   been installed, users may issue <command>GRANT</command> commands to change
-  the privileges on the functions to allow non-superusers to execute them.  See
+  the privileges on the functions to allow non-superusers to execute them. 
Members
+  of the <literal>pg_read_all_stats</literal> role are granted access by 
default. See
   the description of the <xref linkend="sql-grant"> command for specifics.
  </para>
 
diff --git a/doc/src/sgml/pgvisibility.sgml b/doc/src/sgml/pgvisibility.sgml
index fd486696fc..d7b0a448f3 100644
--- a/doc/src/sgml/pgvisibility.sgml
+++ b/doc/src/sgml/pgvisibility.sgml
@@ -140,7 +140,10 @@
   </variablelist>
 
   <para>
-   By default, these functions are executable only by superusers.
+   By default, these functions are executable only by superusers and members 
of the
+   <literal>pg_read_all_stats</literal> role, with the exception of
+   <function>pg_truncate_visibility_map(relation regclass)</function> which 
can only
+   be executed by superusers.
   </para>
  </sect2>
 
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 7eaefe58c2..db2b2f4592 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -516,6 +516,22 @@ DROP ROLE doomed_role;
      </thead>
      <tbody>
       <row>
+       <entry>pg_monitor</entry>
+       <entry>Read/execute various monitoring views and functions.
+       This role is a member of <literal>pg_read_all_settings</literal> and
+       <literal>pg_read_all_stats</literal>.</entry>
+      </row>
+      <row>
+       <entry>pg_read_all_settings</entry>
+       <entry>Read all configuration variables, even those normally visible 
only to
+       superusers.</entry>
+      </row>
+      <row>
+       <entry>pg_read_all_stats</entry>
+       <entry>Read all pg_stat_* views and use various statistics related 
extensions,
+       even those normally visible only to superusers.</entry>
+      </row>
+      <row>
        <entry>pg_signal_backend</entry>
        <entry>Send signals to other backends (eg: cancel query, 
terminate).</entry>
       </row>
@@ -524,6 +540,20 @@ DROP ROLE doomed_role;
    </table>
 
   <para>
+  The <literal>pg_monitor</literal>, <literal>pg_read_all_settings</literal> 
and
+  <literal>pg_read_all_stats</literal> roles are intended to allow 
administrators
+  to easily configure a role for the purpose of monitoring the database server
+  by granting a set of common privileges allowing the role to read various 
useful
+  configuration settings, statistics and other system information normally 
restricted
+  to superusers.
+  </para>
+
+  <para>
+  Care should be taken when granting these roles to ensure they are only used 
where
+  needed to perform the desired monitoring.
+  </para>
+
+  <para>
    Administrators can grant access to these roles to users using the GRANT
    command:
 
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index c2b0bedc1d..d6ca10f2b1 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1111,3 +1111,8 @@ REVOKE EXECUTE ON FUNCTION 
pg_stat_reset_single_function_counters(oid) FROM publ
 
 REVOKE EXECUTE ON FUNCTION pg_ls_logdir() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_ls_waldir() FROM public;
+GRANT EXECUTE ON FUNCTION pg_ls_logdir() TO pg_monitor;
+GRANT EXECUTE ON FUNCTION pg_ls_waldir() TO pg_monitor;
+
+GRANT pg_read_all_settings TO pg_monitor;
+GRANT pg_read_all_stats TO pg_monitor;
diff --git a/src/backend/replication/walreceiver.c 
b/src/backend/replication/walreceiver.c
index 31c567b37e..a61c565593 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -50,6 +50,7 @@
 #include "access/timeline.h"
 #include "access/transam.h"
 #include "access/xlog_internal.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "libpq/pqformat.h"
@@ -1396,7 +1397,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
        /* Fetch values */
        values[0] = Int32GetDatum(walrcv->pid);
 
-       if (!superuser())
+       if (!superuser() && !is_member_of_role(GetUserId(), 
DEFAULT_ROLE_READ_ALL_STATS))
        {
                /*
                 * Only superusers can see details. Other users only get the 
pid value
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 58923912eb..6d56638208 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -17,6 +17,7 @@
 #include "access/htup_details.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_tablespace.h"
 #include "commands/dbcommands.h"
 #include "commands/tablespace.h"
@@ -88,11 +89,17 @@ calculate_database_size(Oid dbOid)
        char            pathname[MAXPGPATH];
        AclResult       aclresult;
 
-       /* User must have connect privilege for target database */
+       /*
+        * User must have connect privilege for target database
+        * or be a member of pg_read_all_stats
+        */
        aclresult = pg_database_aclcheck(dbOid, GetUserId(), ACL_CONNECT);
-       if (aclresult != ACLCHECK_OK)
+       if (aclresult != ACLCHECK_OK &&
+               !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS))
+       {
                aclcheck_error(aclresult, ACL_KIND_DATABASE,
                                           get_database_name(dbOid));
+       }
 
        /* Shared storage in pg_global is not counted */
 
@@ -172,11 +179,12 @@ calculate_tablespace_size(Oid tblspcOid)
        AclResult       aclresult;
 
        /*
-        * User must have CREATE privilege for target tablespace, either
-        * explicitly granted or implicitly because it is default for current
-        * database.
+        * User must be a member of pg_read_all_stats or have CREATE privilege 
for
+        * target tablespace, either explicitly granted or implicitly because
+        * it is default for current database.
         */
-       if (tblspcOid != MyDatabaseTableSpace)
+       if (tblspcOid != MyDatabaseTableSpace &&
+               !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS))
        {
                aclresult = pg_tablespace_aclcheck(tblspcOid, GetUserId(), 
ACL_CREATE);
                if (aclresult != ACLCHECK_OK)
diff --git a/src/backend/utils/adt/pgstatfuncs.c 
b/src/backend/utils/adt/pgstatfuncs.c
index a987d0d621..6bec301450 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "access/htup_details.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_type.h"
 #include "common/ip.h"
 #include "funcapi.h"
@@ -648,8 +649,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
                        nulls[18] = nulls[19] = nulls[20] = nulls[21] = 
nulls[22] = true;
                }
 
-               /* Values only available to role member */
-               if (has_privs_of_role(GetUserId(), beentry->st_userid))
+               /* Values only available to role member or pg_read_all_stats */
+               if (has_privs_of_role(GetUserId(), beentry->st_userid) ||
+                       is_member_of_role(GetUserId(), 
DEFAULT_ROLE_READ_ALL_STATS))
                {
                        SockAddr        zero_clientaddr;
 
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 4feb26aa7a..d1da580c9c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -34,6 +34,7 @@
 #include "access/xact.h"
 #include "access/xlog_internal.h"
 #include "catalog/namespace.h"
+#include "catalog/pg_authid.h"
 #include "commands/async.h"
 #include "commands/prepare.h"
 #include "commands/user.h"
@@ -6677,10 +6678,12 @@ GetConfigOption(const char *name, bool missing_ok, bool 
restrict_superuser)
        }
        if (restrict_superuser &&
                (record->flags & GUC_SUPERUSER_ONLY) &&
-               !superuser())
+               !superuser() &&
+               !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))
                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                                errmsg("must be superuser to examine \"%s\"", 
name)));
+                                errmsg("must be superuser or a member of 
pg_read_all_settings to examine \"%s\"",
+                                name)));
 
        switch (record->vartype)
        {
@@ -6725,10 +6728,13 @@ GetConfigOptionResetString(const char *name)
                ereport(ERROR,
                                (errcode(ERRCODE_UNDEFINED_OBJECT),
                           errmsg("unrecognized configuration parameter 
\"%s\"", name)));
-       if ((record->flags & GUC_SUPERUSER_ONLY) && !superuser())
+       if ((record->flags & GUC_SUPERUSER_ONLY) &&
+               !superuser() &&
+               !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))
                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                                errmsg("must be superuser to examine \"%s\"", 
name)));
+                                errmsg("must be superuser or a member of 
pg_read_all_settings to examine \"%s\"",
+                                name)));
 
        switch (record->vartype)
        {
@@ -8015,10 +8021,13 @@ GetConfigOptionByName(const char *name, const char 
**varname, bool missing_ok)
                           errmsg("unrecognized configuration parameter 
\"%s\"", name)));
        }
 
-       if ((record->flags & GUC_SUPERUSER_ONLY) && !superuser())
+       if ((record->flags & GUC_SUPERUSER_ONLY) &&
+               !superuser() &&
+               !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))
                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                                errmsg("must be superuser to examine \"%s\"", 
name)));
+                                errmsg("must be superuser or a member of 
pg_read_all_settings to examine \"%s\"",
+                                name)));
 
        if (varname)
                *varname = record->name;
@@ -8044,7 +8053,9 @@ GetConfigOptionByNum(int varnum, const char **values, 
bool *noshow)
        if (noshow)
        {
                if ((conf->flags & GUC_NO_SHOW_ALL) ||
-                       ((conf->flags & GUC_SUPERUSER_ONLY) && !superuser()))
+                       ((conf->flags & GUC_SUPERUSER_ONLY) &&
+                       !superuser() &&
+                       !is_member_of_role(GetUserId(), 
DEFAULT_ROLE_READ_ALL_SETTINGS)))
                        *noshow = true;
                else
                        *noshow = false;
diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
index def71edaa8..46b098c84e 100644
--- a/src/include/catalog/pg_authid.h
+++ b/src/include/catalog/pg_authid.h
@@ -99,10 +99,16 @@ typedef FormData_pg_authid *Form_pg_authid;
  * ----------------
  */
 DATA(insert OID = 10 ( "POSTGRES" t t t t t t t -1 _null_ _null_));
+DATA(insert OID = 3355 ( "pg_monitor" f t f f f f f -1 _null_ _null_));
+DATA(insert OID = 3356 ( "pg_read_all_settings" f t f f f f f -1 _null_ 
_null_));
+DATA(insert OID = 3357 ( "pg_read_all_stats" f t f f f f f -1 _null_ _null_));
 DATA(insert OID = 4200 ( "pg_signal_backend" f t f f f f f -1 _null_ _null_));
 
 #define BOOTSTRAP_SUPERUSERID                  10
 
+#define DEFAULT_ROLE_MONITOR           3355
+#define DEFAULT_ROLE_READ_ALL_SETTINGS 3356
+#define DEFAULT_ROLE_READ_ALL_STATS    3357
 #define DEFAULT_ROLE_SIGNAL_BACKENDID  4200
 
 #endif   /* PG_AUTHID_H */
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to