On Mon, Mar 15, 2021 at 11:04 AM Peter Geoghegan <p...@bowt.ie> wrote: > > On Thu, Mar 11, 2021 at 8:31 AM Robert Haas <robertmh...@gmail.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.
Thank you for the patch! > > 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. Agreed. > > 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 agree to create a function like vacuum_indexes_mark_unused() that makes a decision and does index and heap vacumming accordingly. But what is the point of removing both lazy_vacuum_all_indexes() and lazy_vacuum_heap()? I think we can simply have vacuum_indexes_mark_unused() call those functions. I'm concerned that if we add additional criteria to vacuum_indexes_mark_unused() in the future the function will become very large. > > 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. +1 I think we can set VACOPT_TERNARY_DISABLED to tab->at_params.index_cleanup in table_recheck_autovac() or increase the thresholds used to not skipping index vacuuming. > > 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. Aside from whether it's good or bad, strictly speaking, it could change the index AM API contract. The documentation of amvacuumcleanup() says: --- stats is whatever the last ambulkdelete call returned, or NULL if ambulkdelete was not called because no tuples needed to be deleted. --- With this change, we could pass NULL to amvacuumcleanup even though the index might have tuples needed to be deleted. > > 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? Yeah, I agree with that change. As I mentioned in a recent reply, I'm concerned about a case where we ran out maintenance_work_mem and decided not to skip index vacuuming but collected only a few dead tuples in the second index vacuuming (i.g., the total amount of dead tuples is slightly larger than maintenance_work_mem). In this case, I think we can skip the second (i.g., final) index vacuuming at least in terms of maintenance_work_mem. Maybe the same is true in terms of LP_DEAD accumulation. > (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.) Thanks! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
