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