Thread

Re: Track skipped tables during autovacuum and autoanalyze

Yugo Nagata <nagata@sraoss.co.jp> — 2026-05-12T09:47:21Z

On Mon, 4 May 2026 15:44:57 -0500
Sami Imseih <samimseih@gmail.com> wrote:

Thank you for your review!

> > > 1/
> > >
> > > +            relid = RangeVarGetRelid(vrel->relation, NoLock, false);
> > >
> > > Should this be called with "true" as the 3rd (missing_ok) argument, otherwise
> > > we end up with an error instead of a "--- relation no longer exists" log. right?
> >
> > No, it should be false. If missing_ok is true, VACUUM (SKIP_LOCKED) on a not-existing
> > table would emit a "skipping vacuum of ... --- relation no longer exists" message, but
> > it should be "relation ... does not exist".
> 
> Yeah you are right.
> 
> But, after looking more into this, I still think the
> expand_vacuum_rel() changes can be
> improved. The branching
> -                */
> -               if (!OidIsValid(relid))
> +               if (!(options & VACOPT_SKIP_LOCKED))
>                 {
> -                       if (options & VACOPT_VACUUM)
> -                               ereport(WARNING,
> -
> (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
> -                                                errmsg("skipping
> vacuum of \"%s\" --- lock not available",
> -
> vrel->relation->relname)));
> -                       else
> -                               ereport(WARNING,
> -
> (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
> -                                                errmsg("skipping
> analyze of \"%s\" --- lock not available",
> +                       relid = RangeVarGetRelidExtended(vrel->relation,
> +
>                   AccessShareLock,
> +
>                   0, NULL, NULL);
> +                       if (!OidIsValid(relid))
> +                               return vacrels;
> +               }
> +               else
> +               {
> +                       /* Get relid for reporting before taking a lock */
> +                       relid = RangeVarGetRelid(vrel->relation, NoLock, false);
> +
> +                       if (!ConditionalLockRelationOid(relid, AccessShareLock))
> 
> is not needed. We can continue just using RangeVarGetRelidExtended()
> with the rvr_opts and an AccessExclusiveLock, and once we need to
> report that we cannot obtain the lock, RangeVarGetRelid() can be
> called at that point for the purpose of calling
> pgstat_report_skipped_vacuum_analyze(). This is safer than calling
> ConditionalLockRelationOid() on a relid obtained with NoLock. See
> attached v6.

It seems good to me.

Initially, I was concerned that something might go wrong if a concurrent
session performed DROP TABLE or ALTER TABLE RENAME between RangeVarGetRelidExtended()
and RangeVarGetRelid(), but I could not find any actual issue. Even when the table
name is changed, the correct statistics entry is updated correctly.

So I'm fine with your version.

Regards,
Yugo Nagata

> >> 2/
> >>
> >> Can the isolation tests
> >> src/test/isolation/specs/vacuum-skip-locked.spec be updated
> >> to check pg_stat_user_tables as well?
> 
> > Yes, we can. I've attached an updated patch including that test.
> 
> > While working on the test, I noticed that skipped FULL VACUUM was counted
> > in the previous patch, so I fixed it not to avoid counting those cases.
> 
> The tests looks good to me.
> 
> > The names of the new fields are still open. The current pattern is
> > "last_skipped_..." and "skipped_..._count". Alternatively, we could use
> > "..._last_skip" and "..._skip_count", which would be consistent with
> > slotsync_skip_count and slosync_last_skip.
> 
> > Which do you think is better?
> 
> I think last_skipped_* is better since we use last_vacuum, last_autovacuum, etc.
> 
> --
> Sami


-- 
Yugo Nagata <nagata@sraoss.co.jp>