On 29/01/2019 16:28, Andres Freund wrote:
> Hi,
>
> On January 29, 2019 4:19:52 AM PST, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
>> Hi,
>>
>> On 20/01/2019 21:03, Andres Freund wrote:
>>> Hi,
>>>
>>> Currently RelationFindReplTupleByIndex(), RelationFindReplTupleSeq()
>>> lock the found tuple. I don't quite get what that achieves - why
>> isn't
>>> dealing with concurrency in the table_update/delete calls at the
>>> callsites sufficient? As far as I can tell there's no meaningful
>>> concurrency handling in the heap_lock_tuple() paths, so it's not like
>> we
>>> follow update chains or anything.
>>>
>>
>> Yeah that's leftover from the conflict detection/handling code that I
>> stripped away to keep the patched manageable size-wise. As things stand
>> now we could remove that and use normal heap_update instead of simple
>> variant. It'll be likely be needed again if we add conflict handling in
>> the future, but perhaps we could be smarter about it then (i.e. I can
>> imagine that it will be per table anyway, not necessarily default
>> behavior).
>
> Why does conflict handling need the unconditional lock? Wouldn't just doing that after an initial heap_update
returnedHeapTupleUpdated make more sense? And wouldn't it need to reckeck the row afterwards as well?
>
To prevent tuple changing from under the conflict resolution logic - we
need to make sure that whatever tuple the conflict resolution logic gets
is the current one. If the tuple is locked you won't get the
HeapTupleUpdated in heap_update anymore (which is why we can use
simple_heap_update).
In any case we don't have conflict resolution at the moment so it's
probably moot right now.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
