On Wed, Oct 13, 2021 at 6:55 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:
> > I would think we would do both…. That is- move to using GRANT/REVOKE, and
> > then just include a GRANT to pg_read_all_stats.
> >
> > Or not. I can see the argument that, because it just goes into the log,
> > that it doesn’t make sense to grant to a predefined role, since that role
> > wouldn’t be able to see the results even if it had access.
>
> I don't think that this is a bad thing to remove the superuser() check
> and replace it with a REVOKE FROM PUBLIC in this case,
IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.
And, I see there are a lot of functions in the code base that does "if
(!superuser())" check and emit "must be superuser to XXX" sort of
error.
> but linking the
> logging of memory contexts with pg_read_all_stats does not seem right
> to me.
Agreed. The user with pg_read_all_stats can't see the server logs so
it doesn't make sense to make them call the function. I will remove
this change from the patch.
Regards,
Bharath Rupireddy.
