bug in locking an update tuple chain
A customer of ours reported a problem in 9.3.14 while inserting tuples
in a table with a foreign key, with many concurrent transactions doing
the same: after a few insertions worked sucessfully, a later one would
return failure indicating that the primary key value was not present in
the referenced table. It worked fine for them on 9.3.4.
After some research, we determined that the problem disappeared if
commit this commit was reverted:
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Branch: master Release: REL9_6_BR [533e9c6b0] 2016-07-15 14:17:20 -0400
Branch: REL9_5_STABLE Release: REL9_5_4 [649dd1b58] 2016-07-15 14:17:20 -0400
Branch: REL9_4_STABLE Release: REL9_4_9 [166873dd0] 2016-07-15 14:17:20 -0400
Branch: REL9_3_STABLE Release: REL9_3_14 [6c243f90a] 2016-07-15 14:17:20 -0400
Avoid serializability errors when locking a tuple with a committed update
I spent some time writing an isolationtester spec to reproduce the
problem. It turned out that this required six concurrent sessions in
order for the problem to show up at all, but once I had that, figuring
out what was going on was simple: a transaction wants to lock the
updated version of some tuple, and it does so; and some other
transaction is also locking the same tuple concurrently in a compatible
way. So both are okay to proceed concurrently. The problem is that if
one of them detects that anything changed in the process of doing this
(such as the other session updating the multixact to include itself,
both having compatible lock modes), it loops back to ensure xmax/
infomask are still sane; but heap_lock_updated_tuple_rec is not prepared
to deal with the situation of "the current transaction has the lock
already", so it returns a failure and the tuple is returned as "not
visible" causing the described problem.
I *think* that the problem did not show up before the commit cited above
because the bug fixed by that commit reduced concurrency, effectively
masking this bug.
In assertion-enabled builds, this happens instead (about 2 out of 3
times I run the script in my laptop):
TRAP: FailedAssertion(�!(TransactionIdIsCurrentTransactionId(((!((tup)->t_infomask & 0x0800) && ((tup)->t_infomask & 0x1000) && !((tup)->t_infomask & 0x0080)) ? HeapTupleGetUpdateXid(tup) : ( (tup)->t_choice.t_heap.t_xmax) )))�, Archivo: �/pgsql/source/REL9_3_STABLE/src/backend/utils/time/combocid.c�, L�nea: 122)
with this backtrace:
(gdb) bt
#0 0x00007f651cd66067 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1 0x00007f651cd67448 in __GI_abort () at abort.c:89
#2 0x0000000000754ac1 in ExceptionalCondition (
conditionName=conditionName@entry=0x900d68 "!(TransactionIdIsCurrentTransactionId(( (!((tup)->t_infomask & 0x0800) && ((tup)->t_infomask & 0x1000) && !
+((tup)->t_infomask & 0x0080)) ? HeapTupleGetUpdateXid(tup) : ( (tup)->t_choice.t_heap.t_xmax "...,
errorType=errorType@entry=0x78cdb4 "FailedAssertion",
fileName=fileName@entry=0x900ca8 "/pgsql/source/REL9_3_STABLE/src/backend/utils/time/combocid.c", lineNumber=lineNumber@entry=122)
at /pgsql/source/REL9_3_STABLE/src/backend/utils/error/assert.c:54
#3 0x0000000000781cf8 in HeapTupleHeaderGetCmax (tup=0x7f6513f289d0)
at /pgsql/source/REL9_3_STABLE/src/backend/utils/time/combocid.c:122
#4 0x0000000000495911 in heap_lock_tuple (relation=0x7f651e0d9138, tuple=0x7ffe928a7de0, cid=0, mode=LockTupleKeyShare,
nowait=0 '\000', follow_updates=1 '\001', buffer=0x7ffe928a7dcc, hufd=0x7ffe928a7dd0)
at /pgsql/source/REL9_3_STABLE/src/backend/access/heap/heapam.c:4439
#5 0x00000000005b2f74 in ExecLockRows (node=0x290f070) at /pgsql/source/REL9_3_STABLE/src/backend/executor/nodeLockRows.c:134
The attached patch fixes the problem. When locking some old tuple version of
the chain, if we detect that we already hold that lock
(test_lockmode_for_conflict returns HeapTupleSelfUpdated), do not try to lock
it again but instead skip ahead to the next version. This fixes the synthetic
case in my isolationtester as well as our customer's production case.
I attach the isolationtester spec file. In its present form it's rather
noisy, because I added calls to report the XIDs for each transaction, so
that I could exactly replicate the WAL sequence that I obtained from the
customer in order to reproduce the issue. That would have to be removed
in order for the test to be included in the repository, of course. This
spec doesn't work at all with 9.3's isolationtester, because back then
isolationtester had the limitation that only one session could be
waiting; to verify this bug, I backpatched 38f8bdcac498 ("Modify the
isolation tester so that multiple sessions can wait.") so that it'd work
at all. This means that we cannot include the test in branches older
than 9.6.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-if-we-already-hold-lock-skip-this-tuple.patchtext/plain; charset=us-asciiDownload
From d065f5e91c8e2c23debf09252e5f7eb94f99dff4 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 20 Jun 2017 18:15:25 -0400
Subject: [PATCH] if we already hold lock, skip this tuple
---
src/backend/access/heap/heapam.c | 41 ++++++++++++++++++++++++++++++++++++----
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 898950d9a0..ae0b214519 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4906,8 +4906,10 @@ l5:
* with the given xid, does the current transaction need to wait, fail, or can
* it continue if it wanted to acquire a lock of the given mode? "needwait"
* is set to true if waiting is necessary; if it can continue, then
- * HeapTupleMayBeUpdated is returned. In case of a conflict, a different
- * HeapTupleSatisfiesUpdate return code is returned.
+ * HeapTupleMayBeUpdated is returned. If the lock is already held by the
+ * current transaction, return HeapTupleSelfUpdated. In case of a conflict
+ * with another transaction, a different HeapTupleSatisfiesUpdate return code
+ * is returned.
*
* The held status is said to be hypothetical because it might correspond to a
* lock held by a single Xid, i.e. not a real MultiXactId; we express it this
@@ -4930,8 +4932,9 @@ test_lockmode_for_conflict(MultiXactStatus status, TransactionId xid,
if (TransactionIdIsCurrentTransactionId(xid))
{
/*
- * Updated by our own transaction? Just return failure. This shouldn't
- * normally happen.
+ * The tuple has already been locked by our own transaction. This is
+ * very rare but can happen if multiple transactions are trying to
+ * lock an ancient version of the same tuple.
*/
return HeapTupleSelfUpdated;
}
@@ -5100,6 +5103,22 @@ l4:
members[i].xid,
mode, &needwait);
+ /*
+ * If the tuple was already locked by ourselves in a
+ * previous iteration of this (say heap_lock_tuple was
+ * forced to restart the locking loop because of a change
+ * in xmax), then we hold the lock already on this tuple
+ * version and we don't need to do anything; and this is
+ * not an error condition either. We just need to skip
+ * this tuple and continue locking the next version in the
+ * update chain.
+ */
+ if (res == HeapTupleSelfUpdated)
+ {
+ pfree(members);
+ goto next;
+ }
+
if (needwait)
{
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
@@ -5160,6 +5179,19 @@ l4:
res = test_lockmode_for_conflict(status, rawxmax, mode,
&needwait);
+
+ /*
+ * If the tuple was already locked by ourselves in a previous
+ * iteration of this (say heap_lock_tuple was forced to
+ * restart the locking loop because of a change in xmax), then
+ * we hold the lock already on this tuple version and we don't
+ * need to do anything; and this is not an error condition
+ * either. We just need to skip this tuple and continue
+ * locking the next version in the update chain.
+ */
+ if (res == HeapTupleSelfUpdated)
+ goto next;
+
if (needwait)
{
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
@@ -5221,6 +5253,7 @@ l4:
END_CRIT_SECTION();
+next:
/* if we find the end of update chain, we're done. */
if (mytup.t_data->t_infomask & HEAP_XMAX_INVALID ||
ItemPointerEquals(&mytup.t_self, &mytup.t_data->t_ctid) ||
--
2.11.0
On Sat, Jul 15, 2017 at 2:30 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
A customer of ours reported a problem in 9.3.14 while inserting tuples
in a table with a foreign key, with many concurrent transactions doing
the same: after a few insertions worked sucessfully, a later one would
return failure indicating that the primary key value was not present in
the referenced table. It worked fine for them on 9.3.4.After some research, we determined that the problem disappeared if
commit this commit was reverted:Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Branch: master Release: REL9_6_BR [533e9c6b0] 2016-07-15 14:17:20 -0400
Branch: REL9_5_STABLE Release: REL9_5_4 [649dd1b58] 2016-07-15 14:17:20 -0400
Branch: REL9_4_STABLE Release: REL9_4_9 [166873dd0] 2016-07-15 14:17:20 -0400
Branch: REL9_3_STABLE Release: REL9_3_14 [6c243f90a] 2016-07-15 14:17:20 -0400Avoid serializability errors when locking a tuple with a committed update
I spent some time writing an isolationtester spec to reproduce the
problem. It turned out that this required six concurrent sessions in
order for the problem to show up at all, but once I had that, figuring
out what was going on was simple: a transaction wants to lock the
updated version of some tuple, and it does so; and some other
transaction is also locking the same tuple concurrently in a compatible
way. So both are okay to proceed concurrently. The problem is that if
one of them detects that anything changed in the process of doing this
(such as the other session updating the multixact to include itself,
both having compatible lock modes), it loops back to ensure xmax/
infomask are still sane; but heap_lock_updated_tuple_rec is not prepared
to deal with the situation of "the current transaction has the lock
already", so it returns a failure and the tuple is returned as "not
visible" causing the described problem.
Your fix seems logical to me, though I have not tested it till now.
However, I wonder why heap_lock_tuple need to restart from the
beginning of update-chain in this case?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Kapila wrote:
On Sat, Jul 15, 2017 at 2:30 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
a transaction wants to lock the
updated version of some tuple, and it does so; and some other
transaction is also locking the same tuple concurrently in a compatible
way. So both are okay to proceed concurrently. The problem is that if
one of them detects that anything changed in the process of doing this
(such as the other session updating the multixact to include itself,
both having compatible lock modes), it loops back to ensure xmax/
infomask are still sane; but heap_lock_updated_tuple_rec is not prepared
to deal with the situation of "the current transaction has the lock
already", so it returns a failure and the tuple is returned as "not
visible" causing the described problem.Your fix seems logical to me, though I have not tested it till now.
However, I wonder why heap_lock_tuple need to restart from the
beginning of update-chain in this case?
Well, it's possible that we could change things so that it doesn't need
to re-start from the same spot where it initially began, but I think it
requires changing too much code; I'd rather not touch it in a
back-patchable bug fix. If we really wanted, we could perhaps change
things to avoid repeated walks of the chain, but I'd see that as a pg11
(or future) change only. (You would be forgiven for thinking that the
interactions between EvalPlanQualFetch, heap_lock_tuple and
heap_lock_update_tuple are rather Rube Goldbergian, to use Tom's term.)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
The attached patch fixes the problem. When locking some old tuple version of
the chain, if we detect that we already hold that lock
(test_lockmode_for_conflict returns HeapTupleSelfUpdated), do not try to lock
it again but instead skip ahead to the next version. This fixes the synthetic
case in my isolationtester as well as our customer's production case.
Pushed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers