Обсуждение: obsolete autovacuum comment

While working on my autovacuum scheduling patch [0], I noticed the
following comment in relation_needs_vacanalyze():

        /*
         * Note that we don't need to take special consideration for stat
         * reset, because if that happens, the last vacuum and analyze counts
         * will be reset too.
         */

AFAICT this is a relic from pg_autovacuum (the contrib module and
predecessor to modern autovacuum) [1] and early versions of the autovacuum
patch [2] [3].  After digging through the archives [4], I think this is
referring to some stats that had to be managed separately but weren't ever
actually committed.

Any objections to removing this comment?

[0] https://postgr.es/m/flat/aOaAuXREwnPZVISO%40nathan
[1] https://postgr.es/c/9243664
[2] https://postgr.es/m/attachment/82361/autovacuum-5.patch
[3] https://postgr.es/m/attachment/82371/autovacuum-6.patch
[4] https://postgr.es/m/flat/20050706234302.GA20398%40alvh.no-ip.org

-- 
nathan

On Mon, Nov 10, 2025 at 02:03:27PM -0600, Nathan Bossart wrote:
> While working on my autovacuum scheduling patch [0], I noticed the
> following comment in relation_needs_vacanalyze():
> 
>         /*
>          * Note that we don't need to take special consideration for stat
>          * reset, because if that happens, the last vacuum and analyze counts
>          * will be reset too.
>          */
> 
> AFAICT this is a relic from pg_autovacuum (the contrib module and
> predecessor to modern autovacuum) [1] and early versions of the autovacuum
> patch [2] [3].  After digging through the archives [4], I think this is
> referring to some stats that had to be managed separately but weren't ever
> actually committed.

Patch attached.

> Any objections to removing this comment?

If none materialize, I will proceed with removal in a couple of days.

-- 
nathan

On Wed, 12 Nov 2025 at 09:35, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Nov 10, 2025 at 02:03:27PM -0600, Nathan Bossart wrote:
> > Any objections to removing this comment?
>
> If none materialize, I will proceed with removal in a couple of days.

I've never been able to make sense of that comment. I tried to dig a
bit yesterday and still couldn't make sense of it in the context of
the commit when it was added.  I assume it's talking about
pg_stat_all_tables.autovacuum_count, but I can't think why that
matters as I didn't find any code that took that into account for
triggering autovacuum.

It does look like Tom wrote it new for 29094193f rather than copied it
from somewhere, as those words didn't exist in the pg_autovacuum
contrib module as of that commit.

David

On Wed, Nov 12, 2025 at 09:52:49AM +1300, David Rowley wrote:
> I've never been able to make sense of that comment. I tried to dig a
> bit yesterday and still couldn't make sense of it in the context of
> the commit when it was added.  I assume it's talking about
> pg_stat_all_tables.autovacuum_count, but I can't think why that
> matters as I didn't find any code that took that into account for
> triggering autovacuum.

pg_autovacuum used to have this:

      /* If the stats collector is reporting fewer updates then we have on record
         then the stats were probably reset, so we need to reset also */
      if ((tbl->curr_analyze_count < tbl->CountAtLastAnalyze) ||
          (tbl->curr_vacuum_count < tbl->CountAtLastVacuum))
      {
        tbl->CountAtLastAnalyze = tbl->curr_analyze_count;
        tbl->CountAtLastVacuum = tbl->curr_vacuum_count;
      }

I think this was translated to the following in earlier versions of the
autovacuum patch:

+     /*
+      * Consider stat-reset: if the differences are negative, reset the
+      * last-seen counters.
+      *
+      * It's OK if tab->operation is rewritten later to VacuumAnalyze or
+      * Analyze, because these operations take care of updating
+      * pg_autovacuum as needed.
+      *
+      * We reset the counters to 0, not their current value, because that's
+      * what stat-reset did reset them to.  So we will operate on them
+      * closer to the reality (this is like saying they were last vacuumed
+      * at the same time the stat-reset happened.)  Also use these numbers
+      * to determine whether to vacuum on this iteration.
+      */
+     if (vactuples - avForm->vac_last_cnt < 0)
+     {
+         tab->operation = AVOperUpdateCounts;
+         tab->reltuples = 0;
+         tab->update_vactuples = true;
+         vac_last_cnt = 0;
+     }
+     else
+         vac_last_cnt = avForm->vac_last_cnt;

This stuff seems to have been ultimately replaced with the comment in
question.  AFAICT that was briefly discussed here [0].  So, my
interpretation is that this was referring a separately maintained "last
count" of the number of updated/deleted tuples for the table, which was
abandoned in favor of keeping track of the number of dead tuples in the
stats system.  But I'm not super confident in this analysis.

[0] https://postgr.es/m/20050712233455.GB15464%40alvh.no-ip.org

-- 
nathan

On 2025-Nov-10, Nathan Bossart wrote:

> While working on my autovacuum scheduling patch [0], I noticed the
> following comment in relation_needs_vacanalyze():
> 
>         /*
>          * Note that we don't need to take special consideration for stat
>          * reset, because if that happens, the last vacuum and analyze counts
>          * will be reset too.
>          */
> 
> AFAICT this is a relic from pg_autovacuum (the contrib module and
> predecessor to modern autovacuum) [1] and early versions of the autovacuum
> patch [2] [3].

Hmm, I think it's yes and no.  contrib/pg_autovacuum, as introduced in
commit bd18c50ba87f [1], includes the comment about stats reset when
considering whether to vacuum or analyze:

+                /* If the stats collector is reporting fewer updates then we have on record
+                then the stats were probably reset, so we need to reset also */
+                if((numInserts < tbl->InsertsAtLastAnalyze)||(numDeletes < tbl->DeletesAtLastVacuum))
+                {
+                  tbl->InsertsAtLastAnalyze=numInserts;
+                  tbl->DeletesAtLastVacuum=numDeletes;
+                }

The comment you cite appears more or less in the same place in
autovacuum-6.patch (the routine was called test_rel_for_autovac there),
but I think what it really is about, is the AVOperUpdateCounts operation
that was in autovacuum-5.patch.  Note that that older patch was
introducing a new pg_autovacuum catalog which kept track of tuple counts
since last analyze/vacuum, and those counts needed to be updated in case
pgstats was reset.  But by patch autovacuum-6.patch this had disappeared
(the counts were now in pgstats, per review from Tom), so we didn't need
to reset any counters, and the AVOperUpdateCounts operation went away.

It's quite possible that, having removed the "counter reset" operation
from the proposed patch and reduced the comment to what we have today,
we promptly forgot about the whole thing.  So the comment is indeed a
relic that can be removed.

Thanks,

[1] https://postgr.es/c/bd18c50ba87f12d1dc0aa65c1ff0507b2d1c5c41

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La fuerza no está en los medios físicos
sino que reside en una voluntad indomable" (Gandhi)

On 2025-Nov-12, David Rowley wrote:

> I've never been able to make sense of that comment. I tried to dig a
> bit yesterday and still couldn't make sense of it in the context of
> the commit when it was added.  I assume it's talking about
> pg_stat_all_tables.autovacuum_count, but I can't think why that
> matters as I didn't find any code that took that into account for
> triggering autovacuum.

It's not at all talking about autovacuum_count.  That field came in with
Postgres 9.1 in 2010, commit 946045f04d11, several years later.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Por suerte hoy explotó el califont porque si no me habría muerto
 de aburrido"  (Papelucho)

On Wed, Nov 12, 2025 at 06:09:40PM +0100, Álvaro Herrera wrote:
> The comment you cite appears more or less in the same place in
> autovacuum-6.patch (the routine was called test_rel_for_autovac there),
> but I think what it really is about, is the AVOperUpdateCounts operation
> that was in autovacuum-5.patch.  Note that that older patch was
> introducing a new pg_autovacuum catalog which kept track of tuple counts
> since last analyze/vacuum, and those counts needed to be updated in case
> pgstats was reset.  But by patch autovacuum-6.patch this had disappeared
> (the counts were now in pgstats, per review from Tom), so we didn't need
> to reset any counters, and the AVOperUpdateCounts operation went away.
> 
> It's quite possible that, having removed the "counter reset" operation
> from the proposed patch and reduced the comment to what we have today,
> we promptly forgot about the whole thing.  So the comment is indeed a
> relic that can be removed.

I see, thanks for looking.  I've committed the patch.

-- 
nathan