Re: BUG #14150: Attempted to delete invisible tuple

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