Thread: Lock mode in ExecMergeMatched()
Hi! I wonder why does ExecMergeMatched() determine the lock mode using ExecUpdateLockMode(). Why don't we use lock mode set by table_tuple_update() like ExecUpdate() does? I skim through the MERGE-related threads, but didn't find an answer. I also noticed that we use ExecUpdateLockMode() even for CMD_DELETE. That ends up by usage of LockTupleNoKeyExclusive for CMD_DELETE, which seems plain wrong for me. The proposed change is attached. ------ Regards, Alexander Korotkov
Attachment
On Fri, 10 Mar 2023 at 21:42, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > I wonder why does ExecMergeMatched() determine the lock mode using > ExecUpdateLockMode(). Why don't we use lock mode set by > table_tuple_update() like ExecUpdate() does? I skim through the > MERGE-related threads, but didn't find an answer. > > I also noticed that we use ExecUpdateLockMode() even for CMD_DELETE. > That ends up by usage of LockTupleNoKeyExclusive for CMD_DELETE, which > seems plain wrong for me. > > The proposed change is attached. > That won't work if it did a cross-partition update, since it won't have done a table_tuple_update() in that case, and updateCxt.lockmode won't have been set. Also, when it loops back and retries, it might execute a different action next time round. So I think it needs to unconditionally use LockTupleExclusive, since it doesn't know if it'll end up executing an update or a delete. I'm currently working on a patch for bug #17809 that might change that code though. Regards, Dean
> On Fri, 10 Mar 2023 at 21:42, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > I wonder why does ExecMergeMatched() determine the lock mode using > > ExecUpdateLockMode(). Why don't we use lock mode set by > > table_tuple_update() like ExecUpdate() does? I skim through the > > MERGE-related threads, but didn't find an answer. > > > > I also noticed that we use ExecUpdateLockMode() even for CMD_DELETE. > > That ends up by usage of LockTupleNoKeyExclusive for CMD_DELETE, which > > seems plain wrong for me. > I pushed the patch for bug #17809, which in the end didn't directly touch this code. I considered including the change of lockmode in that patch, but in the end decided against it, since it wasn't directly related to the issues being fixed there, and I wanted more time to think about what changing the lockmode here really means. I'm wondering now if it really matters what lock mode we use here. If the point of calling table_tuple_lock() after a concurrent update is detected is to prevent more concurrent updates, so that the retry is guaranteed to succeed, then wouldn't even LockTupleNoKeyExclusive be sufficient in all cases? After all, that does block concurrent updates and deletes. Perhaps there is an issue with using LockTupleNoKeyExclusive, and then having to upgrade it to LockTupleExclusive later? But I wonder if that can already happen -- consider a regular UPDATE (not via MERGE) of non-key columns on a partitioned table, that initially does a simple update, but upon retrying needs to do a cross-partition update (DELETE + INSERT). But perhaps I'm thinking about this in the wrong way. Do you have an example test case where this makes a difference? Regards, Dean
On 2023-Mar-13, Dean Rasheed wrote: > I'm wondering now if it really matters what lock mode we use here. If > the point of calling table_tuple_lock() after a concurrent update is > detected is to prevent more concurrent updates, so that the retry is > guaranteed to succeed, then wouldn't even LockTupleNoKeyExclusive be > sufficient in all cases? After all, that does block concurrent updates > and deletes. The difference in lock mode should be visible relative to concurrent transactions that try to SELECT FOR KEY SHARE the affected row. If you are updating a row but not changing the key-columns, then a KEY SHARE against the same tuple should work concurrently without blocking. If you *are* changing the key columns, then such a lock should be made to wait. DELETE should be exactly equivalent to an update that changes any columns in the "key". After all, the point is that the previous key (as referenced via a FK from another table) is now gone, which happens in both these operations, but does not happen when an update only touches other columns. Two UPDATEs of the same row should always block each other. Note that the code to determine which columns are part of the key is not very careful: IIRC any column part of a unique index is considered part of the key. I don't think this has any implications for the discussion here, but I thought I'd point it out just in case. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On 2023-Mar-11, Alexander Korotkov wrote: > I wonder why does ExecMergeMatched() determine the lock mode using > ExecUpdateLockMode(). Why don't we use lock mode set by > table_tuple_update() like ExecUpdate() does? I skim through the > MERGE-related threads, but didn't find an answer. > > I also noticed that we use ExecUpdateLockMode() even for CMD_DELETE. > That ends up by usage of LockTupleNoKeyExclusive for CMD_DELETE, which > seems plain wrong for me. I agree that in the case of CMD_DELETE it should not run ExecUpdateLockMode() --- that part seems like a bug. As I recall, ExecUpdateLockMode is newer code that should do the same as table_tuple_update does to determine the lock mode ... and looking at the code, I see that both do a bms_overlap operation on "columns in the key" vs. "columns modified", so I'm not sure why you say they would behave differently. Thinking about Dean's comment downthread, where an UPDATE could be turned into a DELETE, I wonder if trying to be selective would lead us to deadlock, in case a concurrent SELECT FOR KEY SHARE is able to lock the tuple while we're doing UPDATE, and then lock out the MERGE when the DELETE is retried. If this is indeed a problem, then I can think of two ways out: 1. if MERGE contains any DELETE, then always use LockTupleExclusive: otherwise, use LockTupleNoKeyExclusive. This is best for concurrency when MERGE does no delete and the key columns are not modified. 2. always use LockTupleExclusive. This is easier, but does not allow MERGE to run concurrently with SELECT FOR KEY SHARE on the same tuples. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/