public inbox for [email protected]
help / color / mirror / Atom feedFrom: Yugo Nagata <[email protected]>
To: Sami Imseih <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Pgsql Hackers <[email protected]>
Subject: Re: Track skipped tables during autovacuum and autoanalyze
Date: Tue, 12 May 2026 18:47:21 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA5RZ0vwgXZ5kF2GvYBR+Ma1LPSbDjE9pjANzSiUw3wmpv51PQ@mail.gmail.com>
References: <[email protected]>
<CAA5RZ0snnePNW1NKGKh+NyJ1CY26T5F_6-tTq+BHWM2kj1fN1g@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAA5RZ0tNh03LLgJVMA=PXSdY8YVoui4_GyfbfTrYK5cka3Q9Rw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAA5RZ0tUHU-r=Tc47P2DZyytF7x5h5OwBiRABe_dZt+zWNqe9g@mail.gmail.com>
<[email protected]>
<CAA5RZ0vwgXZ5kF2GvYBR+Ma1LPSbDjE9pjANzSiUw3wmpv51PQ@mail.gmail.com>
On Mon, 4 May 2026 15:44:57 -0500
Sami Imseih <[email protected]> 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 <[email protected]>
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Track skipped tables during autovacuum and autoanalyze
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox