On Mon, Mar 1, 2021 at 10:33 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > The original design that made VACUUM set > pg_class.reltuples/pg_class.relpages in indexes (from 15+ years ago) > assumed that it was cheap to handle statistics in passing. Even if we > have btvacuumcleanup() not do an index scan at all, this is 100% > allowed by the amvacuumcleanup contract described in the > documentation: > > "It is OK to return NULL if the index was not changed at all during > the VACUUM operation, but otherwise correct stats should be returned." > > The above description was added by commit e57345975cf in 2006 and > hasn't changed for now.
The intention here is not to revise the amvacuumcleanup() contract -- the contract already allows us to do what we want inside nbtree. We want to teach btvacuumcleanup() to not do any real work, at least outside of rare cases where we have known deleted pages that must still be placed in the FSM for recycling -- btvacuumcleanup() would generally just return NULL when there was no btbulkdelete() call during the same VACUUM operation (the main thing that prevents this today is vacuum_cleanup_index_scale_factor). More generally, we would like to change the *general* expectations that we make of index AMs in places like vacuumlazy.c and analyze.c. But we're worried about dependencies that aren't formalized anywhere but still matter -- code may have evolved to assume that index AMs behaved a certain way in common and important cases (probably also in code like vacuumlazy.c). That's what we want to avoid breaking. Masahiko has already given an example of such a problem: currently, VACUUM ANALYZE simply assumes that its VACUUM will call each indexes' amvacuumcleanup() routine in all cases, and will have each call set pg_class.reltuples and pg_class.relpages in respect of each index. ANALYZE therefore avoids overwriting indexes' pg_class stats inside do_analyze_rel() (at the point where it calls vac_update_relstats() for each index). That approach is already wrong with hash indexes, but under this new directly for btvacuumcleanup(), it would become wrong in just the same way with nbtree indexes (if left unaddressed). Clearly "the letter of the law" and "the spirit of the law" must both be considered. We want to talk about the latter on this thread. Concretely, here are specific questions (perhaps Tom can weigh in on these as the principal designer of the relevant interfaces): 1. Any objections to the idea of teaching VACUUM ANALYZE to distinguish between the cases where VACUUM ran and performed "real index vacuuming", to make it more intelligent about overwriting pg_class stats for indexes? I define "real index vacuuming" as calling any indexes ambulkdelete() routine. 2. Does anybody anticipate any other issues? Possibly an issue that resembles this existing known VACUUM ANALYZE issue? Thanks -- Peter Geoghegan
