Re: Allow pg_read_all_stats to read pg_stat_progress_* - Mailing list pgsql-hackers

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