Fix serialization anomalies due to race conditions on INSERT.

On insert the CheckForSerializableConflictIn() test was performed
before the page(s) which were going to be modified had been locked
(with an exclusive buffer content lock).  If another process
acquired a relation SIReadLock on the heap and scanned to a page on
which an insert was going to occur before the page was so locked,
a rw-conflict would be missed, which could allow a serialization
anomaly to be missed.  The window between the check and the page
lock was small, so the bug was generally not noticed unless there
was high concurrency with multiple processes inserting into the
same table.

This was reported by Peter Bailis as bug #11732, by Sean Chittenden
as bug #13667, and by others.

The race condition was eliminated in heap_insert() by moving the
check down below the acquisition of the buffer lock, which had been
the very next statement.  Because of the loop locking and unlocking
multiple buffers in heap_multi_insert() a check was added after all
inserts were completed.  The check before the start of the inserts
was left because it might avoid a large amount of work to detect a
serialization anomaly before performing the all of the inserts and
the related WAL logging.

While investigating this bug, other SSI bugs which were even harder
to hit in practice were noticed and fixed, an unnecessary check
(covered by another check, so redundant) was removed from
heap_update(), and comments were improved.

Back-patch to all supported branches.

Kevin Grittner and Thomas Munro

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 6645732f74c2f51974d3e2d12e2f0dc120048e32..1273d1201ffa49b511dfe14d6d4a783681da976e 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1929,22 +1929,27 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
        else
                heaptup = tup;
 
+       /* Find buffer to insert this tuple into */
+       buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
+                                                                          InvalidBuffer, options, bistate);
+
        /*
-        * We're about to do the actual insert -- but check for conflict first,
-        * to avoid possibly having to roll back work we've just done.
+        * We're about to do the actual insert -- but check for conflict first, to
+        * avoid possibly having to roll back work we've just done.
+        *
+        * This is safe without a recheck as long as there is no possibility of
+        * another process scanning the page between this check and the insert
+        * being visible to the scan (i.e., an exclusive buffer content lock is
+        * continuously held from this point until the tuple insert is visible).
         *
-        * For a heap insert, we only need to check for table-level SSI locks.
-        * Our new tuple can't possibly conflict with existing tuple locks, and
-        * heap page locks are only consolidated versions of tuple locks; they do
-        * not lock "gaps" as index page locks do.  So we don't need to identify
-        * a buffer before making the call.
+        * For a heap insert, we only need to check for table-level SSI locks. Our
+        * new tuple can't possibly conflict with existing tuple locks, and heap
+        * page locks are only consolidated versions of tuple locks; they do not
+        * lock "gaps" as index page locks do.  So we don't need to specify a
+        * buffer when making the call, which makes for a faster check.
         */
        CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
 
-       /* Find buffer to insert this tuple into */
-       buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
-                                                                          InvalidBuffer, options, bistate);
-
        /* NO EREPORT(ERROR) from here till changes are logged */
        START_CRIT_SECTION();
 
@@ -2248,6 +2253,11 @@ l1:
        /*
         * We're about to do the actual delete -- check for conflict first, to
         * avoid possibly having to roll back work we've just done.
+        *
+        * This is safe without a recheck as long as there is no possibility of
+        * another process scanning the page between this check and the delete
+        * being visible to the scan (i.e., an exclusive buffer content lock is
+        * continuously held from this point until the tuple delete is visible).
         */
        CheckForSerializableConflictIn(relation, &tp, buffer);
 
@@ -2604,12 +2614,6 @@ l2:
                return result;
        }
 
-       /*
-        * We're about to do the actual update -- check for conflict first, to
-        * avoid possibly having to roll back work we've just done.
-        */
-       CheckForSerializableConflictIn(relation, &oldtup, buffer);
-
        /* Fill in OID and transaction status data for newtup */
        if (relation->rd_rel->relhasoids)
        {
@@ -2755,14 +2759,20 @@ l2:
        }
 
        /*
-        * We're about to create the new tuple -- check for conflict first, to
+        * We're about to do the actual update -- check for conflict first, to
         * avoid possibly having to roll back work we've just done.
         *
-        * NOTE: For a tuple insert, we only need to check for table locks, since
-        * predicate locking at the index level will cover ranges for anything
-        * except a table scan.  Therefore, only provide the relation.
+        * This is safe without a recheck as long as there is no possibility of
+        * another process scanning the pages between this check and the update
+        * being visible to the scan (i.e., exclusive buffer content lock(s) are
+        * continuously held from this point until the tuple update is visible).
+        *
+        * For the new tuple the only check needed is at the relation level, but
+        * since both tuples are in the same relation and the check for oldtup
+        * will include checking the relation level, there is no benefit to a
+        * separate check for the new tuple.
         */
-       CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
+       CheckForSerializableConflictIn(relation, &oldtup, buffer);
 
        /*
         * At this point newbuf and buffer are both pinned and locked, and newbuf

diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index b9c6d8edee2daa7009dbc32da4b9257be56141b1..1c1617f8e942dd4f82411e517311f28a45be9dcd 100644 (file)
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -3135,22 +3135,21 @@ ReleasePredicateLocks(bool isCommit)
                return;
        }
 
+       LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
+
        Assert(!isCommit || SxactIsPrepared(MySerializableXact));
        Assert(!isCommit || !SxactIsDoomed(MySerializableXact));
        Assert(!SxactIsCommitted(MySerializableXact));
        Assert(!SxactIsRolledBack(MySerializableXact));
 
        /* may not be serializable during COMMIT/ROLLBACK PREPARED */
-       if (MySerializableXact->pid != 0)
-               Assert(IsolationIsSerializable());
+       Assert(MySerializableXact->pid == 0 || IsolationIsSerializable());
 
        /* We'd better not already be on the cleanup list. */
        Assert(!SxactIsOnFinishedList(MySerializableXact));
 
        topLevelIsDeclaredReadOnly = SxactIsReadOnly(MySerializableXact);
 
-       LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
-
        /*
         * We don't hold XidGenLock lock here, assuming that TransactionId is
         * atomic!
@@ -4286,7 +4285,7 @@ CheckTableForSerializableConflictIn(Relation relation)
        LWLockAcquire(SerializablePredicateLockListLock, LW_EXCLUSIVE);
        for (i = 0; i < NUM_PREDICATELOCK_PARTITIONS; i++)
                LWLockAcquire(FirstPredicateLockMgrLock + i, LW_SHARED);
-       LWLockAcquire(SerializableXactHashLock, LW_SHARED);
+       LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
 
        /* Scan through target list */
        hash_seq_init(&seqstat, PredicateLockTargetHash);