Re: BUG #14150: Attempted to delete invisible tuple
От | Peter Geoghegan |
---|---|
Тема | Re: BUG #14150: Attempted to delete invisible tuple |
Дата | |
Msg-id | CAM3SWZQ_8aPcxRKm9Bsy5m46WqVoirmf7PMOw6-APe-k9MfSrw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: BUG #14150: Attempted to delete invisible tuple (Andres Freund <andres@anarazel.de>) |
Ответы |
Re: BUG #14150: Attempted to delete invisible tuple
(Oskari Saarenmaa <os@aiven.io>)
Re: BUG #14150: Attempted to delete invisible tuple (Amit Kapila <amit.kapila16@gmail.com>) |
Список | pgsql-bugs |
On Wed, Jul 6, 2016 at 2:40 PM, Andres Freund <andres@anarazel.de> wrote: > On 2016-07-06 13:30:57 +0300, Oskari Saarenmaa wrote: >> The attached patch against current master >> allows heap_abort_speculative to delete >> toast rows created by the same command >> which makes the above test case and "make >> check" run without failures. Note that I >> haven't touched this code before so I >> don't know how safe my patch is. > > I've not looked closely at this yet, but unfortunately we probably can't > just update the API of HeapTupleSatisfiesUpdate and friends, this needs > to be backpatched to 9.5... Having studied the problem a bit further, I now think that Oskari has the right idea, but that his patch needs polishing. I think that Andres is right that it's not appropriate to modify HeapTupleSatisfiesUpdate(). I'd add that I'd avoid even using heap_delete() directly just to suit the esoteric requirements of TOAST super deletion (a part of the speculative insertion infrastructure). A more specialized routine like heap_abort_speculative() itself seems like a better fit (e.g., a recursive call to heap_abort_speculative() for the TOAST tuple). There are a few obvious reasons why it wouldn't just work if the relevant code path was made to call heap_abort_speculative() rather than simple_heap_delete() from within toast_delete_datum(). However, heap_abort_speculative() is more or less a stripped down version of heap_delete(), and simple_heap_delete() is just a heap_delete() followed by a HTSU_Result test, so it would *almost* just work. heap_abort_speculative() is concerned with deleting a speculatively inserted tuple in a way that releases any other waiting xacts immediately ("super deleting"), as opposed to having them wait until the end of our (the inserter's) transaction (this avoids "unprincipled deadlocks" [1], an important design goal for UPSERT). It is prepared to delete a tuple inserted by the same command, where all existing heap_delete() callers are not; so heap_abort_speculative() doesn't even have a call to HeapTupleSatisfiesUpdate(), and so naturally doesn't require that HeapTupleSatisfiesUpdate() be specially asked to permit us an "instantaneous tuple deletion", which was Oskari's approach in his patch. Long story short, it seems like heap_abort_speculative() is at least closer to what is required than simple_heap_delete()/heap_delete(), because it simply doesn't care about HeapTupleSatisfiesUpdate(). In other words, heap_abort_speculative() was designed to allow an "instantaneous tuple deletion" from day one, so why not reuse that? heap_abort_speculative() does ensure that it's modifying a tuple it finds to be HeapTupleHeaderIsSpeculative(), which would not apply to its TOAST tuple (it doesn't get that flag), so that needs to be tweaked. Also, the "HeapTupleHeaderSetXmin(tp.t_data, InvalidTransactionId)" part (which is what makes a super deletion, uh, "super") may be heavy handed for TOAST tuples. Still, it looks like it wouldn't actually break anything to allow it for TOAST tuples, since HeapTupleSatisfiesToast() *is* prepared for a super deleted TOAST tuple (if only defensively), and there is nothing special about VACUUMing TOAST tables, AFAIK. Maybe heap_abort_speculative() should be refactored to call another function, and keep only the parts that specifically expect a HeapTupleHeaderIsSpeculative() tuple. The function that it is made to call (that consists of the majority of the current heap_abort_speculative() implementation) could also be called by a special super deletion variant of toast_delete(). No need to spread knowledge about speculative insertion any further this way, AFAICT. The UPSERT commit did modify two HeapTupleSatisfies* routines, but that didn't include HeapTupleSatisfiesUpdate() (just HeapTupleSatisfiesDirty(), and the aforementioned defensive code in HeapTupleSatisfiesToast()). What do you think of this outline, Oskari and Andres? [1] https://wiki.postgresql.org/wiki/Value_locking#.22Unprincipled_Deadlocking.22_and_value_locking -- Peter Geoghegan
В списке pgsql-bugs по дате отправления:
Следующее
От: Michael PaquierДата:
Сообщение: Re: BUG #14230: Wrong timeline returned by pg_stop_backup on a standby