On Sat, Jul 16, 2016 at 10:14 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> 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.
Right, but Oskari's latest revision of the patch is a response to this
feedback, which I'm happy with.
At the moment, fixing the bug is blocking on proper review of that
second revision. I've outlined already that I have a couple of
non-specific concerns about how it could be buggy, neither of which
are actionable by Oskari. It needs further scrutiny from other
hackers, particularly Andres, but looks correct to me.
--
Peter Geoghegan