| From: | Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> |
|---|---|
| To: | Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> |
| Cc: | Sami Imseih <samimseih(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Track skipped tables during autovacuum and autoanalyze |
| Date: | 2026-04-13 08:05:51 |
| Message-ID: | 20260413170551.5ec43ba5a2c848f0d46c6a0b@sraoss.co.jp |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello Sami Imseih,
On Sat, 28 Mar 2026 16:18:02 +0900
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> On Fri, 27 Mar 2026 11:48:27 -0500
> Sami Imseih <samimseih(at)gmail(dot)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(at)sraoss(dot)co(dot)jp>
| Attachment | Content-Type | Size |
|---|---|---|
| v4-0001-Track-skipped-vacuum-and-analyze-activity-per-rel.patch | text/x-diff | 24.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ashutosh Sharma | 2026-04-13 08:10:20 | Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication |
| Previous Message | Peter Smith | 2026-04-13 07:47:21 | Re: Support EXCEPT for ALL SEQUENCES publications |