Re: New IndexAM API controlling index vacuum strategies

Hi,

On 2021-03-14 19:04:34 -0700, Peter Geoghegan wrote:
> 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.

It's evil sorcery. Fragile sorcery. I think Robert, Tom and me all run
afoul of edge cases around it in the last few years.

> 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.

You haven't really justified, in the patch or this email, why it's OK to
remove the whole logic around HEAPTUPLE_DEAD part of the logic.

VACUUM can take a long time, and not removing space for all the
transactions that aborted while it wa

> 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).

It doesn't really seem to be *just* doing that - doing the
PageRepairFragmentation() and all-visible marking is relevant too?

For me the patch does way too many things at once, making it harder than
necessary to review, test (including later bisection). I'd much rather
see the tupgone thing addressed on its own, without further changes, and
then the rest done in separate commits subsequently.

I don't like vacuum_indexes_mark_unused() as a name. That sounds like
the index is marked unused, not index entries pointing to tuples. Don't
really like mark_unused_page() either for similar reasons - but it's not
quite as confusing.

> - if (tupgone)
> - {
> - lazy_record_dead_tuple(dead_tuples, &(tuple.t_self));
> - HeapTupleHeaderAdvanceLatestRemovedXid(tuple.t_data,
> - &vacrelstats->latestRemovedXid);
> - tups_vacuumed += 1;
> - has_dead_items = true;
> - }
> - else
> - {
> - bool tuple_totally_frozen;
> + num_tuples += 1;
> + hastup = true;
>
> - num_tuples += 1;
> - hastup = true;
> + /*
> + * Each non-removable tuple must be checked to see if it needs
> + * freezing. Note we already have exclusive buffer lock.
> + */
> + if (heap_prepare_freeze_tuple(tuple.t_data,
> + relfrozenxid, relminmxid,
> + FreezeLimit, MultiXactCutoff,
> + &frozen[nfrozen],
> + &tuple_totally_frozen))
> + frozen[nfrozen++].offset = offnum;

I'm not comfortable with this change without adding more safety
checks. If there's ever a case in which the HEAPTUPLE_DEAD case is hit
and the xid needs to be frozen, we'll either cause errors or
corruption. Yes, that's already the case with params->index_cleanup ==
DISABLED, but that's not that widely used.

See
https://postgr.es/m/20200724165514.dnu5hr4vvgkssf5p%40alap3.anarazel.de
for some discussion around the fragility.

Greetings,

Andres Freund