On Thu, Apr 16, 2020 at 7:05 AM Kyotaro Horiguchi <[email protected]>
wrote:
> At Wed, 15 Apr 2020 15:58:05 +0500, "Andrey M. Borodin" <
> [email protected]> wrote in
> > > 15 апр. 2020 г., в 15:25, Magnus Hagander <[email protected]>
> написал(а):
> > > I think that makes perfect sense. The documentation explicitly says
> "can read all pg_stat_* views", which is clearly wrong -- so either the
> code or the docs should be fixed, and it looks like it's the code that
> should be fixed to me.
> > Should it be bug or v14 feature?
> >
> > Also pgstatfuncs.c contains a lot more checks of
> has_privs_of_role(GetUserId(), beentry->st_userid).
> > Maybe grant them all?
> >
> > > As for the patch, one could argue that we should just store the
> resulting boolean instead of re-running the check (e.g. have a "bool
> has_stats_privilege" or such), but perhaps that's an unnecessary
> micro-optimization, like the attached.
> >
> > Looks good to me.
>
> pg_stat_get_activty checks (has_privs_of_role() ||
> is_member_of_role()) in-place for every entry. It's not necessary but
> I suppose that doing the same thing for pg_stat_progress_info might be
> better.
>
>From a result perspective, it shouldn't make a difference though, should
it? It's a micro-optimization, but it might not have an actual performance
effect in reality as well, but the result should always be the same?
(FWIW, pg_stat_statements has a coding pattern similar to the one I
suggested in the patch)
>
> It's another issue, but pg_stat_get_backend_* functions don't consider
> pg_read_all_stats. I suppose that the functions should work under the
> same criteria to pg_stat views, and maybe explicitly documented?
>
That's a good question. They haven't been documented to do so, but it
certainly seems *weird* that the same information should be available
through a view like pg_stat_activity, but not through the functions.
I would guess this was simply forgotten in 25fff40798f -- I don't recall
any discussion about it. The commit message specifically says
pg_database_size() and pg_tablespace_size(), but mentions nothing about
pg_stat_*.
>
> If we do that, it may be better that we define "PGSTAT_VIEW_PRIV()" or
> something like and replace the all occurances of the idiomatic
> condition with it.
>
You mean something like the attached?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 175f4fd26b..446044609e 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -33,6 +33,8 @@
#define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32 *)&(var))))
+#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role))
+
/* Global bgwriter statistics, from bgwriter.c */
extern PgStat_MsgBgWriter bgwriterStats;
@@ -537,7 +539,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
values[1] = ObjectIdGetDatum(beentry->st_databaseid);
/* show rest of the values including relid only to role members */
- if (has_privs_of_role(GetUserId(), beentry->st_userid))
+ if (HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
{
values[2] = ObjectIdGetDatum(beentry->st_progress_command_target);
for (i = 0; i < PGSTAT_NUM_PROGRESS_PARAM; i++)
@@ -669,8 +671,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[16] = true;
/* 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))
+ if (HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
{
SockAddr zero_clientaddr;
char *clipped_activity;
@@ -1007,7 +1008,7 @@ pg_stat_get_backend_activity(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
activity = "<backend information not available>";
- else if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
activity = "<insufficient privilege>";
else if (*(beentry->st_activity_raw) == '\0')
activity = "<command string not enabled>";
@@ -1031,7 +1032,7 @@ pg_stat_get_backend_wait_event_type(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
wait_event_type = "<backend information not available>";
- else if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
wait_event_type = "<insufficient privilege>";
else if ((proc = BackendPidGetProc(beentry->st_procpid)) != NULL)
wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
@@ -1052,7 +1053,7 @@ pg_stat_get_backend_wait_event(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
wait_event = "<backend information not available>";
- else if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
wait_event = "<insufficient privilege>";
else if ((proc = BackendPidGetProc(beentry->st_procpid)) != NULL)
wait_event = pgstat_get_wait_event(proc->wait_event_info);
@@ -1074,7 +1075,7 @@ pg_stat_get_backend_activity_start(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
- if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
PG_RETURN_NULL();
result = beentry->st_activity_start_timestamp;
@@ -1100,7 +1101,7 @@ pg_stat_get_backend_xact_start(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
- if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
PG_RETURN_NULL();
result = beentry->st_xact_start_timestamp;
@@ -1122,7 +1123,7 @@ pg_stat_get_backend_start(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
- if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
PG_RETURN_NULL();
result = beentry->st_proc_start_timestamp;
@@ -1146,7 +1147,7 @@ pg_stat_get_backend_client_addr(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
- if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
PG_RETURN_NULL();
/* A zeroed client addr means we don't know */
@@ -1193,7 +1194,7 @@ pg_stat_get_backend_client_port(PG_FUNCTION_ARGS)
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
- if (!has_privs_of_role(GetUserId(), beentry->st_userid))
+ else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
PG_RETURN_NULL();
/* A zeroed client addr means we don't know */
