Fix race condition that could allow two concurrent transactions

to insert the same key into a supposedly unique index.  The bug is of
low probability, and may not explain any of the recent reports of
duplicated rows; but a bug is a bug.

diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 1d3a7e82ab0dae2ee5a6c3719f35601301e24511..fa19d7741d79c02ed80581cf23ff0206f383cc7d 100644 (file)
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $Header: /cvsroot/pgsql/src/backend/access/nbtree/nbtinsert.c,v 1.87 2001/10/25 05:49:21 momjian Exp $
+ *   $Header: /cvsroot/pgsql/src/backend/access/nbtree/nbtinsert.c,v 1.88 2002/01/01 20:32:37 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -75,7 +75,6 @@ static void _bt_pgaddtup(Relation rel, Page page,
 static bool _bt_isequal(TupleDesc itupdesc, Page page, OffsetNumber offnum,
            int keysz, ScanKey scankey);
 
-static Relation _xlheapRel;        /* temporary hack */
 
 /*
  * _bt_doinsert() -- Handle insertion of a single btitem in the tree.
@@ -116,7 +115,21 @@ top:
 
    /*
     * If we're not allowing duplicates, make sure the key isn't already
-    * in the index.  XXX this belongs somewhere else, likely
+    * in the index.
+    *
+    * NOTE: obviously, _bt_check_unique can only detect keys that are
+    * already in the index; so it cannot defend against concurrent
+    * insertions of the same key.  We protect against that by means
+    * of holding a write lock on the target page.  Any other would-be
+    * inserter of the same key must acquire a write lock on the same
+    * target page, so only one would-be inserter can be making the check
+    * at one time.  Furthermore, once we are past the check we hold
+    * write locks continuously until we have performed our insertion,
+    * so no later inserter can fail to see our insertion.  (This
+    * requires some care in _bt_insertonpg.)
+    *
+    * If we must wait for another xact, we release the lock while waiting,
+    * and then must start over completely.
     */
    if (index_is_unique)
    {
@@ -135,8 +148,6 @@ top:
        }
    }
 
-   _xlheapRel = heapRel;       /* temporary hack */
-
    /* do the insertion */
    res = _bt_insertonpg(rel, buf, stack, natts, itup_scankey, btitem, 0);
 
@@ -397,9 +408,16 @@ _bt_insertonpg(Relation rel,
        {
            /* step right one page */
            BlockNumber rblkno = lpageop->btpo_next;
+           Buffer  rbuf;
 
+           /*
+            * must write-lock next page before releasing write lock on
+            * current page; else someone else's _bt_check_unique scan
+            * could fail to see our insertion.
+            */
+           rbuf = _bt_getbuf(rel, rblkno, BT_WRITE);
            _bt_relbuf(rel, buf);
-           buf = _bt_getbuf(rel, rblkno, BT_WRITE);
+           buf = rbuf;
            page = BufferGetPage(buf);
            lpageop = (BTPageOpaque) PageGetSpecialPointer(page);
            movedright = true;
@@ -833,7 +851,6 @@ _bt_split(Relation rel, Buffer buf, OffsetNumber firstright,
     * page is not updated yet. Log changes before continuing.
     *
     * NO ELOG(ERROR) till right sibling is updated.
-    *
     */
    START_CRIT_SECTION();
    {