Re: upgrades in row-level locks can deadlock

Hello,

Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> On 2019-Jun-12, Oleksii Kliukin wrote:
>
>> Thank you! I can make it even simpler;  s1 just acquires for share lock, s3
>> gets for update one and s2 takes for share lock first, and then tries to
>> acquire for update one; once s1 finishes, s3 deadlocks.
>
> Cool.  I think it would be worthwhile to include a number of reasonable
> permutations instead of just one, and make sure they all work correctly.
> I don't think we need to include all possible permutations, just a few.
> I think we need at least enough permutations to cover the two places of
> the code that are modified by the patch, for both values of
> have_current_xid (so there should be four permutations, I think).

Makes sense. For the symmetry I have included those that perform lock
upgrades in one session and those that doesn’t, while the other sessions
acquire locks, do updates or deletes. For those that don’t upgrade locks the
test checks that the locks are acquired in the correct order.

> Please don't simplify the table name to just "t" -- the reason I used
> another name is that we want these tests to be able to run concurrently
> at some point; ref.
> https://postgr.es/m/20180124231006.z7spaz5gkzbdvob5@alvherre.pgsql

Alright, thanks.

>
>>> But semantically, I wonder if your transactions are correct.  If you
>>> intend to modify the row in s1 and s2, shouldn't you be acquiring FOR NO
>>> KEY UPDATE lock instead?  I don't see how can s1 and s2 coexist
>>> peacefully.  Also, can your Y transaction use FOR NO KEY UPDATE instead
>>> .. unless you intend to delete the tuple in that transaction?
>>
>> It is correct. I wanted to make sure jobs that acquire for key share lock
>> can run concurrently most of the time; they only execute one update at the
>> end of the job, and it is just to update the last run timestamp.
>
> I see.  Under READ COMMITTED it works okay, I suppose.
>
>>> I'm mulling over your proposed fix.  I don't much like the idea that
>>> DoesMultiXactIdConflict() returns that new boolean -- seems pretty
>>> ad-hoc -- but I don't see any way to do better than that ...  (If we get
>>> down to details, DoesMultiXactIdConflict needn't initialize that
>>> boolean: better let the callers do.)
>>
>> I am also not happy about the new parameter to DoesMultiXactIdConflict, but
>> calling a separate function to fetch the presence of the current transaction
>> in the multixact would mean doing the job of DoesMultiXactIdConflict twice.
>> I am open to suggestions on how to make it nicer.
>
> Yeah, I didn't find anything better either.  We could make things more
> complex that we resolve the multixact once and then extract the two
> sepraate bits of information that we need from that ... but it ends up
> being uglier and messier for no real gain.  So let's go with your
> original idea.

Ok, the v4 is attached. I have addressed your suggestion for the isolation
tests, added a paragraph to README.tuplock explaining why do we skip
LockTuple to avoid a deadlock in the session that upgrades its lock.

Cheers,
Oleksii