Re: Why does execReplication.c lock tuples?

Hi

On January 29, 2019 8:04:55 AM PST, Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>On 29/01/2019 16:28, Andres Freund wrote:
>> Hi,
>>
>> On January 29, 2019 4:19:52 AM PST, Petr Jelinek
><petr(dot)jelinek(at)2ndquadrant(dot)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 returned HeapTupleUpdated 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.

Not sure why that's relevant - what you'd do is to attempt the update, if it succeeds: great. If not, then you'd do the lock tuple and after that do the conflict resolution. Less WAL and cpu for the common case, same WAL as now for the conflict, with just a small bit of additional CPU costs for the latter.

Either way, right now it's definitely superfluous...

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.