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

On Mon, Sep 24, 2018 at 11:13 AM Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

> On Sat, Sep 22, 2018 at 11:23 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>
>> On Mon, Jul 9, 2018 at 11:28 AM Haribabu Kommi <kommi.haribabu@gmail.com>
>> wrote:
>> >
>> > On Mon, Jul 9, 2018 at 3:39 PM Haribabu Kommi <kommi.haribabu@gmail.com>
>> wrote:
>> >>
>> >>
>> >> On Mon, Jul 9, 2018 at 12:24 PM Michael Paquier <michael@paquier.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]
>

Attached new rebased version of the patch that enhances the
pg_stat_statements_reset()
function. This needs to be applied on top of the patch that is posted in
[1].

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

Regards,
Haribabu Kommi
Fujitsu Australia