Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Add code comment explaining ins_since_vacuum and aborted inserts
- 530050d8d285 18.0 landed
Trigger autovacuum based on number of INSERTs
- b07642dbcd8d 13.0 cited

n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T16:57:10Z

Hi,

I came across what appears to be incorrect behavior in the
pg_stat_all_tables.n_ins_since_vacuum
counter after a rollback.

As shown below, the first two inserts of 1,000 rows were rolled back,
yet they are still counted toward
n_ins_since_vacuum.Consequently, they influence the vacuum insert
threshold calculation—even though
such rolled-back rows are dead tuples and should only affect the
vacuum threshold calculation.

Notice that the n_mod_since_analyze actually does the correct thing
here and does
not take into account the rolledback inserts.

Rollbacks are not common, so this may go unnoticed, but I think it should be
corrected, which means that only committed inserts should count
towards n_ins_since_vacuum.

the n_tup_ins|del|upd should continue to track both committed and rolledback
rows, but I also think the documentation [0] for these fields could be improved
to clarify this point, i.e. n_tup_ins should be documented as
"Total number of rows inserted, including those from aborted transactions"
instead of just "Total number of rows inserted"


```
DROP TABLE t;
CREATE TABLE t (id INT);
ALTER TABLE t SET (autovacuum_enabled = OFF);
BEGIN;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
ROLLBACK;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
SELECT
    n_tup_ins,
    n_tup_upd,
    n_tup_del,
    n_live_tup,
    n_dead_tup,
    n_mod_since_analyze,
    n_ins_since_vacuum
FROM
    pg_stat_all_tables
WHERE
    relname = 't';

 n_tup_ins | n_tup_upd | n_tup_del | n_live_tup | n_dead_tup |
n_mod_since_analyze | n_ins_since_vacuum
-----------+-----------+-----------+------------+------------+---------------------+--------------------
      5000 |         0 |         0 |       3000 |       2000 |
       3000 |               5000
(1 row)

```


Thoughts? before I prepare patches for this.


[0] https://www.postgresql.org/docs/current/monitoring-stats.html


--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

Euler Taveira <euler@eulerto.com> — 2025-04-09T18:07:19Z

On Wed, Apr 9, 2025, at 1:57 PM, Sami Imseih wrote:
> I came across what appears to be incorrect behavior in the
> pg_stat_all_tables.n_ins_since_vacuum
> counter after a rollback.

This is *not* an oversight. It is by design. See commit b07642dbcd8d. The
documentation says

  Estimated number of rows inserted since this table was last vacuumed

Those rows were actually inserted. They are physically in the data files. And
that's what matters for the autovacuum algorithm.

--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T18:31:55Z

> This is *not* an oversight. It is by design. See commit b07642dbcd8d. The
> documentation says

I don't see in the commit message why inserts in an aborted transaction
must count towards n_ins_since_vacuum.

>   Estimated number of rows inserted since this table was last vacuumed
>
> Those rows were actually inserted. They are physically in the data files. And
> that's what matters for the autovacuum algorithm.

Correct,but they are dead tuples that are physically in the files, and
are accounted for through
n_dead_tup and are then cleaned up based on the
autovacuum_vacuum_scale_factor|threshold
calculation.

The purpose of b07642dbcd8d is to trigger autovacuum for append only/mainly
workloads that don't generate dead tuples.

--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-09T18:49:07Z

On Wed, Apr 9, 2025 at 11:32 AM Sami Imseih <samimseih@gmail.com> wrote:

> The purpose of b07642dbcd8d is to trigger autovacuum for append only/mainly
> workloads that don't generate dead tuples.
>
>
Why does counting or not counting the dead tuples matter?  Forget original
purpose, is there presently a bug or not?  Between the two options the one
where we count dead tuples makes more sense on its face.

David J.

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T18:57:44Z

> Forget original purpose, is there presently a bug or not?

Yes, there is a bug. Accounting rows inserted as part of an aborted
transaction in
n_ins_since_vacuum is not correct, since the same rows are being
accounted for with n_dead_tup.

> Between the two options the one where we count dead tuples makes more sense on its face.

IIUC, I am saying the same thing, if an inserted row is rolled back,
it should only count as a
dead tuple (n_dead_tup) only, and not in n_ins_since_vacuum.

--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

Mark Dilger <mark.dilger@enterprisedb.com> — 2025-04-09T19:22:48Z

>
> Yes, there is a bug. Accounting rows inserted as part of an aborted
> transaction in
> n_ins_since_vacuum is not correct, since the same rows are being
> accounted for with n_dead_tup.
>

If I create a table with autovacuum_enabled=false, insert rows (some of
which abort), and check the stats, surely the n_ins_tup and the
n_ins_since_vacuum should be the same, because all the insertions (however
we count them) have happened since the nonexistent last vacuum:

    CREATE TABLE n_insert_test (
        i   INTEGER NOT NULL PRIMARY KEY
    ) WITH (autovacuum_enabled = false);
    INSERT INTO n_insert_test (i) VALUES (1);
    INSERT INTO n_insert_test
        (SELECT 1 FROM generate_series(1,100000))
        ON CONFLICT
        DO NOTHING;
    SELECT pg_sleep(1);
     pg_sleep
    ----------

    (1 row)

    SELECT n_live_tup, n_dead_tup, n_tup_ins, n_ins_since_vacuum
        FROM pg_stat_all_tables
        WHERE relname = 'n_insert_test';
     n_live_tup | n_dead_tup | n_tup_ins | n_ins_since_vacuum
    ------------+------------+-----------+--------------------
              1 |          0 |         1 |                  1
    (1 row)

    INSERT INTO n_insert_test
        (SELECT 2 FROM generate_series(1,100000))
        ON CONFLICT
        DO NOTHING;
    SELECT pg_sleep(1);
     pg_sleep
    ----------

    (1 row)

    SELECT n_live_tup, n_dead_tup, n_tup_ins, n_ins_since_vacuum
        FROM pg_stat_all_tables
        WHERE relname = 'n_insert_test';
     n_live_tup | n_dead_tup | n_tup_ins | n_ins_since_vacuum
    ------------+------------+-----------+--------------------
              2 |          0 |         2 |                  2
    (1 row)

    BEGIN;
    INSERT INTO n_insert_test
        (SELECT * FROM generate_series(3,100000));
    ROLLBACK;
    SELECT pg_sleep(1);
     pg_sleep
    ----------

    (1 row)

    SELECT n_live_tup, n_dead_tup, n_tup_ins, n_ins_since_vacuum
        FROM pg_stat_all_tables
        WHERE relname = 'n_insert_test';
     n_live_tup | n_dead_tup | n_tup_ins | n_ins_since_vacuum
    ------------+------------+-----------+--------------------
              2 |      99998 |    100000 |             100000
    (1 row)

If we went with your suggestion, I think the final n_ins_since_vacuum
column would be 2.  Do you think the n_tup_ins should also be 2?  Should
those two columns differ?  If so, why?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-09T19:31:03Z

On Wed, Apr 9, 2025, 11:57 Sami Imseih <samimseih@gmail.com> wrote:

> > Forget original purpose, is there presently a bug or not?
>
> Yes, there is a bug. Accounting rows inserted as part of an aborted
> transaction in
> n_ins_since_vacuum is not correct, since the same rows are being
> accounted for with n_dead_tup.
>

So why is it important we not account for the aborted insert in both
n_ins_since_vacuum and n_dead_tup?

When would you ever add them together so that an actual double-counting
would reflect in some total.

You aren't upset that n_live_tup and this both include the non-aborted
inserts.

David J.

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T19:39:32Z

> If we went with your suggestion, I think the final n_ins_since_vacuum column would be 2.  Do you think the n_tup_ins should also be 2?

n_ins_since_vacuum should be 2 and n_tup_ins should be 100000.

A user tracks how many inserts they performed with n_tup_ins
to measure load/activity on the database. It's important to also
include aborted transactions in this metric,

n_ins_since_vacuum however is not used to measure database activity,
but is used to drive autovacuum decisions. So, it has a different purpose.

> Should those two columns differ?  If so, why?

They will differ because n_tup_ins keeps increasing, while n_ins_since_vacuum is
reset after a vacuum. The issue I see is that n_ins_since_vacuum should only
reflect the number of newly inserted rows that are eligible for
freezing, as described
in pgstat_report_vacuum [0]

[0] https://github.com/postgres/postgres/blob/master/src/backend/utils/activity/pgstat_relation.c#L238-L247


--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T19:56:25Z

> So why is it important we not account for the aborted insert in both n_ins_since_vacuum and n_dead_tup?

I am saying n_dead_tup should continue to account for n_dead_tup. I am
not saying it should not.
What I am saying is n_ins_since_vacuum should not account for aborted inserts.

> When would you ever add them together so that an actual double-counting would reflect in some total.

I would never add them together. n_ins_since_vacuum uses this value
for vacuuming purposes.

> You aren't upset that n_live_tup and this both include the non-aborted inserts.

n_live_tup only shows the non-aborted inserts. I will put a better formatted
version of the repro below. IN the exampleI have 2k dead tuples from the rolled
back transactions and 3k live tuples from the committed transactions.

```
DROP TABLE t;
CREATE TABLE t (id INT);
ALTER TABLE t SET (autovacuum_enabled = OFF);
BEGIN;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
ROLLBACK;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
INSERT INTO t SELECT n FROM generate_series(1, 1000) AS n;
\x
SELECT
    n_tup_ins,
    n_tup_upd,
    n_tup_del,
    n_live_tup,
    n_dead_tup,
    n_mod_since_analyze,
    n_ins_since_vacuum
FROM
    pg_stat_all_tables
WHERE
    relname = 't';

-[ RECORD 1 ]-------+-----
n_tup_ins           | 5000
n_tup_upd           | 0
n_tup_del           | 0
n_live_tup          | 3000
n_dead_tup          | 2000
n_mod_since_analyze | 3000
n_ins_since_vacuum  | 5000
```


--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

Mark Dilger <mark.dilger@enterprisedb.com> — 2025-04-09T19:58:32Z

> The issue I see is that n_ins_since_vacuum should only
> reflect the number of newly inserted rows that are eligible for
> freezing, as described
> in pgstat_report_vacuum [0]
>
> [0]
> https://github.com/postgres/postgres/blob/master/src/backend/utils/activity/pgstat_relation.c#L238-L247


For the archives, that paragraph reads:

 /*
* It is quite possible that a non-aggressive VACUUM ended up skipping
* various pages, however, we'll zero the insert counter here regardless.
* It's currently used only to track when we need to perform an "insert"
* autovacuum, which are mainly intended to freeze newly inserted tuples.
* Zeroing this may just mean we'll not try to vacuum the table again
* until enough tuples have been inserted to trigger another insert
* autovacuum.  An anti-wraparound autovacuum will catch any persistent
* stragglers.
*/

What work do you believe the word "mainly" does in that paragraph?  The
presence of the word "mainly" rather than "only" somewhat cuts against your
argument that we should only be counting tuples that get inserted without
aborting.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-09T20:13:34Z

On Wed, Apr 9, 2025, 12:56 Sami Imseih <samimseih@gmail.com> wrote:

>
> What I am saying is n_ins_since_vacuum should not account for aborted
> inserts.
>

It does and from what I can see it should.  You need to explain why it
should not.  More importantly, convincingly enough to change five year old
behavior.

You should get away from showing counts and talk about system behavior.
Or, if this is accounting related, explain the math.

David J.

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-09T20:19:12Z

On Wed, Apr 9, 2025, 12:39 Sami Imseih <samimseih@gmail.com> wrote:

>
> They will differ because n_tup_ins keeps increasing, while
> n_ins_since_vacuum is
> reset after a vacuum. The issue I see is that n_ins_since_vacuum should
> only
> reflect the number of newly inserted rows that are eligible for
> freezing, as described
> in pgstat_report_vacuum [0]
>

Vacuuming them into oblivion is a form of freezing.  It also removes the
aging xid from the table.

David J.

>

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T20:30:24Z

>> What I am saying is n_ins_since_vacuum should not account for aborted inserts.
>
> It does and from what I can see it should.  You need to explain why it should not.  More importantly, convincingly enough to change five year old behavior.

n_ins_since_vacuum was introduced to trigger autovacuum based on the #
of inserts
committed, and does not care about about dead tuples in this formula.

If I have a transaction that rolledback an insert of a million rows,
I expect autovacuum to kick in based on the fact there are now 1 million
n_dead_tup. n_ins_since_vacuumm is not relevant to the formula
for this case.

In other words, the reason n_ins_since_vacuum was introduced is to freeze
(committed) rows, so it should not need to track dead rows to do what it intends
to do.

--
Sami Imseih

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-09T21:01:10Z

On Wed, Apr 9, 2025 at 1:30 PM Sami Imseih <samimseih@gmail.com> wrote:

> >> What I am saying is n_ins_since_vacuum should not account for aborted
> inserts.
> >
> > It does and from what I can see it should.  You need to explain why it
> should not.  More importantly, convincingly enough to change five year old
> behavior.
>
> n_ins_since_vacuum was introduced to trigger autovacuum based on the #
> of inserts
> committed, and does not care about about dead tuples in this formula.
>
> If I have a transaction that rolledback an insert of a million rows,
> I expect autovacuum to kick in based on the fact there are now 1 million
> n_dead_tup. n_ins_since_vacuumm is not relevant to the formula
> for this case.
>
> In other words, the reason n_ins_since_vacuum was introduced is to freeze
> (committed) rows, so it should not need to track dead rows to do what it
> intends
> to do.
>
>
Well, then the authors failed to implement what they intended.  Which seems
unlikely, but even accepting that to be true, you still haven't shown that
the current behavior is undesirable.  Just unexpected.  Which means we
haven't documented this well enough so people reading the docs/code get an
expectation that matches reality.

So, please, go ahead with some documentation/code comment changes.  But,
for my part, the existing behavior seems quite reasonable: "yes, please,
get rid of my bloat on this otherwise infrequently updated table"; thus
leave the existing behavior and the tracking alone.

David J.

Re: n_ins_since_vacuum stats for aborted transactions

Mark Dilger <mark.dilger@enterprisedb.com> — 2025-04-09T21:23:18Z

>
> In other words, the reason n_ins_since_vacuum was introduced is to freeze
> (committed) rows, so it should not need to track dead rows to do what it
> intends
> to do.
>

Wouldn't that result in the rather strange behavior that 1 million dead
rows might trigger a vacuum due to one threshold, 1 million inserted live
rows might trigger a vacuum due to another threshold, while half a million
dead plus half a million live fails to meet either threshold and fails to
trigger a vacuum?  What is the use case for that behavior?  Perhaps you
have one, but until you make it explicit, it is hard for others to get
behind your proposal.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-09T21:48:24Z

On Wednesday, April 9, 2025, Sami Imseih <samimseih@gmail.com> wrote:

>
> In other words, the reason n_ins_since_vacuum was introduced is to freeze
> (committed) rows, so it should not need to track dead rows to do what it
> intends
> to do.
>

n_ins_since_vacuum was introduced to indicate how many tuples a vacuum
would touch on an insert-only table should vacuum be run now.  Autovacuum
uses this value when determining whether a given relation should be
vacuumed.

David J.

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T22:05:39Z

On Wed, Apr 9, 2025 at 4:23 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>>
>> In other words, the reason n_ins_since_vacuum was introduced is to freeze
>> (committed) rows, so it should not need to track dead rows to do what it intends
>> to do.
>
>
> Wouldn't that result in the rather strange behavior that 1 million dead rows might trigger a vacuum due to one threshold,
> 1 million inserted live rows might trigger a vacuum due to another threshold,
> while half a million dead plus half a million live fails to meet either threshold and fails to trigger a vacuum?

Vacuum works based on different thresholds already, right? A user is
able to configure different thresholds:
autovacuum_vacuum_scale_factor|threshold
autovacuum_vacuum_insert_scale_factor|threshold

> What is the use case for that behavior?  Perhaps you have one, but until you make it explicit, it is hard for others to get behind your proposal.

The point is to ensure that the pg_stats fields that autovacuum uses
are supplied the correct values
for the different thresholds they need to calculate, as described here [0]


[0] https://www.postgresql.org/message-id/CAA5RZ0uDyGW1omWqWkxyW8NB1qzsKmXhnoMtzTBeRzSd4DMatQ%40mail.gmail.com

--
Sami Imseih

Re: n_ins_since_vacuum stats for aborted transactions

Andres Freund <andres@anarazel.de> — 2025-04-09T22:16:14Z

Hi,

On 2025-04-09 17:05:39 -0500, Sami Imseih wrote:
> On Wed, Apr 9, 2025 at 4:23 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> >>
> >> In other words, the reason n_ins_since_vacuum was introduced is to freeze
> >> (committed) rows, so it should not need to track dead rows to do what it intends
> >> to do.
> >
> >
> > Wouldn't that result in the rather strange behavior that 1 million dead rows might trigger a vacuum due to one threshold,
> > 1 million inserted live rows might trigger a vacuum due to another threshold,
> > while half a million dead plus half a million live fails to meet either threshold and fails to trigger a vacuum?
> 
> Vacuum works based on different thresholds already, right? A user is
> able to configure different thresholds:
> autovacuum_vacuum_scale_factor|threshold
> autovacuum_vacuum_insert_scale_factor|threshold
> 
> > What is the use case for that behavior?  Perhaps you have one, but until you make it explicit, it is hard for others to get behind your proposal.
> 
> The point is to ensure that the pg_stats fields that autovacuum uses
> are supplied the correct values
> for the different thresholds they need to calculate, as described here [0]

You so far have not outlined a single scenario where the current behaviour
causes an issue.

Greetings,

Andres Freund

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T22:23:43Z

>> >> What I am saying is n_ins_since_vacuum should not account for aborted inserts.
>> >
>> > It does and from what I can see it should.  You need to explain why it should not.  More importantly, convincingly enough to change five year old behavior.
>>
>> n_ins_since_vacuum was introduced to trigger autovacuum based on the #
>> of inserts
>> committed, and does not care about about dead tuples in this formula.
>>
>> If I have a transaction that rolledback an insert of a million rows,
>> I expect autovacuum to kick in based on the fact there are now 1 million
>> n_dead_tup. n_ins_since_vacuumm is not relevant to the formula
>> for this case.
>>
>> In other words, the reason n_ins_since_vacuum was introduced is to freeze
>> (committed) rows, so it should not need to track dead rows to do what it intends
>> to do.
>>
>
> Well, then the authors failed to implement what they intended.  Which seems unlikely, but even accepting that to be true,

At least, the reasoning behind this is neither explained in code comments or
in public docs.

> you still haven't shown that the current behavior is undesirable.  Just unexpected.

It's not expected for sure.

As far as being undesirable, I don't think it is because rollbacks are not
that common and this is not preventing or delaying autovacuum.

I do think it's probably a good idea to add code comment that it is OK
to include
dead tuples from a aborted insert into the n_ins_since_vacuum counter,
for the above reasons.

I will also update the public documentation for the counters to mention that
they include rows from aborted transactions.


--
Sami Imseih

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-09T22:30:18Z

On Wednesday, April 9, 2025, Sami Imseih <samimseih@gmail.com> wrote:

>
> > What is the use case for that behavior?  Perhaps you have one, but until
> you make it explicit, it is hard for others to get behind your proposal.
>
> The point is to ensure that the pg_stats fields that autovacuum uses
> are supplied the correct values
> for the different thresholds they need to calculate, as described here [0]
>
>
> [0] https://www.postgresql.org/message-id/CAA5RZ0uDyGW1omWqWkxyW8NB1qzsK
> mXhnoMtzTBeRzSd4DMatQ%40mail.gmail.com
>
>
Except there isn’t some singular provably correct value here.  Today’s
behavior (considering dead tuples) is not intrinsically wrong nor correct,
and neither is what you propose (ignoring the dead tuples).  The fact that
those dead tuples get removed regardless is a point in favor of counting
them when deciding what to do.  And it’s also the long-standing behavior.
You need to make a compelling argument to change to your preference.

Inserting aborted dead tuples moves the counter closer to both autovacuum
thresholds.  There is no reason why that should be prohibited.  I can see
the argument for why one threshold should be dead tuples only and the other
live tuples only - but I don’t favor that design.

David J.

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-09T22:53:56Z

> Except there isn’t some singular provably correct value here.  Today’s behavior (considering dead tuples)
> is not intrinsically wrong nor correct, and neither is what you propose (ignoring the dead tuples).
> The fact that those dead tuples get removed regardless is a point in favor of counting them when deciding what to do.
> And it’s also the long-standing behavior.  You need to make a compelling argument to change to your preference.
>
> Inserting aborted dead tuples moves the counter closer to both autovacuum thresholds.
> There is no reason why that should be prohibited.  I can see the argument for why one
> threshold should be dead tuples only and the other live tuples only - but I don’t favor that design.

Fair enough, and I think I got my answers from this thread. This
design decision was not
discussed in the thread for b07642dbcd8.

So, I think public documentation updates to clarify that
n_tup_upd|del|ins, and n_ins_since_vacuum include
aborted rows is a good enhancement.

Also a code comment in pgstat_relation_flush_cb that explains
ins_since_vacuum intentionally includes aborted inserts.

tabentry->ins_since_vacuum += lstats->counts.tuples_inserted;

--
Sami Imseih

Re: n_ins_since_vacuum stats for aborted transactions

David Rowley <dgrowleyml@gmail.com> — 2025-04-10T01:48:24Z

On Thu, 10 Apr 2025 at 10:54, Sami Imseih <samimseih@gmail.com> wrote:
> Fair enough, and I think I got my answers from this thread. This
> design decision was not
> discussed in the thread for b07642dbcd8.

This discussion is mostly settled down now, but...

I also went through that thread to see if it was mentioned and saw
nothing about it.

One possible bad behaviour that this could cause is if
autovacuum_vacuum_insert_scale_factor was set lower than
autovacuum_vacuum_scale_factor is that we could end up performing a
vacuum for inserts when all we've got is dead tuples from aborted
inserts. That does not seem terrible. It's just an extra autovacuum
that could happen in a not very common case.  We could fix it but it
would require adding a new field to PgStat_TableCounts to track this
as you can't selectively only update
PgStat_TableCounts.tuples_inserted on commit as the n_tup_ins would
then be wrong.

If the above is the only misbehaviour this is causing, then I'm
doubting that it's worth expanding PgStat_TableCounts by 8 bytes to
have this work better.

> So, I think public documentation updates to clarify that
> n_tup_upd|del|ins, and n_ins_since_vacuum include
> aborted rows is a good enhancement.

A code comment might help settle future debates. If you'd arrived from
a user perspective and were confused about this, then maybe that would
warrant something going into the docs. On the other hand, if you have
a suggestion, please put it into patch form.

I've attached a small patch which adds a code comment about this,
which might save a future discussion.

David

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-10T03:42:31Z

> One possible bad behaviour that this could cause is if
> autovacuum_vacuum_insert_scale_factor was set lower than
> autovacuum_vacuum_scale_factor is that we could end up performing a
> vacuum for inserts when all we've got is dead tuples from aborted
> inserts.

Thanks for pointing this out!

> We could fix it but it
> would require adding a new field to PgStat_TableCounts to track this
> as you can't selectively only update
> PgStat_TableCounts.tuples_inserted on commit as the n_tup_ins would
> then be wrong.
>
> If the above is the only misbehaviour this is causing, then I'm
> doubting that it's worth expanding PgStat_TableCounts by 8 bytes to
> have this work better.

I agree, as I mentioned at the top of the thread, rollbacks are
not common enough for this to be worth it. But, the code comment
as you have it is good enough.

> > So, I think public documentation updates to clarify that
> > n_tup_upd|del|ins, and n_ins_since_vacuum include
> > aborted rows is a good enhancement.

I also attached public doc clarification of how aborted ( and committed )
rows are handled for pg_stat_all_tables fields that count row changes
I initially thought
about adding clarification for every field, but that felt too
repetitive. So, I add a Note
section after pg_stat_all_tables in the public docs to describe the behavior.
It may be better to apply your code comment patch and the public
docs patch separately.

--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-10T14:01:05Z

I created an entry for the July CF
https://commitfest.postgresql.org/patch/5691/

... and I realized I forgot to include David's code comment patch yesterday,
Reattaching both patches.


--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

David Rowley <dgrowleyml@gmail.com> — 2025-04-10T23:45:57Z

On Fri, 11 Apr 2025 at 02:01, Sami Imseih <samimseih@gmail.com> wrote:
> I created an entry for the July CF
> https://commitfest.postgresql.org/patch/5691/
>
> ... and I realized I forgot to include David's code comment patch yesterday,
> Reattaching both patches.

I've pushed the code comment patch.

For the docs patch, the fact you're having to name specific fields in
the note at the bottom makes me think the details should be added to
the row for the field in question instead.

David

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-11T00:38:05Z

> On Fri, 11 Apr 2025 at 02:01, Sami Imseih <samimseih@gmail.com> wrote:
> > I created an entry for the July CF
> > https://commitfest.postgresql.org/patch/5691/
> >
> > ... and I realized I forgot to include David's code comment patch yesterday,
> > Reattaching both patches.
>
> I've pushed the code comment patch.
>
> For the docs patch, the fact you're having to name specific fields in
> the note at the bottom makes me think the details should be added to
> the row for the field in question instead.

Here is how per line will look like. Either way is fine with me.
with v2, I also did add the clarification in the pg_stat_database.tup_inserted|
updated|deleted fields as well.

-- 
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-11T01:37:40Z

On Thu, Apr 10, 2025 at 5:38 PM Sami Imseih <samimseih@gmail.com> wrote:

> > On Fri, 11 Apr 2025 at 02:01, Sami Imseih <samimseih@gmail.com> wrote:
> > > I created an entry for the July CF
> > > https://commitfest.postgresql.org/patch/5691/
> > >
> > > ... and I realized I forgot to include David's code comment patch
> yesterday,
> > > Reattaching both patches.
> >
> > I've pushed the code comment patch.
> >
> > For the docs patch, the fact you're having to name specific fields in
> > the note at the bottom makes me think the details should be added to
> > the row for the field in question instead.
>
> Here is how per line will look like. Either way is fine with me.
> with v2, I also did add the clarification in the
> pg_stat_database.tup_inserted|
> updated|deleted fields as well.
>
>
I'm learning toward a hybrid.  At the top:

"The tuple counters below, except where noted, are incremented even if the
transaction aborts."

(We can leave it unwritten that if the description says live or dead tuples
that qualifies as otherwise noted.)

The only exception I see that we need to note is:  n_mod_since_analyze;
which I would explain as opposed to simply noting the oddity.

"Estimated number of rows modified since this table was last analyzed.
Aborted transactions are ignored here since they will not cause the
statistics computed by analyze to change."

So, here are the relevant counters, with their treatment of aborted
transaction tuples:

seq_tup_read - says live
idx_tup_fetch - says live
n_tup_ins - default notice
n_tup_upd - default notice
n_tup_del - default notice
n_tup_hot_upd - default notice (is this correct?)
n_tup_newpage_upd - default notice (is this correct?)
n_live_tup - says live (is this a counter?)
n_dead_tup - says dead (is this a counter?)
n_mod_since_analyze - inline reason for non-default
n_ins_since_vacuum - default notice

I'm also thinking to reword n_tup_upd, something like:

Total number of rows updated.  Subsets of these updates are also tracked in
n_tup_hot_upd and n_tup_newpage_upd to facilitate performance monitoring.

David J.

P.S. I'm trying to understand what it means for rows from aborted deletes
to be counted (or not...).

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-11T19:33:35Z

I spent some time thinking about this today.

> "The tuple counters below, except where noted, are incremented even if the transaction aborts."

I like this idea, and I think it fits good as a blurb under "27.2.2.
Viewing Statistics"

I suggest a slight re-write however.

+  <para>
+   An aborted transaction will also increment tuple-related counters,
unless otherwise noted.
+  </para>

> So, here are the relevant counters, with their treatment of aborted transaction tuples:
>
> seq_tup_read - says live
> idx_tup_fetch - says live
> n_tup_ins - default notice
> n_tup_upd - default notice
> n_tup_del - default notice
> n_mod_since_analyze - inline reason for non-default
> n_ins_since_vacuum - default notice

All the counters mentioned above will increment the number of rows
modified/accessed even in the case of an aborted transaction, except
for n_mod_since_analyze.

> n_live_tup - says live (is this a counter?)
> n_dead_tup - says dead (is this a counter?)

They are not values that are purely incremental. They are incremented
by insert/update/delete for committed transactions, but are also
updated
by VACUUM or VACUUM FULL. So, these will need some inlined description
of their behavior,

> I'm also thinking to reword n_tup_upd, something like:
>
> Total number of rows updated.  Subsets of these updates are also tracked in n_tup_hot_upd and n_tup_newpage_upd to facilitate performance monitoring.

I think the current explanation is clear enough, I am also not too
thrilled about the "...to facilitate performance monitoring." since
the cumulative stats system
as a whole is known to be used to facilitate perf monitoring.

What do you think of the attached?

--
Sami Imseih
Amazon Web Services (AWS)



--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-11T22:03:59Z

On Fri, Apr 11, 2025 at 12:33 PM Sami Imseih <samimseih@gmail.com> wrote:

>
> > I'm also thinking to reword n_tup_upd, something like:
> >
> > Total number of rows updated.  Subsets of these updates are also tracked
> in n_tup_hot_upd and n_tup_newpage_upd to facilitate performance monitoring.
>
> I think the current explanation is clear enough, I am also not too
> thrilled about the "...to facilitate performance monitoring." since
> the cumulative stats system
> as a whole is known to be used to facilitate perf monitoring.
>

Yeah, it was mostly a style thing - I was trying to avoid using
parentheses, but the existing does make the needed point.


> What do you think of the attached?
>
>
WFM.  Though is there a reason to avoid adding the "why" of the exception
for n_mod_since_analyze?

David J.

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-12T00:18:55Z

> WFM.  Though is there a reason to avoid adding the "why" of the exception for n_mod_since_analyze?

I did think about that. I thought it will be understood that since
this is a field that deals with ANALYZE,
it will be understood that only committed changes matter here, and not
worth adding more text to the
description. but, maybe it's worth it?

--
Sami

Re: n_ins_since_vacuum stats for aborted transactions

David G. Johnston <david.g.johnston@gmail.com> — 2025-04-12T01:13:45Z

On Fri, Apr 11, 2025 at 5:19 PM Sami Imseih <samimseih@gmail.com> wrote:

> > WFM.  Though is there a reason to avoid adding the "why" of the
> exception for n_mod_since_analyze?
>
> I did think about that. I thought it will be understood that since
> this is a field that deals with ANALYZE,
> it will be understood that only committed changes matter here, and not
> worth adding more text to the
> description. but, maybe it's worth it?
>
>
Absent field questions I'm content to think it is sufficiently obvious or
discoverable for others.

David J.

Re: n_ins_since_vacuum stats for aborted transactions

David Rowley <dgrowleyml@gmail.com> — 2025-04-18T04:13:02Z

On Sat, 12 Apr 2025 at 07:33, Sami Imseih <samimseih@gmail.com> wrote:
> What do you think of the attached?

I looked at the v3 patch and I'm having trouble getting excited about it.

I'd say this part is misleading:

@@ -3956,7 +3961,8 @@ description | Waiting for a newly initialized
WAL file to reach durable storage
        <structfield>n_dead_tup</structfield> <type>bigint</type>
       </para>
       <para>
-       Estimated number of dead rows
+       Estimated number of dead rows (updated by committed transactions, or by
+       <command>VACUUM</command> and <command>VACUUM FULL</command>)
       </para></entry>

An aborted insert will contribute to that counter, but that's not mentioned.

Would it be ok just to adjust n_mod_since_analyze's "Estimated number
of rows modified since this table was last analyzed" and inject "by
committed transactions" after "modified", then call it a day?

David

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-18T19:05:04Z

On Thu, Apr 17, 2025 at 11:13 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sat, 12 Apr 2025 at 07:33, Sami Imseih <samimseih@gmail.com> wrote:
> > What do you think of the attached?
>
> I looked at the v3 patch and I'm having trouble getting excited about it.
>
> I'd say this part is misleading:
>
> @@ -3956,7 +3961,8 @@ description | Waiting for a newly initialized
> WAL file to reach durable storage
>         <structfield>n_dead_tup</structfield> <type>bigint</type>
>        </para>
>        <para>
> -       Estimated number of dead rows
> +       Estimated number of dead rows (updated by committed transactions, or by
> +       <command>VACUUM</command> and <command>VACUUM FULL</command>)
>        </para></entry>
>
> An aborted insert will contribute to that counter, but that's not mentioned.

I thought the first line "An aborted transaction will also increment
tuple-related counters, unless otherwise noted."
makes it clear that it will be updated in an aborted transaction, but
after re-reading
I can see it being confusing. What about this wording to make it more
clear when the field is
updated?


        <structfield>n_dead_tup</structfield> <type>bigint</type>
       </para>
       <para>
-       Estimated number of dead rows
+       Estimated number of dead rows (updated either by committed or
aborted transactions,
+       or by <command>VACUUM</command> and <command>VACUUM FULL</command>)
       </para></entry>
      </row>

> Would it be ok just to adjust n_mod_since_analyze's "Estimated number
> of rows modified since this table was last analyzed" and inject "by
> committed transactions" after "modified", then call it a day?

That works also.

--
Sami Imseih
Amazon Web Services (AWS)

Re: n_ins_since_vacuum stats for aborted transactions

Sami Imseih <samimseih@gmail.com> — 2025-04-18T19:41:21Z

> I can see it being confusing. What about this wording to make it more
> clear when the field is
> updated?

here are both of the changes in v4.

--
Sami Imseih
Amazon Web Services (AWS)