Re: Allow pg_read_all_stats to read pg_stat_progress_*

From: Magnus Hagander <magnus(at)hagander(dot)net>
To: "Andrey M(dot) Borodin" <x4mmm(at)yandex-team(dot)ru>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Allow pg_read_all_stats to read pg_stat_progress_*
Date: 2020-04-15 10:25:20
Message-ID: CABUevEzF+JY3CYUyMT4Tgh13MdhrHoduO-T4MfGvq6wqqCF7BA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Apr 15, 2020 at 9:14 AM Andrey M. Borodin <x4mmm(at)yandex-team(dot)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/>

Attachment Content-Type Size
allow_read_all_stats2.diff text/x-patch 1.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrey M. Borodin 2020-04-15 10:58:05 Re: Allow pg_read_all_stats to read pg_stat_progress_*
Previous Message Peter J. Holzer 2020-04-15 10:14:25 Re: pg_restore: could not close data file: Success