On Wed, Jul 27, 2016 at 4:53 PM, Andres Freund <andres@anarazel.de> wrote:
> Maybe I'm missing something, but toasted columns aren't actually marked
> as speculative right now, no? Cf.
> so going through heap_abort_speculative()/heap_delete_speculative()
> doesn't look right to me. We could change thing so toasted rows also
> get inserted speculatively - but I don't see much of a point.
That was my understanding also. And like you, I see zero point in
making sure that TOAST tuples are in fact speculatively inserted. On
the other hand, I also fail to see any consequence of "super deleting"
TOAST tuples created as part of speculative insertion, since
HeapTupleSatisfiesToast() is already prepared for that possibility (if
only defensively).
In other words, while I suggested that there was plenty to be reused
in heap_abort_speculative(), some things clearly had to be a little
different -- as few as possible, though. I didn't think it was worth
bothering teaching heap_abort_speculative()/heap_delete_speculative()
to give additional special treatment to these TOAST tuples just
because they weren't actually speculatively inserted. In short:
perhaps we should always "HeapTupleHeaderSetXmin(tp.t_data,
InvalidTransactionId)", even for TOAST tuples, *just* to be
consistent.
I don't think it's that important that we actually go this way. (Note
also that Oskari might have changed the wording of comments within
HeapTupleSatisfiesToast() to indicate that a super-deleted TOAST tuple
is definitely possible, and not just something to be paranoid about).
>> +void
>> +heap_delete_speculative(Relation relation, ItemPointer tid, bool is_toast)
>> +{
>
> If we go this way, it seems easier to get is_toast from
> IsToastRelation(relation), rather than hand it through painstakingly.
+1
--
Peter Geoghegan
