Date: Wed, 12 Jun 2019 12:46:49 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Oleksii Kliukin <alexk@hintbits.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: upgrades in row-level locks can deadlock
Message-ID: <20190612164649.GA10230@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <E621393D-A213-41B0-8C9D-584F8EC9287F@hintbits.com>
 <62A83BCF-1B90-44F8-BADF-69F750E677B6@hintbits.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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).

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

> > 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.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services