From: Kevin Grittner Date: Sat, 31 Oct 2015 19:36:58 +0000 (-0500) Subject: Fix serialization anomalies due to race conditions on INSERT. X-Git-Tag: REL9_1_20~63 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=08322daed33e0c35b77194c4ad70dbd21790c7e6;p=postgresql 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 6645732f74..1273d1201f 100644 --- 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 b9c6d8edee..1c1617f8e9 100644 --- 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);