Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?

On Fri, Oct 22, 2021 at 3:15 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 10/20/21, 11:44 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> > I would like to confine this thread to allowing non-superusers with a
> > predefined role (earlier suggestion was to use pg_read_all_stats) to
> > access views pg_backend_memory_contexts and pg_shmem_allocations and
> > functions pg_get_backend_memory_contexts and pg_get_shmem_allocations.
> > Attaching the previous v2 patch here for further review and thoughts.
>
> I took a look at the new patch.  The changes to system_views.sql look
> good to me.

Thanks for reviewing.

> Let's be sure to update doc/src/sgml/catalogs.sgml as
> well.

Added.

> -SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
> +SELECT pg_log_backend_memory_contexts(pg_backend_pid());
>
> nitpick: Do we need to remove the "* FROM" here?  This seems like an
> unrelated change.

Yes it's not mandatory, while we are on this I thought we could
combine them, I've also specified this in the commit message. IMO, we
can leave it to the committer.

> +-- test to check privileges of system views pg_shmem_allocations,
> +-- pg_backend_memory_contexts and function pg_log_backend_memory_contexts.
>
> I think the comment needs to be updated to remove the reference to
> pg_log_backend_memory_contexts.  It doesn't appear to be tested here.

Removed.

> +SELECT name, ident, parent, level, total_bytes >= free_bytes
> +  FROM pg_backend_memory_contexts WHERE level = 0; -- permission denied error
> +SELECT COUNT(*) >= 0 AS ok FROM pg_shmem_allocations; -- permission denied error
>
> Since we're really just checking the basic permissions, could we just
> do the "count(*) >= 0" check for both views?

Done.

Here's v3 for further review.

Regards,
Bharath Rupireddy.