Re: BUG #14150: Attempted to delete invisible tuple

On Thu, Jul 7, 2016 at 7:45 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Wed, Jul 6, 2016 at 2:40 PM, Andres Freund <andres(at)anarazel(dot)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?
>

Agreed. I think that makes sense.

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

IIUC, then I think you are proposing to have an API (something like
heap_delete_minimal) which will workout well for both heap and toast
tuples with respect to heap_abort_speculative(). I think to solve
this issue, the approach you outlined above seems to be better than
what's being done in Oskari's patch. The advantage of this approach
is that it will save us from touching HeapTupleSatisfiesUpdate and
will do the minimal things (like excluding the replica identity &
replication origin information from WAL) required for deletion of
toast tuples.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com