On Fri, 27 Mar 2026 11:48:27 -0500 Sami Imseih <[email protected]> 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. Regards, Yugo Nagata -- Yugo Nagata <[email protected]>
