Обсуждение: possible deadlock: different lock ordering for heap pages
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
Вложения
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
numberorder to avoid deadlock. In certain cases, we do have to drop and reacquire lock on heap pages (when we have to
pinvisibility map pages) to avoid doing IO while holding exclusive lock. The problem is we now acquire locks in
bufferIdorder, 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
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
Вложения
Thanks David and Amit for reviewing the patch.
Attaching revised patch addressing review comments.
-Nishant
Вложения
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
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
Вложения
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
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