Horton

Thread

Commits

  1. Avoid possible deadlock while locking multiple heap pages.

  2. Try again to make the visibility map crash safe.

  1. possible deadlock: different lock ordering for heap pages

    Nishant, Fnu <nishantf@amazon.com> — 2019-01-18T22:31:55Z

    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.
Thanks,
Nishant

  2. Re: possible deadlock: different lock ordering for heap pages

    Amit Kapila <amit.kapila16@gmail.com> — 2019-01-20T14:54:50Z

    On Sat, Jan 19, 2019 at 4:02 AM Nishant, Fnu <nishantf@amazon.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@postgresql.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

  3. Re: possible deadlock: different lock ordering for heap pages

    Nishant, Fnu <nishantf@amazon.com> — 2019-01-21T17:09:38Z

    Thanks Amit for your review.

On 1/20/19, 6:55 AM, "Amit Kapila" <amit.kapila16@gmail.com> wrote:
    > I think you need to change below code as well....
       Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);

Done. Updated the patch.

Will wait for Robert comments.

Thanks
-Nishant

  4. Re: possible deadlock: different lock ordering for heap pages

    Nishant, Fnu <nishantf@amazon.com> — 2019-01-28T20:47:04Z

    Thanks David and Amit for reviewing the patch.
Attaching revised patch addressing review comments.
-Nishant



  5. Re: possible deadlock: different lock ordering for heap pages

    Robert Haas <robertmhaas@gmail.com> — 2019-01-31T19:00:44Z

    On Sun, Jan 20, 2019 at 9:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Robert, can you please once see if we are missing anything here
> because to me the report and fix look genuine.

I think so, too.  I guess the probability of a deadlock here must be *very* low.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  6. Re: possible deadlock: different lock ordering for heap pages

    Amit Kapila <amit.kapila16@gmail.com> — 2019-02-01T05:20:49Z

    On Mon, Jan 21, 2019 at 10:39 PM Nishant, Fnu <nishantf@amazon.com> wrote:
>
> Thanks Amit for your review.
>
> On 1/20/19, 6:55 AM, "Amit Kapila" <amit.kapila16@gmail.com> wrote:
>     > I think you need to change below code as well....
>        Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);
>
> Done. Updated the patch.
>

Attached is an updated patch.  I have edited comments and commit
message in the patch.  I would like to backpatch this till 9.4 unless
anyone thinks otherwise.

BTW, do you have a reproducible test case for this fix?  How did you
catch this, browsing code?

How do we pronounce your name, is Nishant Fnu okay?  I would like to
mention you as the author of the patch, so need to write your name.

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

  7. Re: possible deadlock: different lock ordering for heap pages

    Nishant, Fnu <nishantf@amazon.com> — 2019-02-01T05:30:18Z

    
On 1/31/19, 9:21 PM, "Amit Kapila" <amit.kapila16@gmail.com> wrote:


    > BTW, do you have a reproducible test case for this fix?  How did you
    catch this, browsing code?
    
Yes, I observed it while reading code. I do not have a repro.

    > How do we pronounce your name, is Nishant Fnu okay?  I would like to
    mention you as the author of the patch, so need to write your name.
Yes, Nishant Fnu is okay.

Thanks
Nishant

  8. Re: possible deadlock: different lock ordering for heap pages

    Amit Kapila <amit.kapila16@gmail.com> — 2019-02-04T02:29:08Z

    On Fri, Feb 1, 2019 at 10:50 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jan 21, 2019 at 10:39 PM Nishant, Fnu <nishantf@amazon.com> wrote:
> >
> > Thanks Amit for your review.
> >
> > On 1/20/19, 6:55 AM, "Amit Kapila" <amit.kapila16@gmail.com> wrote:
> >     > I think you need to change below code as well....
> >        Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);
> >
> > Done. Updated the patch.
> >
>
> Attached is an updated patch.  I have edited comments and commit
> message in the patch.  I would like to backpatch this till 9.4 unless
> anyone thinks otherwise.
>

Pushed this patch and backpatched till 9.4.

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