Horton

Thread

Commits

  1. Allow pg_read_all_stats to access all stats views again

  1. Allow pg_read_all_stats to read pg_stat_progress_*

    x4mmm@yandex-team.ru — 2020-04-15T07:13:57Z

    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)

Thanks!

Best regards, Andrey Borodin.

  2. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Magnus Hagander <magnus@hagander.net> — 2020-04-15T10:25:20Z

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

  3. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    x4mmm@yandex-team.ru — 2020-04-15T10:58:05Z

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

  4. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-16T05:05:04Z

    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

  5. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Magnus Hagander <magnus@hagander.net> — 2020-04-16T12:46:00Z

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

  6. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-17T01:29:10Z

    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

  7. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    x4mmm@yandex-team.ru — 2020-04-20T10:43:22Z

    
> 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,

  8. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Magnus Hagander <magnus@hagander.net> — 2020-04-20T11:05:35Z

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

  9. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Stephen Frost <sfrost@snowman.net> — 2020-04-20T11:10:02Z

    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

  10. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-20T14:12:11Z

    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

  11. Re: Allow pg_read_all_stats to read pg_stat_progress_*

    Magnus Hagander <magnus@hagander.net> — 2020-04-20T14:15:10Z

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