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(at)alvh(dot)no-ip(dot)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(at)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(at)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(at)entry=0x78cdb4 "FailedAssertion",
fileName=fileName(at)entry=0x900ca8 "/pgsql/source/REL9_3_STABLE/src/backend/utils/time/combocid.c", lineNumber=lineNumber(at)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