Thread: pg_stat_reset_single_*_counters vs pg_stat_database.stats_reset

Hi,

Starting with the below commit, pg_stat_reset_single_function_counters,
pg_stat_reset_single_table_counters don't just reset the stats for the
individual function, but also set pg_stat_database.stats_reset.

commit 4c468b37a281941afd3bf61c782b20def8c17047
Author: Magnus Hagander <magnus@hagander.net>
Date:   2011-02-10 15:09:35 +0100

    Track last time for statistics reset on databases and bgwriter

    Tracks one counter for each database, which is reset whenever
    the statistics for any individual object inside the database is
    reset, and one counter for the background writer.

    Tomas Vondra, reviewed by Greg Smith
     /*


@@ -4107,6 +4118,8 @@ pgstat_recv_resetsinglecounter(PgStat_MsgResetsinglecounter *msg, int len)
     if (!dbentry)
         return;
 
+    /* Set the reset timestamp for the whole database */
+    dbentry->stat_reset_timestamp = GetCurrentTimestamp();
 
     /* Remove object if it exists, ignore it if not */
     if (msg->m_resettype == RESET_TABLE)


The relevant thread is [1], with the most-on-point message at [2].
pg_stat_reset_single_*_counters were introduced in [3]


This behaviour can be trivially (and is) implemented for the shared memory
stats patch. But every time I read over that part of the code it feels just
profoundly wrong to me.  Way worse than *not* resetting
pg_stat_database.stats_reset.

Anybody that uses the time since the stats reset as part of a calculation of
transactions / sec, reads / sec or such will get completely bogus results
after a call to pg_stat_reset_single_table_counters().


Maybe I just don't understand what these reset functions are intended for?
Their introduction [3] didn't explain much either. To me the behaviour of
resetting pg_stat_database.stats_reset but nothing else in pg_stat_database
makes them kind of dangerous.

Greetings,

Andres Freund

[1] https://postgr.es/m/7177d0cd40b82409024e7c495e9d6992.squirrel%40sq.gransy.com
[2] https://www.postgresql.org/message-id/4D0E5A54.3060302%40fuzzy.cz
[3] https://www.postgresql.org/message-id/9837222c1001240837r5c103519lc6a74c37be5f1831%40mail.gmail.com

Hi,

On 2022-03-23 17:55:16 -0700, Andres Freund wrote:
> Maybe I just don't understand what these reset functions are intended for?
> Their introduction [3] didn't explain much either. To me the behaviour of
> resetting pg_stat_database.stats_reset but nothing else in pg_stat_database
> makes them kind of dangerous.

Forgot to add: At the very least we should document that weird behaviour,
because it's certainly not obvious.  But imo we should either remove the
behaviour or drop the functions.

      <row>
       <entry role="func_table_entry"><para role="func_signature">
        <indexterm>
         <primary>pg_stat_reset_single_table_counters</primary>
        </indexterm>
        <function>pg_stat_reset_single_table_counters</function> ( <type>oid</type> )
        <returnvalue>void</returnvalue>
       </para>
       <para>
        Resets statistics for a single table or index in the current database
        or shared across all databases in the cluster to zero.
       </para>
       <para>
        This function is restricted to superusers by default, but other users
        can be granted EXECUTE to run the function.
       </para></entry>
      </row>

      <row>
       <entry role="func_table_entry"><para role="func_signature">
        <indexterm>
         <primary>pg_stat_reset_single_function_counters</primary>
        </indexterm>
        <function>pg_stat_reset_single_function_counters</function> ( <type>oid</type> )
        <returnvalue>void</returnvalue>
       </para>
       <para>
        Resets statistics for a single function in the current database to
        zero.
       </para>
       <para>
        This function is restricted to superusers by default, but other users
        can be granted EXECUTE to run the function.
       </para></entry>
      </row>


Greetings,

Andres Freund

Hi,

On 2022-03-23 18:47:58 -0700, David G. Johnston wrote:
> It also seems that each tracked object type needs to have its own
> last_reset field (we could choose to name it stats_reset too, leaving
> pg_stat_database.last_reset as the only anomaly) added as an implied
> behavior needed for such individualized resetting.  I would go with
> *.last_reset though and leave the absence of pg_stat_archiver.last_reset as
> the anomaly (or just add it redundantly for consistency).

It's not free to track more information. We always have the stats for the
whole system in memory at least once (stats collector currently, shared hash
table with shared memory stats patch), often more than that (stats accessing
backends).


> I don't see removing existing functionality as a good course to getting a
> consistent implementation; we should just push forward with figuring out
> what is missing and fill in those gaps.  At worst if that isn't something
> we want to fix right now our new setup should at least leave the status quo
> behaviors in place.

Well, it depends on whether there's an actual use case for those super fine
grained reset functions. Neither docs nor the thread introducing them
presented that.  We don't have SQL level stats
values either.

Greetings,

Andres Freund

On Wed, 2022-03-23 at 17:55 -0700, Andres Freund wrote:
> Starting with the below commit, pg_stat_reset_single_function_counters,
> pg_stat_reset_single_table_counters don't just reset the stats for the
> individual function, but also set pg_stat_database.stats_reset.

I see the point in the fine-grained reset, but I am -1 on having that
reset "pg_stat_database.stats_reset".  That would make the timestamp
mostly useless.

One could argue that resetting a single counter and *not* resetting
"pg_stat_database.stats_reset" would also be a lie, but at least it is
a smaller lie.

Yours,
Laurenz Albe

Hi,

On 2022-03-24 06:27:48 +0100, Laurenz Albe wrote:
> On Wed, 2022-03-23 at 17:55 -0700, Andres Freund wrote:
> > Starting with the below commit, pg_stat_reset_single_function_counters,
> > pg_stat_reset_single_table_counters don't just reset the stats for the
> > individual function, but also set pg_stat_database.stats_reset.
> 
> I see the point in the fine-grained reset, but I am -1 on having that
> reset "pg_stat_database.stats_reset".  That would make the timestamp
> mostly useless.

Just to be clear - that's the current and long-time behaviour.

Greetings,

Andres Freund

On 3/24/22 01:59, Andres Freund wrote:
> Hi,
> 
> On 2022-03-23 17:55:16 -0700, Andres Freund wrote:
>> Maybe I just don't understand what these reset functions are intended for?
>> Their introduction [3] didn't explain much either. To me the behaviour of
>> resetting pg_stat_database.stats_reset but nothing else in pg_stat_database
>> makes them kind of dangerous.
> 
> Forgot to add: At the very least we should document that weird behaviour,
> because it's certainly not obvious.  But imo we should either remove the
> behaviour or drop the functions.
> 

I agree it should have been documented, but I still stand behind the
current behavior. I'm not willing to die on this hill, but I think the
reasoning was/is sound.

Firstly, calculating transactions/second, reads/second just from by
looking at pg_stat_database data (counter and stat_reset) is nonsense.
It might work for short time periods, but for anything longer it's bound
to give you bogus results - you don't even know if the system was
running at all, and so on.

Secondly, to do anything really meaningful you need to calculate deltas,
and be able to detect if some of the stats were reset for the particular
interval. And the stat_reset timestamp was designed to be a simple way
to detect that (instead of having to inspect all individual timestamps).


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2022-03-24 13:12:24 +0100, Tomas Vondra wrote:
> I agree it should have been documented, but I still stand behind the
> current behavior. I'm not willing to die on this hill, but I think the
> reasoning was/is sound.
> 
> Firstly, calculating transactions/second, reads/second just from by
> looking at pg_stat_database data (counter and stat_reset) is nonsense.
> It might work for short time periods, but for anything longer it's bound
> to give you bogus results - you don't even know if the system was
> running at all, and so on.

It's not that you'd use it as the sole means of determining the time
delta. But using it to see if stats were reset between two samples of
pg_stat_database imo makes plenty sense.


> Secondly, to do anything really meaningful you need to calculate deltas,
> and be able to detect if some of the stats were reset for the particular
> interval. And the stat_reset timestamp was designed to be a simple way
> to detect that (instead of having to inspect all individual timestamps).

I wonder if we should just split that per-database timestamp into two. One
about the pg_stat_database contents, one about per-database stats? That
doesn't have the same memory-usage-increase concerns as adding
per-table/function reset stats.

Greetings,

Andres Freund

Hi,

On 2022-03-29 14:14:05 -0700, David G. Johnston wrote:
> On Tue, Mar 29, 2022 at 1:37 PM Andres Freund <andres@anarazel.de> wrote:
> 
> > > Secondly, to do anything really meaningful you need to calculate deltas,
> > > and be able to detect if some of the stats were reset for the particular
> > > interval. And the stat_reset timestamp was designed to be a simple way
> > > to detect that (instead of having to inspect all individual timestamps).
> >
> > I wonder if we should just split that per-database timestamp into two. One
> > about the pg_stat_database contents, one about per-database stats? That
> > doesn't have the same memory-usage-increase concerns as adding
> > per-table/function reset stats.
> >
> >
> That seems like only half a solution.  The reasoning for doing such a split
> for pg_stat_database is identical to the reason that new fields should be
> added to pg_stat_all_tables and pg_stat_user_functions (and possibly
> others).

Not really IMO. There's obviously the space usage aspect - there's always
fewer pg_stat_database stats than relation stats. But more importantly, a
per-relation/function reset field wouldn't address Tomas's concern: He wants a
single thing to check to see if any stats have been reset - and that's imo a
quite reasonable desire.


> pg_stat_all_tables already has 16 bigint fields, 4 timestamptz fields, 2
> names and an oid.  Seems like one more timestamptz field is a marginal
> increase whose presence lets us keep the already implemented per-table
> reset mechanism.  We should at least measure the impact that adding the
> field has before deciding its presence is too costly.

Because of the desire for a single place to check whether there has been a
reset within a database, that's imo an orthogonal debate.

Greetings,

Andres Freund

Hi,

On 2022-03-29 17:06:24 -0700, David G. Johnston wrote:
> On Tue, Mar 29, 2022 at 4:43 PM Andres Freund <andres@anarazel.de> wrote:
> > But more importantly, a
> > per-relation/function reset field wouldn't address Tomas's concern: He
> > wants a
> > single thing to check to see if any stats have been reset - and that's imo
> > a
> > quite reasonable desire.
> >
> 
> Per the original email:
> 
> "Starting with the below commit, pg_stat_reset_single_function_counters,
> pg_stat_reset_single_table_counters don't just reset the stats for the
> individual function, but also set pg_stat_database.stats_reset."
> 
> Thus we already have the desired behavior, it is just poorly documented.

The problem is that it also make stats_reset useless for other purposes -
which I do consider a problem. Hence this thread.  My concern would be
mollified if I there were a separate reset timestamp counting the last
"database wide" reset time. Your comment about that was something about
relation/function level timestamps, which doesn't seem relevant.


> Now, maybe other functions aren't doing this?  If so, given these functions
> do, we probably should just change any outliers to match.

Don't think there are other functions.

Greetings,

Andres Freund

On Wed, Mar 23, 2022 at 8:55 PM Andres Freund <andres@anarazel.de> wrote:
> This behaviour can be trivially (and is) implemented for the shared memory
> stats patch. But every time I read over that part of the code it feels just
> profoundly wrong to me.  Way worse than *not* resetting
> pg_stat_database.stats_reset.
>
> Anybody that uses the time since the stats reset as part of a calculation of
> transactions / sec, reads / sec or such will get completely bogus results
> after a call to pg_stat_reset_single_table_counters().

Sure, but that's unavoidable anyway. If some stats have been reset and
other stats have not, you can't calculate a meaningful average over
time unless you have a specific reset time for each statistic.

To me, the current behavior feels more correct than what you propose.
Imagine for example that you are looking for tables/indexes where the
counters are 0 as a way of finding unused objects. If you know that no
counters have been zeroed in a long time, you know that this is
reliable. But under your proposal, there's no way to know this. All
you know is that the entire system wasn't reset, and therefore some of
the 0s that you are seeing might be for individual objects that were
reset.

I think of this mechanism like as answering the question "When's the
last time anybody tinkered with this thing by hand?". If it's recent,
the tinkering has a good chance of being related to whatever problem
I'm trying to solve. If it's not, it's probably unrelated.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-03-30 14:57:25 -0400, Robert Haas wrote:
> On Wed, Mar 23, 2022 at 8:55 PM Andres Freund <andres@anarazel.de> wrote:
> > This behaviour can be trivially (and is) implemented for the shared memory
> > stats patch. But every time I read over that part of the code it feels just
> > profoundly wrong to me.  Way worse than *not* resetting
> > pg_stat_database.stats_reset.
> >
> > Anybody that uses the time since the stats reset as part of a calculation of
> > transactions / sec, reads / sec or such will get completely bogus results
> > after a call to pg_stat_reset_single_table_counters().
> 
> Sure, but that's unavoidable anyway. If some stats have been reset and
> other stats have not, you can't calculate a meaningful average over
> time unless you have a specific reset time for each statistic.

Individual pg_stat_database columns can't be reset independently. Other views
summarizing large parts of the system, like pg_stat_bgwriter, pg_stat_wal etc
have a stats_reset column that is only reset if their counters is also
reset. So the only reason we can't do that for pg_stat_database is that we
don't know since when pg_stat_database counters are counting.


> To me, the current behavior feels more correct than what you propose.
> Imagine for example that you are looking for tables/indexes where the
> counters are 0 as a way of finding unused objects. If you know that no
> counters have been zeroed in a long time, you know that this is
> reliable. But under your proposal, there's no way to know this. All
> you know is that the entire system wasn't reset, and therefore some of
> the 0s that you are seeing might be for individual objects that were
> reset.

My current proposal is to just have two reset times. One for the contents of
pg_stat_database (i.e. not affected by pg_stat_reset_single_*_counters()), and
one for stats within the entire database.


> I think of this mechanism like as answering the question "When's the
> last time anybody tinkered with this thing by hand?". If it's recent,
> the tinkering has a good chance of being related to whatever problem
> I'm trying to solve. If it's not, it's probably unrelated.

When I look at a database with a problem, I'll often look at pg_stat_database
to get a first impression of the type of workload running. The fact that
stats_reset doesn't reflect the age of other pg_stat_database columns makes
that much harder.

Greetings,

Andres Freund

My current proposal is to just have two reset times. One for the contents of
pg_stat_database (i.e. not affected by pg_stat_reset_single_*_counters()), and
one for stats within the entire database.

Hi,

On 2022-03-30 12:29:51 -0700, David G. Johnston wrote:
> On Wednesday, March 30, 2022, Andres Freund <andres@anarazel.de> wrote:
> > My current proposal is to just have two reset times. One for the contents
> > of
> > pg_stat_database (i.e. not affected by pg_stat_reset_single_*_counters()),
> > and
> > one for stats within the entire database.

> What IS it affected by?  And does whatever affects it affect anything else?

pg_stat_reset() resets the current database's stats. That includes the
database's row in pg_stat_database and all table and function stats.

Greetings,

Andres Freund