Hi,
On Tue, Oct 07, 2025 at 08:58:30AM +0900, Michael Paquier wrote:
> On Mon, Oct 06, 2025 at 08:57:32AM +0000, Bertrand Drouvot wrote:
> > A few remakrs:
>
> This looks globally sensible, from here.
Thanks for looking at it!
> 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.
Yeah, no need to add a new macro here.
> > - 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.
Good catch! Fixed in v2 attached.
> Did you also look at the test stability under CATCACHE_FORCE_RELEASE
> (d6c0db14836c)?
I just checked with CATCACHE_FORCE_RELEASE (and RELCACHE_FORCE_RELEASE) and
that looks stable on my side.
> Should we care about the step s2_func_stats, as well?
stats are reset only for s1, but that does not hurt to check that s2 also
provides expected results: done in the attached. Also re-tested CATCACHE_FORCE_RELEASE
and RELCACHE_FORCE_RELEASE with this new test.
> > - 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.
Yeah. Maybe add a word or two in this commit if you really feel the
need. I just mentioned it in the thread for reference anyway.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
