Re: Add stats_reset to pg_stat_all_tables|indexes and related views

On Mon, Oct 06, 2025 at 08:57:32AM +0000, Bertrand Drouvot wrote:
> A few remakrs:

This looks globally sensible, from here.  A few remarks.

> - It does not use an existing macro (as such macro does not exist) to generate the
> function that reports this new field. So we've more flexibility for the name
> and went for pg_stat_get_function_stat_reset_time() (to be consistent with
> say pg_stat_get_db_stat_reset_time()).

There is the same exception for function_calls().  Not using a macro
is OK.  We cannot remove any code if introduced.

> - The tests are done in the isolation suite (as this is where
> pg_stat_reset_single_function_counters() is already being tested) and I don't
> think there is a need to add one in the regress suite (that test for the
> pg_stat_user_functions output). Indeed the pg_stat_get_function_stat_reset_time()
> call output test is already added in the isolation one.

A test addition in the isolation stats spec sounds OK to me.  As far
as I can see, there is one mistake here: stats_1.out is left
untouched.  This alternate output has been introduced by a2f433fa491f
for the case where 2PC is disabled.

Did you also look at the test stability under CATCACHE_FORCE_RELEASE
(d6c0db14836c)?  I'd rather avoid a bet on some buildfarm members if
we can check that beforehand (FORCE_RELEASE is cheaper once run with
installcheck on an instance already initdb'd).

Should we care about the step s2_func_stats, as well?

> - The new field is not added in pg_stat_xact_user_functions for the exact
> same reasons as it was not added for the relations case.

Of course.  reset_stats is a state stored in shmem, we don't need it
in the transaction views that only get what's local to the
transaction.  Perhaps I should have kept the note in a5b543258aa2
about the table/index stats, but that did not strike me as worth
mentioning based on how the xact views are implemented.
--
Michael