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

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]