Re: Track skipped tables during autovacuum and autoanalyze

Hi,

On Sat, 25 Jul 2026 01:48:32 +0900
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:

> Hi,
>
> I realized that I had been accidentally dropped from the CC list and had
> missed the recent discussions. I'm sorry for my late response, and thank
> you for keeping this moving.
>
> I've attached the v12 patch, which incorporates Kato-san's feedback.

Sorry for the noise, but I'm resubmitting v12 to include the missing expected
files for the isolation tests.

I've also added Michael back to the CC list, as both he and I had accidentally
been dropped from the recent thread.
>
> Regarding the column names, I noticed the change from skipped_* to lock_skipped_*.
> While I initially preferred skipped_*, I understand Kato-san's point considering
> that there could be other reasons for skipping in the future.
>
> > In 0001,
> >
> > > +step check_stat
> > > +{
> > > + SELECT relname,
> > > + vacuum_count, lock_skipped_vacuum_count,
> > > + analyze_count, lock_skipped_analyze_count
> > > + FROM pg_stat_all_tables
> > > + WHERE relname IN ('parted', 'part1', 'part2');
> > > +}
> >
> > How about adding "ORDER BY relname" here? The row order is not
> > guaranteed otherwise.
>
> Agreed. Fixed.
>
> >
> > > +{ oid => '8142', descr => 'statistics: last skipped vacuum time for a table',
> > > + proname => 'pg_stat_get_last_lock_skipped_vacuum_time', provolatile => 's',
> >
> > The proname now says lock_skipped, but the descr still says just
> > "skipped". I think the descr should mention the lock cause as well,
> > and the same applies to the other seven entries.
>
> I changed them like:
>
> 'statistics: last vacuum skip time due to lock unavailability for a table'
> 'statistics: number of vacuum skips due to lock unavailability for a table'
>
> How do these sound?
>
> > > +#define PGSTAT_REPORT_SKIPPED_VACUUM (1 << 0) /* vacuum is skipped */
> > > +#define PGSTAT_REPORT_SKIPPED_ANALYZE (1 << 1) /* analyze is skipped */
> >
> > The struct fields were renamed to lock_skipped_*, but these flags and
> > pgstat_report_skipped_vacuum_analyze() keep the generic name. Should
> > we rename them too, for consistency?
>
> I renamed them to PGSTAT_REPORT_LOCK_SKIPPED_*.
>
> >
> > In 0002,
> >
> > > + 'vacuum_skip_lock_stats',
> >
> > This also needs to be added to the ISOLATION list in
> > src/test/modules/injection_points/Makefile. Without it the test never
> > runs in the autoconf build. I confirmed it passes once registered
> > there.
>
> I fixed the Makefile.
>
> Also, I added a comment explaining the RangeVarGetRelid() call with
> NoLock in expand_vacuum_rel(), including why NoLock is used and why it is safe
> in this case.
>
> + /*
> + * Get relid for statistics reporting.
> + *
> + * Since we failed to acquire the lock, use NoLock here. Although
> + * a concurrent DDL may have dropped or renamed the relation,
> + * RangeVarGetRelid() with NoLock does not check for invalidation
> + * messages.
> + */
> + relid = RangeVarGetRelid(vrel->relation, NoLock, true);
> +

Regards,
Yugo Nagata

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>