From: | Peter Geoghegan <pg(at)bowt(dot)ie> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Noah Misch <noah(at)leadboat(dot)com> |
Subject: | Re: New IndexAM API controlling index vacuum strategies |
Date: | 2021-03-15 19:58:33 |
Message-ID: | CAH2-Wz=btR5YeJbHMA0BjmrTdTo_DuwkyG5vSqxBbR8KBGpXsQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Mar 15, 2021 at 12:21 PM Andres Freund <andres(at)anarazel(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2021-03-15 20:20:22 | Re: Support tab completion for upper character inputs in psql |
Previous Message | Alvaro Herrera | 2021-03-15 19:47:25 | Re: [HACKERS] PATCH: Batch/pipelining support for libpq |