On Tue, Sep 25, 2018 at 3:09 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Sep 25, 2018 at 01:49:09PM +1000, Haribabu Kommi wrote: > Thanks for the review. > Fixed in the attached patch as per your suggestion.
Thanks for the review.
Hmm. I see a problem with the tests and the stability of what pg_stat_statements_reset() can return. Normally installcheck is disabled in contrib/pg_stat_statements/Makefile but if you remove this barrier and run the tests with a server loading the module in shared_preload_libraries then things are not stable. We don't have this kind of instability on HEAD. Some call to pg_stat_statements_reset() system-wide is visibly missing.
The difference in results of the output of the pg_stat_statements_reset()
function is based on the earlier statements that are stored in the pg_stat_statements
table, this varies based on the environment. So I created a wrapper function
that masks the return value of the first reset and then the test is stable.
check whether is it fine or any better approach to handle it?
+ if (!pgss || !pgss_hash) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("pg_stat_statements must be loaded via shared_preload_libraries"))); This check can be within entry_reset().
Moved.
+ the specified userid, dbid and queryid. Returns the total number of + statement statistics that are reset based on the specified input. + If any of the parameter is not specified, the default value NULL(invalid) Missing some markups for the three field names here, as well as for NULL which is a value.
Corrected.
I can buy the compatibility breakage with the return result of pg_stat_statements_reset when specified without arguments.
Some nannyism: If all entries are removed and a new file needs to be written, you could save a bit of indentation by returning immediately when (num_entries != num_remove).
