Re: Track skipped tables during autovacuum and autoanalyze

Hi,

> 1. pgstat_report_skipped_vacuum_analyze() accesses the catalog while
> holding the stats entry lock.
>
> pgstat_get_entry_ref_locked() returns with the entry's LWLock held,
> and the re-check runs under it. A catcache miss there opens pg_class,
> so we can wait for a heavyweight lock while holding an LWLock, which
> can deadlock. The deadlock detector does not see the LWLock side, and
> LWLockAcquire() holds interrupts, so pg_cancel_backend() does not
> break the wait either.
>
> Could the re-check simply run after pgstat_unlock_entry()? The same
> case then cancels immediately.

Right. That is a good point. The cost becomes updating an entry that
will, on the rare occurrence, be immediately dropped. That is not a
big deal.

> 2. A skipped TOAST vacuum is also counted as a skipped analyze.
>
> vacuum_rel() copies params for the TOAST recursion and only adds
> VACOPT_PROCESS_MAIN, so VACOPT_ANALYZE survives into the recursive
> call, and a contended lock on the toast table bumps its
> lock_skipped_analyze_count as well. The comment just above that code
> says analyze is never done on toast tables, so clearing the flag next
> to the VACOPT_PROCESS_MAIN line looks right to me:
>
> ```
> + toast_vacuum_params.options &= ~VACOPT_ANALYZE;
> ```

Right. Fixed.

> 3. Relations that can never be vacuumed get a stats entry.
>
> The skip is reported before the relkind checks in vacuum_rel() and
> analyze_rel(), so VACUUM (SKIP_LOCKED) on a locked view creates a
> relation stats entry and bumps the counters. pg_stat_all_tables
> filters on relkind, so nothing can ever display them. The function
> already reads the pg_class tuple for relisshared, so it could pick up
> relkind at the same time and return early for the relkinds those two
> functions reject.

Added a filter to only track stats for any relation kind that can be displayed.
I added a test for the view case of this also.

It should be noted that we don't show foreign tables in pg_stat_all_tables,
so while these could be skipped in analyze, they do not have a way to be
reported.

Also, a a follow-up we may want to clean up cases in which relations that
cannot be vacuumed or analyzed could log a "lock not available"

```
postgres=# VACUUM (SKIP_LOCKED) codex_skip_test.v;
WARNING: skipping vacuum of "v" --- lock not available
VACUUM
```

This is less about stats reporting and more about filtering out such
relations earlier in expand_vacuum_rel() for manually specified
VACUUM/ANALYZE targets. Not a big thing, but it would avoid an extra
syscache lookup and a misleading "lock not available" warning.

> 4. Smaller things:
>
> - pgstat.h says "/* flags for pgstat_flush_backend() */" above the new
> PGSTAT_REPORT_LOCK_SKIPPED_* macros. They are the flags of
> pgstat_report_skipped_vacuum_analyze().

Fixed.

> - The docs never mention SKIP_LOCKED. A manual command waits for the
> lock otherwise, so the four manual columns only advance for commands
> using that option. A sentence in the new note would cover it.

I just added the SKIP_LOCKED callut in the description for the columns.

> - last_lock_skipped_autoanalyze says "by the autovacuum" and "Last
> time at which", while the three sibling entries say "by the autovacuum
> daemon" and "Last time".

Fixed

> - The four non-auto descrs in pg_proc.dat drop "manual", although the
> existing entries next to them say things like 'statistics: last manual
> vacuum time for a table'.

Fixed.

--
Sami Imseih
Amazon Web Services (AWS)