Re: New IndexAM API controlling index vacuum strategies

On Thu, Mar 11, 2021 at 8:31 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> But even if not, I'm not sure this
> helps much with the situation you're concerned about, which involves
> non-HOT tuples.

Attached is a POC-quality revision of Masahiko's
skip_index_vacuum.patch [1]. There is an improved design for skipping
index vacuuming (improved over the INDEX_CLEANUP stuff from Postgres
12). I'm particularly interested in your perspective on this
refactoring stuff, Robert, because you ran into the same issues after
initial commit of the INDEX_CLEANUP reloption feature [2] -- you ran
into issues with the "tupgone = true" special case. This is the case
where VACUUM considers a tuple dead that was not marked LP_DEAD by
pruning, and so needs to be killed in the second heap scan in
lazy_vacuum_heap() instead. You'll recall that these issues were fixed
by your commit dd695979888 from May 2019. I think that we need to go
further than you did in dd695979888 for this -- we ought to get rid of
the special case entirely.

ISTM that any new code that skips index vacuuming really ought to be
structured as a dynamic version of the "VACUUM (INDEX_CLEANUP OFF)"
mechanism. Or vice versa. The important thing is to recognize that
they're essentially the same thing, and to structure the code such
that they become exactly the same mechanism internally. That's not
trivial right now. But removing the awful "tupgone = true" special
case seems to buy us a lot -- it makes unifying everything relatively
straightforward. In particular, it makes it possible to delay the
decision to vacuum indexes until the last moment, which seems
essential to making index vacuuming optional. And so I have removed
the tupgone/XLOG_HEAP2_CLEANUP_INFO crud in the patch -- that's what
all of the changes relate to. This results in a net negative line
count, which is a nice bonus!

I've CC'd Noah, because my additions to this revision (of Masahiko's
patch) are loosely based on an abandoned 2013 patch from Noah [3] --
Noah didn't care for the "tupgone = true" special case either. I think
that it's fair to say that Tom doesn't much care for it either [4], or
at least was distressed by its lack of test coverage as of a couple of
years ago -- which is a problem that still exists today. Honestly, I'm
surprised that somebody else hasn't removed the code in question
already, long ago -- what possible argument can be made for it now?

This patch makes the "VACUUM (INDEX_CLEANUP OFF)" mechanism no longer
get invoked as if it was like the "no indexes on table so do it all in
one heap pass" optimization. This seems a lot clearer -- INDEX_CLEANUP
OFF isn't able to call lazy_vacuum_page() at all (for the obvious
reason), so any similarity between the two cases was always
superficial -- skipping index vacuuming should not be confused with
doing a one-pass VACUUM/having no indexes at all. The original
INDEX_CLEANUP structure (from commits a96c41fe and dd695979) always
seemed confusing to me for this reason, FWIW.

Note that I've merged multiple existing functions in vacuumlazy.c into
one: the patch merges lazy_vacuum_all_indexes() and lazy_vacuum_heap()
into a single function named vacuum_indexes_mark_unused() (note also
that lazy_vacuum_page() has been renamed to mark_unused_page() to
reflect the fact that it is now strictly concerned with making LP_DEAD
line pointers LP_UNUSED). The big idea is that there is one choke
point that decides whether index vacuuming is needed at all at one
point in time, dynamically. vacuum_indexes_mark_unused() decides this
for us at the last moment. This can only happen during a VACUUM that
has enough memory to fit all TIDs -- otherwise we won't skip anything
dynamically.

We may in the future add additional criteria for skipping index
vacuuming. That can now just be added to the beginning of this new
vacuum_indexes_mark_unused() function. We may even teach
vacuum_indexes_mark_unused() to skip some indexes but not others in a
future release, a possibility that was already discussed at length
earlier in this thread. This new structure has all the context it
needs to do all of these things.

I wonder if we can add some kind of emergency anti-wraparound vacuum
logic to what I have here, for Postgres 14. Can we come up with logic
that has us skip index vacuuming because XID wraparound is on the
verge of causing an outage? That seems like a strategically important
thing for Postgres, so perhaps we should try to get something like
that in. Practically every post mortem blog post involving Postgres
also involves anti-wraparound vacuum.

One consequence of my approach is that we now call
lazy_cleanup_all_indexes(), even when we've skipped index vacuuming
itself. We should at least "check-in" with the indexes IMV. To an
index AM, this will be indistinguishable from a VACUUM that never had
tuples for it to delete, and so never called ambulkdelete() before
calling amvacuumcleanup(). This seems logical to me: why should there
be any significant behavioral divergence between the case where there
are 0 tuples to delete and the case where there is 1 tuple to delete?
The extra work that we perform in amvacuumcleanup() (if any) should
almost always be a no-op in nbtree following my recent refactoring
work. More generally, if an index AM is doing too much during cleanup,
and this becomes a bottleneck, then IMV that's a problem that needs to
be fixed in the index AM.

Masahiko: Note that I've also changed the SKIP_VACUUM_PAGES_RATIO
logic to never reset the count of heap blocks with one or more LP_DEAD
line pointers, per remarks in a recent email [5] -- that's now a table
level count of heap blocks. What do you think of that aspect? (BTW, I
pushed your fix for the "not setting has_dead_tuples/has_dead_items
variable" issue today, just to get it out of the way.)

[1] https://postgr.es/m/CAD21AoAtZb4+HJT_8RoOXvu4HM-Zd4HKS3YSMCH6+-W=bDyh-w@mail.gmail.com
[2] https://postgr.es/m/23885.1555357618%40sss.pgh.pa.us
[3] https://postgr.es/m/20130108024957.GA4751%40tornado.leadboat.com
[4] https://postgr.es/m/16814.1555348381%40sss.pgh.pa.us
[5] https://postgr.es/m/CAH2-Wznpywm4qparkQxf2ns3c7BugC4x7VzKjiB8OCYswwu-=g@mail.gmail.com
--
Peter Geoghegan