Re: New IndexAM API controlling index vacuum strategies

On Mon, Mar 15, 2021 at 12:21 PM Andres Freund <andres@anarazel.de> wrote:
> 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.

Right, which is why I thought that I might be missing something; why
put up with that at all for so long?

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

I don't follow.

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

I guess that you trailed off here. My understanding is that removing
the special case results in practically no loss of dead tuples removed
in practice -- so there are no practical performance considerations
here.

Have I missed something?

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

I wrote it in a day, just to show what I had in mind. The renaming
stuff is a part of unifying those functions, which can be discussed
after the "tupgone = true" special case is removed. It's not like I'm
set on the details that you see in the patch.

> 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 agree that it should be broken up for review.

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

I noticed that Noah's similar 2013 patch [1] added a defensive
heap_tuple_needs_freeze() + elog(ERROR) to the HEAPTUPLE_DEAD case. I
suppose that that's roughly what you have in mind here?

I suppose that that was pre-9.3-MultiXacts, and so now it's more complicated.

Comments above heap_prepare_freeze_tuple() say something about making
sure that HTSV did not return HEAPTUPLE_DEAD...but that's already
possible today:

 * It is assumed that the caller has checked the tuple with
 * HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD
 * (else we should be removing the tuple, not freezing it).

Does that need work too?

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

That's a good reference, thanks.

[1] https://www.postgresql.org/message-id/20130130020456.GE3524%40tornado.leadboat.com
-- 
Peter Geoghegan