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
Attachments:
lock-order.patchapplication/octet-stream; name=lock-order.patchDownload
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 3da0b49ccc..236d7f9f61 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -465,7 +465,7 @@ loop:
* done a bit of extra work for no gain, but there's no real harm
* done.
*/
- if (otherBuffer == InvalidBuffer || buffer <= otherBuffer)
+ if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
GetVisibilityMapPins(relation, buffer, otherBuffer,
targetBlock, otherBlock, vmbuffer,
vmbuffer_other);
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
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
Attachments:
lock-order.patchapplication/octet-stream; name=lock-order.patchDownload
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 3da0b49ccc..98dfca6cc9 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -127,7 +127,7 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
bool need_to_pin_buffer2;
Assert(BufferIsValid(buffer1));
- Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);
+ Assert(buffer2 == InvalidBuffer || block1 <= block2);
while (1)
{
@@ -465,7 +465,7 @@ loop:
* done a bit of extra work for no gain, but there's no real harm
* done.
*/
- if (otherBuffer == InvalidBuffer || buffer <= otherBuffer)
+ if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
GetVisibilityMapPins(relation, buffer, otherBuffer,
targetBlock, otherBlock, vmbuffer,
vmbuffer_other);
Thanks David and Amit for reviewing the patch.
Attaching revised patch addressing review comments.
-Nishant
Attachments:
lock-order.patchapplication/octet-stream; name=lock-order.patchDownload
From 870e2520aa7b9881e2619d85fa46bac8a27817e4 Mon Sep 17 00:00:00 2001
From: Fnu Nishant <nishantf@amazon.com>
Date: Mon, 28 Jan 2019 19:48:47 +0000
Subject: [PATCH] possible deadlock locking multiple heap pages
To avoid deadlock, Postgres backend acquire lock on heap pages in
blockid order. In certain cases, lock on heap pages are dropped and
reacquired. In this case the locks are dropped for reading in
corresponding VM page/s. The bug is we re-acquire locks in bufferId
order where as the intention was to acquire in blockid order. Post this
patch we will always acquire locks in blockid order.
---
src/backend/access/heap/hio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 3da0b49ccc..98dfca6cc9 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -127,7 +127,7 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
bool need_to_pin_buffer2;
Assert(BufferIsValid(buffer1));
- Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);
+ Assert(buffer2 == InvalidBuffer || block1 <= block2);
while (1)
{
@@ -465,7 +465,7 @@ loop:
* done a bit of extra work for no gain, but there's no real harm
* done.
*/
- if (otherBuffer == InvalidBuffer || buffer <= otherBuffer)
+ if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
GetVisibilityMapPins(relation, buffer, otherBuffer,
targetBlock, otherBlock, vmbuffer,
vmbuffer_other);
--
2.15.3.AMZN
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
Attachments:
lock-order.1.patchapplication/octet-stream; name=lock-order.1.patchDownload
From 8d724382d8a82c007798d735acdeb5ba05911be9 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 1 Feb 2019 10:40:12 +0530
Subject: [PATCH] Avoid possible deadlock while locking multiple heap pages.
To avoid deadlock, backend acquires a lock on heap pages in block
number order. In certain cases, lock on heap pages is dropped and
reacquired. In this case, the locks are dropped for reading in
corresponding VM page/s. The issue is we re-acquire locks in bufferId
order whereas the intention was to acquire in blockid order.
This commit ensures that we will always acquire locks on heap pages in
blockid order.
Reported-by: Nishant Fnu
Author: Nishant Fnu
Reviewed-by: Amit Kapila and Robert Haas
Backpatch-through: 9.4
Discussion: https://postgr.es/m/5883C831-2ED1-47C8-BFAC-2D5BAE5A8CAE@amazon.com
---
src/backend/access/heap/hio.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 3da0b49..5839c16 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -115,8 +115,8 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
* visibility map page, if we haven't already got one.
*
* buffer2 may be InvalidBuffer, if only one buffer is involved. buffer1
- * must not be InvalidBuffer. If both buffers are specified, buffer1 must
- * be less than buffer2.
+ * must not be InvalidBuffer. If both buffers are specified, block1 must
+ * be less than block2.
*/
static void
GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
@@ -127,7 +127,7 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
bool need_to_pin_buffer2;
Assert(BufferIsValid(buffer1));
- Assert(buffer2 == InvalidBuffer || buffer1 <= buffer2);
+ Assert(buffer2 == InvalidBuffer || block1 <= block2);
while (1)
{
@@ -465,7 +465,7 @@ loop:
* done a bit of extra work for no gain, but there's no real harm
* done.
*/
- if (otherBuffer == InvalidBuffer || buffer <= otherBuffer)
+ if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
GetVisibilityMapPins(relation, buffer, otherBuffer,
targetBlock, otherBlock, vmbuffer,
vmbuffer_other);
--
1.8.3.1
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