On Tue, Sep 25, 2018 at 3:09 PM Michael Paquier <mich...@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).
>
Corrected.
Attached is the updated patch.
Regards,
Haribabu Kommi
Fujitsu Australia
0001-pg_stat_statements_reset-to-reset-specific-query-use_v7.patch
Description: Binary data
