Allow pg_read_all_stats to read pg_stat_progress_*

On Wed, Apr 15, 2020 at 9:14 AM Andrey M. Borodin <x4mmm@yandex-team.ru>
wrote:

Hi!

One of our users asked me why they cannot read details of
pg_stat_progress_vacuum while they have pg_read_all_stats role.
Maybe I'm missing something, but I think they should be able to read
stats...

PFA fix.
This affects pg_stat_progress_analyze, pg_stat_progress_basebackup,
pg_stat_progress_cluster, pg_stat_progress_create_index and
pg_stat_progress_vacuum.

With patch
postgres=# set role pg_read_all_stats ;
postgres=> select * from pg_stat_progress_vacuum ;
pid | datid | datname | relid | phase | heap_blks_total |
heap_blks_scanned | heap_blks_vacuumed | index_vacuum_count |
max_dead_tuples | num_dead_tuples

-------+-------+----------+-------+---------------+-----------------+-------------------+--------------------+--------------------+-----------------+-----------------
76331 | 12923 | postgres | 1247 | scanning heap | 10 |
1 | 0 | 0 | 2910 |
0
(1 row)

Without patch
postgres=# set role pg_read_all_stats ;
SET
postgres=> select * from pg_stat_progress_vacuum ;
pid | datid | datname | relid | phase | heap_blks_total |
heap_blks_scanned | heap_blks_vacuumed | index_vacuum_count |
max_dead_tuples | num_dead_tuples

-------+-------+----------+-------+-------+-----------------+-------------------+--------------------+--------------------+-----------------+-----------------
76331 | 12923 | postgres | | | |
| | | |

(1 row)

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.

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.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net> написал(а):

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.

Thanks!

Best regards, Andrey Borodin.

At Wed, 15 Apr 2020 15:58:05 +0500, "Andrey M. Borodin" <x4mmm@yandex-team.ru> wrote in

15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net> написал(а):
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.

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?

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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Apr 16, 2020 at 7:05 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

At Wed, 15 Apr 2020 15:58:05 +0500, "Andrey M. Borodin" <
x4mmm@yandex-team.ru> wrote in

15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net>

написал(а):

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/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

At Thu, 16 Apr 2020 14:46:00 +0200, Magnus Hagander <magnus@hagander.net> wrote in

On Thu, Apr 16, 2020 at 7:05 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

At Wed, 15 Apr 2020 15:58:05 +0500, "Andrey M. Borodin" <
x4mmm@yandex-team.ru> wrote in

15 апр. 2020 г., в 15:25, Magnus Hagander <magnus@hagander.net>

написал(а):

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)

As a priciple, I prefer the "optimized" (or pg_stat_statements')
pattern because that style suggests that the privilege is (shold be)
same to all entries, not because that it might be a bit faster. My
suggestion above is just from "same style with a nearby code". But at
least the v2 code introduces the third style (mixture of in-place and
pre-evaluated) seemed a kind of ad-hoc.

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_*.

Yeah. pg_database_size() ERRORs out for insufficient privileges. On
the other hand pg_stat_* returns "<insufficient privilege>" not
ERRORing out.

For example, pg_stat_get_backend_wait_event_type is documented as

https://www.postgresql.org/docs/current/monitoring-stats.html#MONITORING-STATS-BACKEND-FUNCS-TABLE

"Wait event type name if backend is currently waiting, otherwise
NULL. See Table 27.4 for details."

I would read this as "If the function returns non-null value, the
returned value represents the wait event type mentioned in Table
27.4", but, "<insufficient privilege>" is not a wait event type. I
think something like "text-returning functions may return some
out-of-the-domain strings like "<insufficient privilege>" under
corresponding conditions".

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?

Exactly. It looks good to me. Thanks!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

16 апр. 2020 г., в 17:46, Magnus Hagander <magnus@hagander.net> написал(а):

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?

<allow_read_all_stats3.diff>

Is it correct that we use DEFAULT_ROLE_READ_ALL_STATS regardless of inheritance? I'm not familiar with what is inherited and what is not, so I think it's better to ask explicitly.

+#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role))

Besides this, the patch looks good to me.
Thanks!

Best regards, Andrey Borodin,

On Mon, Apr 20, 2020 at 12:43 PM Andrey M. Borodin <x4mmm@yandex-team.ru>
wrote:

16 апр. 2020 г., в 17:46, Magnus Hagander <magnus@hagander.net>

написал(а):

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?

<allow_read_all_stats3.diff>

Is it correct that we use DEFAULT_ROLE_READ_ALL_STATS regardless of
inheritance? I'm not familiar with what is inherited and what is not, so I
think it's better to ask explicitly.

+#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(),
DEFAULT_ROLE_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role))

It is consistent with all the other uses of DEFAULT_ROLE_READ_ALL_STATS
that I can find.

Besides this, the patch looks good to me.

Thanks, I've pushed it now.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

Greetings,

* Magnus Hagander (magnus@hagander.net) wrote:

On Mon, Apr 20, 2020 at 12:43 PM Andrey M. Borodin <x4mmm@yandex-team.ru>
wrote:

16 апр. 2020 г., в 17:46, Magnus Hagander <magnus@hagander.net>

написал(а):

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?

<allow_read_all_stats3.diff>

Is it correct that we use DEFAULT_ROLE_READ_ALL_STATS regardless of
inheritance? I'm not familiar with what is inherited and what is not, so I
think it's better to ask explicitly.

+#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(),
DEFAULT_ROLE_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role))

It is consistent with all the other uses of DEFAULT_ROLE_READ_ALL_STATS
that I can find.

Ugh. That doesn't make it correct though.. We really should be using
has_privs_of_role() for these cases (and that goes for all of the
default role cases- some of which are correct and others are not, it
seems).

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Ugh. That doesn't make it correct though.. We really should be using
has_privs_of_role() for these cases (and that goes for all of the
default role cases- some of which are correct and others are not, it
seems).

I have a different concern about this patch: while reading statistical
values is fine, do we REALLY want pg_read_all_stats to enable
pg_stat_get_activity(), ie viewing other sessions' command strings?
That opens security considerations that don't seem to me to be covered
by the description of the role.

regards, tom lane

On Mon, Apr 20, 2020 at 4:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Stephen Frost <sfrost@snowman.net> writes:

Ugh. That doesn't make it correct though.. We really should be using
has_privs_of_role() for these cases (and that goes for all of the
default role cases- some of which are correct and others are not, it
seems).

I have a different concern about this patch: while reading statistical
values is fine, do we REALLY want pg_read_all_stats to enable
pg_stat_get_activity(), ie viewing other sessions' command strings?
That opens security considerations that don't seem to me to be covered
by the description of the role.

It already did allow that, and that's fully documented.

The patch only adds the ability to get at it through functions, but not
through views. (And the pg_stat_progress_* views).

The pg_stat_activity change is only:
@@ -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;

Which moves the check into the macro, but doesn't change how it works.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;