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)
v15-0002-Add-injection-point-test-for-vacuum-skip_locked-.patch
Description: Binary data
v15-0001-Track-skipped-vacuum-and-analyze-activity-per-re.patch
Description: Binary data
