Re: Track skipped tables during autovacuum and autoanalyze - Mailing list pgsql-hackers
| From | Yugo Nagata |
|---|---|
| Subject | Re: Track skipped tables during autovacuum and autoanalyze |
| Date | |
| Msg-id | 20260413170551.5ec43ba5a2c848f0d46c6a0b@sraoss.co.jp Whole thread Raw |
| In response to | Re: Track skipped tables during autovacuum and autoanalyze (Yugo Nagata <nagata@sraoss.co.jp>) |
| List | pgsql-hackers |
Hello Sami Imseih, On Sat, 28 Mar 2026 16:18:02 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Fri, 27 Mar 2026 11:48:27 -0500 > Sami Imseih <samimseih@gmail.com> wrote: > > > > 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. > > That makes sense, since using int for flags seems common in other > places in the code. I'm not sure how much it affects performance, > though. > > > 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 > > I am fine with that. In that case, the nested logic would move to the > caller side. > > > 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 > > Hmm, I think skipped_vacuum_count is more consistent with > fields like last_vacuum and total_vacuum_time, where the modifier > comes before vacuum/analyze. What do you think about that? > > > 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>) > > I intended to keep consistency with the existing last_vacuum: > > Last time at which this table was manually vacuumed (not counting VACUUM FULL) > > although "at which" was accidentally omitted. Your suggestion seems > simpler and more natural to me. Should we prioritize that over consistency? > > > 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 > > The "a munual" was also accidentally omitted, so I'll fix it. > > > 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? > > Yeah, we cannot report skips on the children when a manual > vacuum/analyze on the parent table is skipped. (It might be possible > to obtain child information with NoLock, but that would not be safe.) > > Therefore, I agree that the best we can do here is to add a note to the > documentation of last_skipped_vacuum/analyze and skipped_vacuum/analyze_count. > > For example: > > When a manual vacuum or analyze on a parent table in an inheritance > or partitioning hierarchy is skipped, the statistics are recorded > only for the parent table, not for its children. > > > 6/ > > It would be nice to add a test for this, but this requires concurrency and I'm > > not sure it's woth it. > > I'm not sure what meaningful tests we could add for these statistics. > I couldn't find any existing tests for fields like last_vacuum. I've attached a patch reflecting your comments on items 1, 2, and 5. As for items 3, 4, and 6, I am waiting for your comments, so the patch is left unchanged for now. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
pgsql-hackers by date: