Hi, Andrey!
I've checked the 5th version of the patch and there are some remarks.
>I've created a new view named pg_stat_statements_aux. But for now both
>views are using the same function pg_stat_statements which returns all
>fields. It seems reasonable to me - if sampling solution will need all
>values it can query the function.
Agreed, it might be useful in some cases.
>But it seems "stats_reset" term is not quite correct in this case. The
>"stats_since" field holds the timestamp of hashtable entry, but not the
>reset time. The same applies to aux_stats_since - for new statement it
>holds its entry time, but in case of reset it will hold the reset time.
Thanks for the clarification. Indeed if we mean the word 'reset' as the
removal of all the hashtable entries during pg_stat_statements_reset()
then we shouldn't use it for the first occurrence timestamp in the
struct pgssEntry.
So with the stats_since field everything is clear.
On the other hand aux_stats_since field can be updated for two reasons:
1) The same as for stats_since due to first occurrence of entry in the
hashtable. And it will be equal to stats_since timestamp in that case.
2) Due to an external reset from an upper level sampler.
I think it's not very important how to name this field, but it would be
better to mention both these reasons in the comment.
As for more important things, if the aux_min_time initial value is zero
like now, then if condition on line 1385 of pg_stat_statements.c will
never be true and aux_min_time will remain zero. Init aux_min_time with
INT_MAX can solve this problem.
It is possible to reduce size of entry_reset_aux() function via
removing if condition on line 2606 and entire third branch from line
2626. Thanks to check in 2612 this will work in all cases.
Also it would be nice to move the repeating several times
lines 2582-2588 into separate function. I think this can make
entry_reset_aux() more shorter and clearer.
In general, the 5th patch applies with no problems, make check-world and
CI gives no error and patch seems to be closely to become RFC.
With best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company