Re: Read access for pg_monitor to pg_replication_origin_status view

Hello, Martín.

Thanks for the new version.

At Thu, 4 Jun 2020 09:17:18 -0300, Martín Marqués <martin(at)2ndquadrant(dot)com> wrote in
> > I'm not sure where to put the GRANT on
> > pg_show_replication_origin_status(), but maybe it also should be at
> > the same place.
>
> Yes, I agree that it makes the revoking/granting easier to read if
> it's grouped by objects, or groups of objects.
>
> Done.

0002 looks fine to me.

> > In the previous comment, one point I meant is that the "to the
> > superuser" should be "to superusers", because a PostgreSQL server
> > (cluster) can define multiple superusers. Another is that "permitted
> > to other users by using the GRANT command." might be obscure for
> > users. In this regard I found a more specific description in the same
> > file:
>
> OK, now I understand what you were saying. :-)

I'm happy to hear that:)

> > So, as the result it would be like the following: (Note that, as you
> > know, I'm not good at this kind of task..)
> >
> > Use of functions for replication origin is restricted to superusers.
> > Use for these functions may be permitted to other users by granting
> > <literal>EXECUTE<literal> privilege on the functions.
> >
> > And in regard to the view, granting privileges on both the view and
> > function to individual user is not practical so we should mention only
> > granting pg_read_all_stats to users, like the attached patch.
>
> I did some re-writing of the doc, which is pretty close to what you
> proposed above.

(0003) Unfortunately, the closing tag of EXECUTE is missing prefixing
'/'.

I see many nearby occurrences of "This function is restricted to
superusers by default, but other users can be granted EXECUTE to run
the function". I'm not sure, but it might be better to use the same
expression, but I don't insist on that. It's not changed in the
attached.

> > By the way, the attachements of your mail are out-of-order. I'm not
> > sure that that does something bad, though.
>
> That's likely Gmail giving them random order when you attach multiple
> files all at once.
>
> New patches attached.

- I'm fine with the direction of this patch. Works as expected, that
is, makes no changes of behavior for replication origin functions,
and pg_read_all_stats can read the pg_replication_origin_status
view.

- The patches cleanly applied to the current HEAD and can be compiled
with a minor fix (fixed in the attached v5).

- The patches should be merged but I'll left that for committer.

- The commit titles are too long. Each of them should be split up into
a brief title and a description. But I think committers would rewrite
them for the final patch to commit so I don't think we don't need to
rewrite them right now.

I'll wait for a couple of days for comments from others or opinions
before moving this to Ready for Committer.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center