On 29/01/2019 17:14, Andres Freund wrote: > Hi > > On January 29, 2019 8:04:55 AM PST, Petr Jelinek > <petr.jeli...@2ndquadrant.com> wrote: >> On 29/01/2019 16:28, Andres Freund wrote: >>> Hi, >>> >>> On January 29, 2019 4:19:52 AM PST, Petr Jelinek >> <petr.jeli...@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 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. >
But the conflict is not about local node concurrency, rather multi-node one (ie, merging data from multiple nodes) so you can't depend on heap_update return value alone. You usually need to figure out if there is conflict before you even do update (based on tuple metadata/data) which is why the lock is necessary. > Either way, right now it's definitely superfluous... > Agreed. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
