MIME-Version: 1.0
References: <20260324151133.7940a5c1f2ebd594d54da481@sraoss.co.jp>
 <CAA5RZ0snnePNW1NKGKh+NyJ1CY26T5F_6-tTq+BHWM2kj1fN1g@mail.gmail.com>
 <20260325012847.e026ba1860c07288efe3e97d@sraoss.co.jp>
 <20260326192203.e6dbb8d80f8d27dc15ceee59@sraoss.co.jp>
 <acW8MW-Gb8Aj_TH2@paquier.xyz>
 <20260327163549.b5df519c0099970ddbb3412d@sraoss.co.jp>
In-Reply-To: <20260327163549.b5df519c0099970ddbb3412d@sraoss.co.jp>
From: Sami Imseih <samimseih@gmail.com>
Date: Fri, 27 Mar 2026 11:48:27 -0500
Message-ID: 
 <CAA5RZ0tNh03LLgJVMA=PXSdY8YVoui4_GyfbfTrYK5cka3Q9Rw@mail.gmail.com>
Subject: Re: Track skipped tables during autovacuum and autoanalyze
To: Yugo Nagata <nagata@sraoss.co.jp>
Cc: Michael Paquier <michael@paquier.xyz>,
 Pgsql Hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA5RZ0tNh03LLgJVMA%3DPXSdY8YVoui4_GyfbfTrYK5cka3Q9Rw%40mail.gmail.com>
Precedence: bulk

> I've attached a revised patch reflecting this change, and it also includes
> the documentation.

Thanks fo the update!

I have some comments:

1/
+pgstat_report_skipped_vacuum_analyze(Oid relid, bits8 flags)

using bit8 is fine here, but I would have just used int. For this
case, it's just a matter of prefernace.

2/
+/* flags for pgstat_flush_backend() */
+#define PGSTAT_REPORT_SKIPPED_VACUUM   (1 << 0)        /* vacuum is skipped */
+#define PGSTAT_REPORT_SKIPPED_ANALYZE  (1 << 1)        /* analyze is skipped */
+#define PGSTAT_REPORT_SKIPPED_AUTOVAC  (1 << 2)        /* skipped
during autovacuum/autoanalyze */
+#define PGSTAT_REPORT_SKIPPED_ANY   (PGSTAT_REPORT_SKIPPED_VACUUM |
PGSTAT_REPORT_SKIPPED_ANALYZE)

can we just have 4 flags, SKIPPED_VACUUM, SKIPPED_ANALYZE,
SKIPPED_AUTOVACUUM, SKIPPED_AUTOANALYZE,
which can then remove the nested if/else and makes the mapping more obvious

+       if (flags & PGSTAT_REPORT_SKIPPED_AUTOVAC)
+       {
+               if (flags & PGSTAT_REPORT_SKIPPED_VACUUM)
+               {
+                       tabentry->last_skipped_autovacuum_time = ts;
+                       tabentry->skipped_autovacuum_count++;
+               }
+               if (flags & PGSTAT_REPORT_SKIPPED_ANALYZE)
+               {
+                       tabentry->last_skipped_autoanalyze_time = ts;
+                       tabentry->skipped_autoanalyze_count++;
+               }
+       }
+       else
+       {
+               if (flags & PGSTAT_REPORT_SKIPPED_VACUUM)
+               {
+                       tabentry->last_skipped_vacuum_time = ts;
+                       tabentry->skipped_vacuum_count++;
+               }
+               if (flags & PGSTAT_REPORT_SKIPPED_ANALYZE)
+               {
+                       tabentry->last_skipped_analyze_time = ts;
+                       tabentry->skipped_analyze_count++;
+               }
+       }

3/
For the sake of consistency, can we rename the fields from

skipped_vacuum_count to vacuum_skipped_count, etc. ? to be similar
to fields like vacuum_count

4/
field documentation could be a bit better to match existing phrasing

For example, the timestamp fields:

-       Last time a manual vacuum on this table was attempted but skipped due to
-       lock unavailability (not counting <command>VACUUM FULL</command>)
+       The time of the last manual vacuum on this table that was skipped
+       due to lock unavailability (not counting <command>VACUUM FULL</command>)

and the counter fields

-       Number of times vacuums on this table have been attempted but skipped
+       Number of times a manual vacuum on this table has been skipped

5/
Partitioned table asymmetry between vacuum_count and vacuum_skipped_count.

vacuum_count never increments on a the parenttable, because the parent is never
pocessed. On the other hand, if the manual VACUUM/ANALYZE is on the
parent table,
then we will skip all the children. So, we should still report the skip on the
parent table, but we should add a Notes section in the docs perhaps to
document this caveat?

6/
It would be nice to add a test for this, but this requires concurrency and I'm
not sure it's woth it.

Also, can you create a CF entry in
https://commitfest.postgresql.org/59/, please.

Thanks!

--
Sami Imseih
Amazon Web Services (AWS)