Re: Track skipped tables during autovacuum and autoanalyze

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.

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>