Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

On Sat, Sep 22, 2018 at 11:23 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Mon, Jul 9, 2018 at 11:28 AM Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
> wrote:
> >
> > On Mon, Jul 9, 2018 at 3:39 PM Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
> wrote:
> >>
> >>
> >> On Mon, Jul 9, 2018 at 12:24 PM Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
> >>>
> >>> On Fri, Jul 06, 2018 at 05:10:18PM -0400, Alvaro Herrera wrote:
> >>> > Ugh, it's true :-(
> >>> >
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=25fff40798fc4ac11a241bfd9ab0c45c085e2212#patch8
> >>> >
> >>> > Dave, Simon, any comments?
> >>>
> >>> The offending line:
> >>> contrib/pg_stat_statements/pg_stat_statements--1.4--1.5.sql:
> >>> GRANT EXECUTE ON FUNCTION pg_stat_statements_reset() TO
> pg_read_all_stats;
> >>>
> >>> This will need a new version bump down to REL_10_STABLE...
> >>
> >>
> >> Hearing no objections, attached patch removes all permissions from
> PUBLIC
> >> as before this change went in. Or do we need to add command for revoke
> only
> >> from pg_read_all_stats?
> >
> >
> > Revoke all doesn't work, so patch updated with revoke from
> pg_read_all_stats.
> >
>

Thanks for the review.

> The other questionable part of that commit (25fff40798) is that it
> changes permissions for function pg_stat_statements_reset at SQL level
> and for C function it changes the permission check for
> pg_stat_statements, refer below change.
>

The below changes are to support the read access of particular columns
of the pg_stat_statements view to pg_read_all_stats role. These changes
should exist and only the permissions of pg_stat_statements_reset() function
needs to be removed.

> @@ -1391,7 +1392,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
> MemoryContext per_query_ctx;
> MemoryContext oldcontext;
> Oid userid = GetUserId();
> - bool is_superuser = superuser();
> + bool is_allowed_role = false;
> char *qbuffer = NULL;
> Size qbuffer_size = 0;
> Size extent = 0;
> @@ -1399,6 +1400,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
> HASH_SEQ_STATUS hash_seq;
> pgssEntry *entry;
>
> + /* Superusers or members of pg_read_all_stats members are allowed */
> + is_allowed_role = is_member_of_role(GetUserId(),
> DEFAULT_ROLE_READ_ALL_STATS);
>
> Am I confused here? In any case, I think it is better to start a
> separate thread to discuss this issue. It might help us in getting
> more attention on this issue and we can focus on your proposed patch
> in this thread.
>

Started a new thread to discuss about the revoke the execute permissions
of the pg_stat_statements_reset() from pg_read_all_stats role at [1]

[1] -
https://www.postgresql.org/message-id/CAJrrPGf5fCnKqXObpwGN9nMyD--tzOf-7LFCJiz59Z1wJ5qj9A%40mail.gmail.com

Regards,
Haribabu Kommi
Fujitsu Australia