Re: Track skipped tables during autovacuum and autoanalyze

> Thanks for working on this. I read through v9 and I am +1 on the feature.
>
> It does not build against master for me:
>
> pgstat_relation.c: In function ‘pgstat_report_skipped_vacuum_analyze’:
> pgstat_relation.c:419:17: error: too few arguments to function
> ‘pgstat_drop_entry’
> 419 | pgstat_drop_entry(PGSTAT_KIND_RELATION,
> | ^~~~~~~~~~~~~~~~~
> In file included from pgstat_relation.c:27:
> ../../../../src/include/utils/pgstat_internal.h:811:13: note: declared here
> 811 | extern bool pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid,
> | ^~~~~~~~~~~~~~~~~
> make[4]: *** [<builtin>: pgstat_relation.o] Error 1
>
> pgstat_drop_entry() gained a missing_ok argument in commit 850b9218c8e
> [1], so the call in pgstat_report_skipped_vacuum_analyze() needs a
> fourth argument. Passing true, the way the function-usage path in
> pgstat_function.c now does, builds cleanly.

Correct. This is due to 850b9218c8e4. I added 'true' to silently ignore
concurrently dropped tables.

> On the field names: every skip these fields count is a lock skip, but
> skipped_vacuum_count, last_skipped_vacuum and friends do not say so.
>
> In the "Add autovacuum_warning to surface concurrent vacuum
> collisions" thread [2], I raised another skip worth tracking, an
> autovacuum worker skipping a table because another worker is already
> vacuuming it, as an early sign that autovacuum is not keeping up.
> There it was suggested to expose it as a per-table counter in
> pg_stat_all_tables, the same surface this patch builds. That needs its
> own discussion and I am not proposing to widen this patch for it.
>
> But since another skip reason may be added later, I think the column
> name should make clear the skip is due to lock unavailability. What do
> you think?
>
> [1] https://postgr.es/c/850b9218c8e
> [2] https://www.postgresql.org/message-id/CALj2ACUYYv_z3C5VkcYmyezbqxUuJ%2B0H1GeNGZqPK3WFexmTYw%40mail.gmail.chttps://postgr.es/c/850b9218c8e

I am not sure about this. I read [1], and I really don't think that
showing the message " table ... is already being vacuumed by another worker"
or tracking a counter for this case is needed, but we should keep that
discussion
in that thread.

However, I still agree with your point about making the column name more
precise, so doing something like *_lock_skipped_* is better. I don't
foresee this,
but if we do ever track another skip reason besides locking, it will need to
be a separate counter.

See v10.

[1] https://www.postgresql.org/message-id/flat/CALj2ACUYYv_z3C5VkcYmyezbqxUuJ%2B0H1GeNGZqPK3WFexmTYw%40mail.gmail.com#ccd6fdf16eb76b4e97e6932d24da449c

--
Sami Imseih
Amazon Web Services (AWS)