Re: Removing vacuum_cleanup_index_scale_factor

On Mon, Mar 1, 2021 at 10:33 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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