Re: Track skipped tables during autovacuum and autoanalyze

Hi Sami,

On Sat, 25 Jul 2026 02:51:59 +0900
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:

> 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.

I found athat the injection_points's isolation test could fail [1]
because the check_stats step can be launched before the vacuum finishes.

I've attached a patch to fix this by inserting "noop" step in session s2
to ensure that the vacuum has finished.

Does this look good to you?

[1] https://github.com/postgresql-cfbot/postgresql/actions/runs/30115503531/attempts/1
> >
> > 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>