Обсуждение: Add stats_reset to pg_stat_all_tables|indexes and related views

Hi hackers,

while working on relfilenode statistics [1], I observed that it's possible to call
pg_stat_reset_single_table_counters() on a relation but the reset time is not
reported in the related views.

I think it's interesting to know when the stats have been reset, so the attached
patch is adding stats_reset to the related views.

Also, it's more consistent with other stat kinds that also report reset times.

This new field is not included into the pg_stat_xact_* views because the 0 values
in transaction-local stats have nothing to do with reset operations.

The patch is pretty straightforward, it:

- Adds a reset_timestamp_cb to PGSTAT_KIND_RELATION
- Adds a function to retrieve the stats_reset field (note that this function is
created on top of the existing PG_STAT_GET_RELENTRY_TIMESTAMPTZ macro, so lacking
some flexibility regarding the function name)
- Adds the stats_reset field in the views that are concerned
- Updates the documentation
- Updates some tests

Regards,

[1]: https://postgr.es/m/ZlGYokUIlERemvpB%40ip-10-97-1-34.eu-west-3.compute.internal

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

> while working on relfilenode statistics [1], I observed that it's possible to call
> pg_stat_reset_single_table_counters() on a relation but the reset time is not
> reported in the related views.
>
> I think it's interesting to know when the stats have been reset, so the attached
> patch is adding stats_reset to the related views.
>
> Also, it's more consistent with other stat kinds that also report reset times.

+1. This field should clearly be there.

> This new field is not included into the pg_stat_xact_* views because the 0 values
> in transaction-local stats have nothing to do with reset operations.
>
> The patch is pretty straightforward, it:

Nothing jumped out at me in the code. Although, I think we should add
at least one test where  pg_stat_reset_single_table_counters() is called
with an index OID. There isn't a difference in the way the stats are
reset for indexes and tables, but they are presented in different views,
so it makes sense to add test coverage.

On a side note: I really think pg_stat_reset_single_table_counters is
the wrong name here, since other OIDs can be used here; indexes
or materialized views, etc. Maybe pg_stat_reset_single_relation_counters
will be better?

--
Sami Imseih
Amazon Web Services (AWS)

On Thu, Oct 02, 2025 at 05:27:06PM -0500, Sami Imseih wrote:
> +1. This field should clearly be there.

Yeah, Bertrand has mentioned this one to me offlist, and I was equally
surprised by the field gone missing.

One question would be if we need to worry about the additional bytes
of this field, but seeing the size of PgStat_StatTabEntry currently
I'm going to answer "no" to my own question in advance.

> Nothing jumped out at me in the code. Although, I think we should add
> at least one test where  pg_stat_reset_single_table_counters() is called
> with an index OID. There isn't a difference in the way the stats are
> reset for indexes and tables, but they are presented in different views,
> so it makes sense to add test coverage.

Makes sense to me.  This matters in terms of coverage for HEAD,
being outside of the scope of this proposal.

> On a side note: I really think pg_stat_reset_single_table_counters is
> the wrong name here, since other OIDs can be used here; indexes
> or materialized views, etc. Maybe pg_stat_reset_single_relation_counters
> will be better?

It's mostly a historical artifact at this stage, and the function is
documented as being usable for an index or a table.  Using "relation"
would be more consistent, indeed.  I am not sure if it's worth
bothering, though.

What's the point of having tests for two tables?  Shouldn't the one
based on test_last_scan be enough?  The one on pg_shdescription may
actually fail on repeated runs, may it not?  It is a shared catalog.
--
Michael

Hi,

On Fri, Oct 03, 2025 at 10:24:39AM +0900, Michael Paquier wrote:
> On Thu, Oct 02, 2025 at 05:27:06PM -0500, Sami Imseih wrote:
> > +1. This field should clearly be there.
> 

Thank you both for looking at it!

> One question would be if we need to worry about the additional bytes
> of this field, but seeing the size of PgStat_StatTabEntry currently
> I'm going to answer "no" to my own question in advance.

Yeah, I was thinking the same and reached the same conclusion.

> > Nothing jumped out at me in the code. Although, I think we should add
> > at least one test where  pg_stat_reset_single_table_counters() is called
> > with an index OID. There isn't a difference in the way the stats are
> > reset for indexes and tables, but they are presented in different views,
> > so it makes sense to add test coverage.
> 
> Makes sense to me.  This matters in terms of coverage for HEAD,
> being outside of the scope of this proposal.

Added one test on pg_stat_all_indexes in v2 attached. That's the first test
on "pg_stat_all_indexes" in .sql files. It just tests the new stats_reset field,
I think it's sufficient for the purpose of this patch.

> > On a side note: I really think pg_stat_reset_single_table_counters is
> > the wrong name here, since other OIDs can be used here; indexes
> > or materialized views, etc. Maybe pg_stat_reset_single_relation_counters
> > will be better?
> 
> It's mostly a historical artifact at this stage,

Yeah, it comes from 083e1b0f27df and the associated discussion is [1]. From what
I can see, at that time the struct that was holding the table and index stats
was "PgStat_TableCounts". So the naming "pg_stat_reset_single_table_counters"
somehow made more sense at that time.

> and the function is
> documented as being usable for an index or a table.  Using "relation"
> would be more consistent, indeed.  I am not sure if it's worth
> bothering, though.

It's done and documented that way since 2010, so I'm also not sure it's worth
bothering.

> What's the point of having tests for two tables?  Shouldn't the one
> based on test_last_scan be enough?  The one on pg_shdescription may
> actually fail on repeated runs, may it not?  It is a shared catalog.

Yeah this one may need to be done differently. I just removed it as it does
not provide extra value here.

[1]: https://www.postgresql.org/message-id/flat/9837222c1001240837r5c103519lc6a74c37be5f1831%40mail.gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Oct 03, 2025 at 05:33:00AM +0000, Bertrand Drouvot wrote:
> On Fri, Oct 03, 2025 at 10:24:39AM +0900, Michael Paquier wrote:
>> Makes sense to me.  This matters in terms of coverage for HEAD,
>> being outside of the scope of this proposal.
>
> Added one test on pg_stat_all_indexes in v2 attached. That's the first test
> on "pg_stat_all_indexes" in .sql files. It just tests the new stats_reset field,
> I think it's sufficient for the purpose of this patch.

But the fact that the reset function is able to work on indexes is
something separate than this patch.  Wouldn't it be better to check
some of the non-timestamp fields of pg_stat_all_indexes with a reset
in a test that applies before the main patch?  That's a minor comment,
sure, but this part feels like a separate item.  So I'd like to
extract that as a patch of its own, then apply it as an independent
piece.

> Yeah, it comes from 083e1b0f27df and the associated discussion is [1]. From what
> I can see, at that time the struct that was holding the table and index stats
> was "PgStat_TableCounts". So the naming "pg_stat_reset_single_table_counters"
> somehow made more sense at that time.

Thanks for the history digging.

I have double-checked the whole patch, at it looks like you have the
right coverage in terms of the docs and the system views, impacting
the set of system views for pg_stat_{all,user,sys}_{tables,indexes},
and same for the pg_statio_* counterparts.  So things look clear here.
--
Michael

Hi,

On Fri, Oct 03, 2025 at 04:50:07PM +0900, Michael Paquier wrote:
> On Fri, Oct 03, 2025 at 05:33:00AM +0000, Bertrand Drouvot wrote:
> > On Fri, Oct 03, 2025 at 10:24:39AM +0900, Michael Paquier wrote:
> >> Makes sense to me.  This matters in terms of coverage for HEAD,
> >> being outside of the scope of this proposal.
> > 
> > Added one test on pg_stat_all_indexes in v2 attached. That's the first test
> > on "pg_stat_all_indexes" in .sql files. It just tests the new stats_reset field,
> > I think it's sufficient for the purpose of this patch.
> 
> But the fact that the reset function is able to work on indexes is
> something separate than this patch.  Wouldn't it be better to check
> some of the non-timestamp fields of pg_stat_all_indexes with a reset
> in a test that applies before the main patch?

Yeah, stats.sql is already doing some tests coverage on index stats (through
retrieving idx_scan and friends in pg_stat_all_tables) but the reset function
is not tested explicitly on indexes.

> That's a minor comment,
> sure, but this part feels like a separate item.  So I'd like to
> extract that as a patch of its own, then apply it as an independent
> piece.

That makes sense. Done in 0001 attached.

> > Yeah, it comes from 083e1b0f27df and the associated discussion is [1]. From what
> > I can see, at that time the struct that was holding the table and index stats
> > was "PgStat_TableCounts". So the naming "pg_stat_reset_single_table_counters"
> > somehow made more sense at that time.
> 
> Thanks for the history digging.
> 
> I have double-checked the whole patch, at it looks like you have the
> right coverage in terms of the docs and the system views, impacting
> the set of system views for pg_stat_{all,user,sys}_{tables,indexes},
> and same for the pg_statio_* counterparts.  So things look clear here.

Thanks! Attached v3, split in 2 sub patches as discussed above (0002 is same
as v2 except that the tests has been changed due to 0001).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Oct 03, 2025 at 10:03:32AM +0000, Bertrand Drouvot wrote:
> That makes sense. Done in 0001 attached.

Done this one, with a tweak to use directly the index name, which is a
primary key from the test table.

> Thanks! Attached v3, split in 2 sub patches as discussed above (0002 is same
> as v2 except that the tests has been changed due to 0001).

And applied this one as well after a few tweaks, mainly consistency in
the code, with what should be the proper format bumps (thanks for the
notes in your proposed commit message).
--
Michael

On Mon, Oct 06, 2025 at 03:33:41PM +0900, Michael Paquier wrote:
> And applied this one as well after a few tweaks, mainly consistency in
> the code, with what should be the proper format bumps (thanks for the
> notes in your proposed commit message).

And I forgot to mention something: now the only stats kind that does
not have a reset timestamp callback but has a SQL function able to do
resets is PGSTAT_KIND_FUNCTION.  Is it worth caring about doing
something for this case as well?
--
Michael

Hi,

On Mon, Oct 06, 2025 at 03:37:18PM +0900, Michael Paquier wrote:
> On Mon, Oct 06, 2025 at 03:33:41PM +0900, Michael Paquier wrote:
> > And applied this one as well after a few tweaks, mainly consistency in
> > the code, with what should be the proper format bumps (thanks for the
> > notes in your proposed commit message). 

Thanks!

> 
> And I forgot to mention something: now the only stats kind that does
> not have a reset timestamp callback but has a SQL function able to do
> resets is PGSTAT_KIND_FUNCTION.

Checking for other stats kinds would have been my next step, step for doing it!

> Is it worth caring about doing
> something for this case as well?

I think so as I don't see any reasons not to do so.

PFA a patch to add stats_reset to pg_stat_user_functions.

A few remakrs:

- 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()).

- 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.

- 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.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

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

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

On Tue, Oct 07, 2025 at 08:21:16AM +0000, Bertrand Drouvot wrote:
> 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.

One thing which was looking annoying, with fresh eyes, is the fact
that querying stats_reset in each query that wants to retrieve the
function stats leads to 1.2k lines of additional diffs.  However, only
one permutation uses the reset function.  So at the end, I have added
a new step that queries pg_stat_get_function_stat_reset_time(), and
used it in the only permutation that does a reset of the counters.
This move leads to 20 lines of additional output for each output file,
which is more tolerable.  Applied with this tweak, and bumps with the
mandatory bumps for the catalog version and PGSTAT_FILE_FORMAT_ID.

>> 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.

This did not feel absolutely mandatory, so I have left this part out
of the commit message.

We should be done here, then.
--
Michael

Hi,

On Wed, Oct 08, 2025 at 12:54:18PM +0900, Michael Paquier wrote:
> 
> So at the end, I have added
> a new step that queries pg_stat_get_function_stat_reset_time(), and
> used it in the only permutation that does a reset of the counters.

Makes sense and it still tests null and not null cases for the new field (
so both cases are covered).

> Applied with this tweak, and bumps with the
> mandatory bumps for the catalog version and PGSTAT_FILE_FORMAT_ID.

Thanks!

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com