Re: possible deadlock: different lock ordering for heap pages

On Sat, Jan 19, 2019 at 4:02 AM Nishant, Fnu <nishantf(at)amazon(dot)com> wrote:
>
> Hello,
>
> While locking heap pages (function RelationGetBufferForTuple() in file hio.c) we acquire locks in increasing block number order to avoid deadlock. In certain cases, we do have to drop and reacquire lock on heap pages (when we have to pin visibility map pages) to avoid doing IO while holding exclusive lock. The problem is we now acquire locks in bufferId order, which looks like a slip and the intention was to grab it in block number order. Since it is a trivial change, I am attaching a patch to correct it.
>

On a quick look, your analysis seems correct, but I am bit surprised
to see how this survived so long without being caught. I think you
need to change below code as well if your analysis and fix is correct.
GetVisibilityMapPins()
{
..
Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);
..
}

AFAICS, this code has been added by below commit, so adding author in the loop.

commit e16954f3d27fa8e16c379ff6623ae18d6250a39c
Author: Robert Haas <rhaas(at)postgresql(dot)org>
Date: Mon Jun 27 13:55:55 2011 -0400

Try again to make the visibility map crash safe.

My previous attempt was quite a bit less than half-baked with respect to
heap_update().

Robert, can you please once see if we are missing anything here
because to me the report and fix look genuine.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com