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
