Re: Read access for pg_monitor to pg_replication_origin_status view

From: Martín Marqués <martin(at)2ndquadrant(dot)com>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Read access for pg_monitor to pg_replication_origin_status view
Date: 2020-06-03 16:32:28
Message-ID: CAPdiE1zUeSycQ-VHrZQMDR4vkPthYTSxeQPXM-ZfH8QHDBCE=Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Kyotaro-san,

Thank you for taking the time to review my patches. Would you like to
set yourself as a reviewer in the commit entry here?
https://commitfest.postgresql.org/28/2577/

> 0002:
>
> It is forgetting to grant pg_read_all_stats to execute
> pg_show_replication_origin_status. As the result pg_read_all_stats
> gets error on executing the function, not on doing select on the view.

Seems I was testing on a cluster where I had already been digging, so
pg_real_all_stats had execute privileges on
pg_show_replication_origin_status (I had manually granted that) and
didn't notice because I forgot to drop the cluster and initialize
again.

Thanks for the pointer here!

> 0003:
>
> It seems to be a take-after of adminpack's documentation, but a
> superuser is not the only one on PostgreSQL. The something like the
> description in 27.2.2 Viewing Statistics looks more suitable.
>
> > Superusers and members of the built-in role pg_read_all_stats (see
> > also Section 21.5) can see all the information about all sessions.
>
> Section 21.5 is already saying as follows.
>
> > pg_monitor
> > Read/execute various monitoring views and functions. This role is a
> > member of pg_read_all_settings, pg_read_all_stats and
> > pg_stat_scan_tables.

I'm not sure if I got this right, but I added some more text to point
out that the pg_read_all_stats role can also access one specific
function. I personally think it's a bit too detailed, and if we wanted
to add details it should be formatted differently, which would require
a more invasive patch (would prefer leaving that out, as it might even
mean moving parts which are not part of this patch).

In any case, I hope the change fits what you've kindly pointed out.

> 0004:
>
> Looks fine by me.

Here goes v3 of the patch

--
Martín Marqués http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachment Content-Type Size
v3-0001-Access-to-pg_replication_origin_status-view-has-r.patch text/x-patch 3.0 KB
v3-0004-Apply-changes-to-the-documentation-to-reflect-tha.patch text/x-patch 1.1 KB
v3-0002-We-want-the-monitoring-role-pg_read_all_stats-to-.patch text/x-patch 1.2 KB
v3-0003-Change-replication-origin-function-documenation-t.patch text/x-patch 1.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Mark Dilger 2020-06-03 17:05:26 Towards easier AMs: Cleaning up inappropriate use of name "relkind"
Previous Message Robert Haas 2020-06-03 16:16:08 Re: Expand the use of check_canonical_path() for more GUCs