On Wed, Jul 4, 2018 at 7:12 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
>
> On Mon, Jul 2, 2018 at 6:42 PM Sergei Kornilov <sk@zsrv.org> wrote:
>>
>> Hello
>
>
> Thanks for the review.
>
>>
>> I found SELECT pg_stat_statements_reset(NULL,NULL,s.queryid) in tests and
>> it pass tests, but i wonder how it works. Should not we check the NULL
>> through PG_ARGISNULL macro before any PG_GETARG_*? According
>> src/include/fmgr.h
>> > * If function is not marked "proisstrict" in pg_proc, it must check for
>> > * null arguments using this macro. Do not try to GETARG a null
>> > argument!
>
>
> Thanks for checking, Added.
>
>>
>> > pg_stat_statements_reset(userid Oid, dbid Oid, queryid bigint) returns
>> > void
>> And you forgot to change return type in docs (and description of return
>> value)
>
>
> Corrected and also added details of the returns value.
>
> Update patch attached.
+ if (userid != 0 && dbid != 0 && queryid != 0)
UINT64CONST() should be used for the constant for queryid?
It's rare case, but 0 can be assigned as valid queryid. Right?
If yes, it's problematic to use 0 as an invalid queryid here.
+ By default, this function can only be executed by superusers and members
+ of the <literal>pg_read_all_stats</literal> role.
Currently pg_stat_statements_reset() and other stats reset functions like
pg_stat_reset() can be executed only by superusers.
But why did you allow pg_read_all_stats role to execute that function,
by default? That change looks strange to me.
Regards,
--
Fujii Masao