Merge pull request #785 from katzyn/MVSecondaryIndex

Optimize NULL handling in MVSecondaryIndex.add()

h2/src/main/org/h2/engine/Mode.java

@@ -9,7 +9,6 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Set;
 import java.util.regex.Pattern;
-import org.h2.util.New;
 import org.h2.util.StringUtils;
 
 /**
@@ -22,6 +21,29 @@ public class Mode {
         REGULAR, DB2, Derby, MSSQLServer, HSQLDB, MySQL, Oracle, PostgreSQL, Ignite,
     }
 
+    /**
+     * Determines how rows with {@code NULL} values in indexed columns are handled
+     * in unique indexes.
+     */
+    public enum UniqueIndexNullsHandling {
+        /**
+         * Multiple identical indexed columns with at least one {@code NULL} value are
+         * allowed in unique index.
+         */
+        ALLOW_DUPLICATES_WITH_ANY_NULL,
+
+        /**
+         * Multiple identical indexed columns with all {@code NULL} values are allowed
+         * in unique index.
+         */
+        ALLOW_DUPLICATES_WITH_ALL_NULLS,
+
+        /**
+         * Multiple identical indexed columns are not allowed in unique index.
+         */
+        FORBID_ANY_DUPLICATES;
+    }
+
     private static final HashMap<String, Mode> MODES = new HashMap<>();
 
     // Modes are also documented in the features section
@@ -87,17 +109,10 @@ public class Mode {
     public boolean systemColumns;
 
     /**
-     * For unique indexes, NULL is distinct. That means only one row with NULL
-     * in one of the columns is allowed.
-     */
-    public boolean uniqueIndexSingleNull;
-
-    /**
-     * When using unique indexes, multiple rows with NULL in all columns
-     * are allowed, however it is not allowed to have multiple rows with the
-     * same values otherwise.
+     * Determines how rows with {@code NULL} values in indexed columns are handled
+     * in unique indexes.
      */
-    public boolean uniqueIndexSingleNullExceptAllColumnsAreNull;
+    public UniqueIndexNullsHandling uniqueIndexNullsHandling = UniqueIndexNullsHandling.ALLOW_DUPLICATES_WITH_ANY_NULL;
 
     /**
      * Empty strings are treated like NULL values. Useful for Oracle emulation.
@@ -208,7 +223,7 @@ public class Mode {
 
         mode = new Mode(ModeEnum.Derby.name());
         mode.aliasColumnName = true;
-        mode.uniqueIndexSingleNull = true;
+        mode.uniqueIndexNullsHandling = UniqueIndexNullsHandling.FORBID_ANY_DUPLICATES;
         mode.supportOffsetFetch = true;
         mode.sysDummy1 = true;
         mode.isolationLevelInSelectOrInsertStatement = true;
@@ -220,7 +235,7 @@ public class Mode {
         mode.aliasColumnName = true;
         mode.convertOnlyToSmallerScale = true;
         mode.nullConcatIsNull = true;
-        mode.uniqueIndexSingleNull = true;
+        mode.uniqueIndexNullsHandling = UniqueIndexNullsHandling.FORBID_ANY_DUPLICATES;
         mode.allowPlusForStringConcat = true;
         // HSQLDB does not support client info properties. See
         // http://hsqldb.org/doc/apidocs/
@@ -232,7 +247,7 @@ public class Mode {
         mode = new Mode(ModeEnum.MSSQLServer.name());
         mode.aliasColumnName = true;
         mode.squareBracketQuotedNames = true;
-        mode.uniqueIndexSingleNull = true;
+        mode.uniqueIndexNullsHandling = UniqueIndexNullsHandling.FORBID_ANY_DUPLICATES;
         mode.allowPlusForStringConcat = true;
         mode.swapConvertFunctionParameters = true;
         mode.supportPoundSymbolForColumnNames = true;
@@ -260,7 +275,7 @@ public class Mode {
         mode = new Mode(ModeEnum.Oracle.name());
         mode.aliasColumnName = true;
         mode.convertOnlyToSmallerScale = true;
-        mode.uniqueIndexSingleNullExceptAllColumnsAreNull = true;
+        mode.uniqueIndexNullsHandling = UniqueIndexNullsHandling.ALLOW_DUPLICATES_WITH_ALL_NULLS;
         mode.treatEmptyStringsAsNull = true;
         mode.regexpReplaceBackslashReferences = true;
         mode.supportPoundSymbolForColumnNames = true;

h2/src/main/org/h2/index/BaseIndex.java

@@ -9,7 +9,6 @@ import java.util.HashSet;
 import org.h2.api.ErrorCode;
 import org.h2.engine.Constants;
 import org.h2.engine.DbObject;
-import org.h2.engine.Mode;
 import org.h2.engine.Session;
 import org.h2.message.DbException;
 import org.h2.message.Trace;
@@ -302,35 +301,35 @@ public abstract class BaseIndex extends SchemaObjectBase implements Index {
     }
 
     /**
-     * Check if one of the columns is NULL and multiple rows with NULL are
-     * allowed using the current compatibility mode for unique indexes. Note:
-     * NULL behavior is complicated in SQL.
+     * Check if this row may have duplicates with the same indexed values in the
+     * current compatibility mode. Duplicates with {@code NULL} values are
+     * allowed in some modes.
      *
-     * @param newRow the row to check
-     * @return true if one of the columns is null and multiple nulls in unique
-     *         indexes are allowed
+     * @param searchRow
+     *            the row to check
+     * @return {@code true} if specified row may have duplicates,
+     *         {@code false otherwise}
      */
-    protected boolean containsNullAndAllowMultipleNull(SearchRow newRow) {
-        Mode mode = database.getMode();
-        if (mode.uniqueIndexSingleNull) {
-            return false;
-        } else if (mode.uniqueIndexSingleNullExceptAllColumnsAreNull) {
+    protected boolean mayHaveNullDuplicates(SearchRow searchRow) {
+        switch (database.getMode().uniqueIndexNullsHandling) {
+        case ALLOW_DUPLICATES_WITH_ANY_NULL:
             for (int index : columnIds) {
-                Value v = newRow.getValue(index);
-                if (v != ValueNull.INSTANCE) {
-                    return false;
-                }
-            }
+                if (searchRow.getValue(index) == ValueNull.INSTANCE) {
                     return true;
                 }
+            }
+            return false;
+        case ALLOW_DUPLICATES_WITH_ALL_NULLS:
             for (int index : columnIds) {
-            Value v = newRow.getValue(index);
-            if (v == ValueNull.INSTANCE) {
-                return true;
+                if (searchRow.getValue(index) != ValueNull.INSTANCE) {
+                    return false;
                 }
             }
+            return true;
+        default:
             return false;
         }
+    }
 
     /**
      * Compare the positions of two rows.

h2/src/main/org/h2/index/PageBtree.java

@@ -116,7 +116,7 @@ public abstract class PageBtree extends Page {
             comp = index.compareRows(row, compare);
             if (comp == 0) {
                 if (add && index.indexType.isUnique()) {
-                    if (!index.containsNullAndAllowMultipleNull(compare)) {
+                    if (!index.mayHaveNullDuplicates(compare)) {
                         throw index.getDuplicateKeyException(compare.toString());
                     }
                 }

h2/src/main/org/h2/index/TreeIndex.java

@@ -66,7 +66,7 @@ public class TreeIndex extends BaseIndex {
             int compare = compareRows(row, r);
             if (compare == 0) {
                 if (indexType.isUnique()) {
-                    if (!containsNullAndAllowMultipleNull(row)) {
+                    if (!mayHaveNullDuplicates(row)) {
                         throw getDuplicateKeyException(row.toString());
                     }
                 }

h2/src/main/org/h2/mvstore/db/MVSecondaryIndex.java

@@ -143,7 +143,9 @@ public final class MVSecondaryIndex extends BaseIndex implements MVIndex {
                     array[keyColumns - 1] = ValueLong.get(Long.MIN_VALUE);
                     ValueArray unique = ValueArray.get(array);
                     SearchRow row = convertToSearchRow(rowData);
-                    checkUnique(row, dataMap, unique);
+                    if (!mayHaveNullDuplicates(row)) {
+                        requireUnique(row, dataMap, unique);
+                    }
                 }
 
                 dataMap.putCommitted(rowData, ValueNull.INSTANCE);
@@ -193,25 +195,26 @@ public final class MVSecondaryIndex extends BaseIndex implements MVIndex {
             // this will detect committed entries only
             unique = convertToKey(row);
             unique.getList()[keyColumns - 1] = ValueLong.get(Long.MIN_VALUE);
-            checkUnique(row, map, unique);
+            if (mayHaveNullDuplicates(row)) {
+                // No further unique checks required
+                unique = null;
+            } else {
+                requireUnique(row, map, unique);
+            }
         }
         try {
             map.put(array, ValueNull.INSTANCE);
         } catch (IllegalStateException e) {
             throw mvTable.convertException(e);
         }
-        if (indexType.isUnique()) {
+        if (unique != null) {
+            // This code expects that mayHaveDuplicates(row) == false
             Iterator<Value> it = map.keyIterator(unique, true);
             while (it.hasNext()) {
                 ValueArray k = (ValueArray) it.next();
-                SearchRow r2 = convertToSearchRow(k);
-                if (compareRows(row, r2) != 0) {
+                if (compareRows(row, convertToSearchRow(k)) != 0) {
                     break;
                 }
-                if (containsNullAndAllowMultipleNull(r2)) {
-                    // this is allowed
-                    continue;
-                }
                 if (map.isSameTransaction(k)) {
                     continue;
                 }
@@ -224,21 +227,16 @@ public final class MVSecondaryIndex extends BaseIndex implements MVIndex {
         }
     }
 
-    private void checkUnique(SearchRow row, TransactionMap<Value, Value> map, ValueArray unique) {
-        Iterator<Value> it = map.keyIterator(unique, true);
-        while (it.hasNext()) {
+    private void requireUnique(SearchRow row, TransactionMap<Value, Value> map, ValueArray unique) {
+        Iterator<Value> it = map.keyIterator(unique);
+        if (it.hasNext()) {
             ValueArray k = (ValueArray) it.next();
-            SearchRow r2 = convertToSearchRow(k);
-            if (compareRows(row, r2) != 0) {
-                break;
-            }
-            if (map.get(k) != null) {
-                if (!containsNullAndAllowMultipleNull(r2)) {
+            if (compareRows(row, convertToSearchRow(k)) == 0) {
+                // committed
                 throw getDuplicateKeyException(k.toString());
             }
         }
     }
-    }
 
     @Override
     public void remove(Session session, Row row) {

h2/src/test/org/h2/test/db/TestIndex.java

@@ -11,7 +11,9 @@ import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Random;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import org.h2.api.ErrorCode;
 import org.h2.result.SortOrder;
@@ -44,6 +46,7 @@ public class TestIndex extends TestBase {
         testHashIndexOnMemoryTable();
         testErrorMessage();
         testDuplicateKeyException();
+        testConcurrentUpdate();
         testNonUniqueHashIndex();
         testRenamePrimaryKey();
         testRandomized();
@@ -187,6 +190,103 @@ public class TestIndex extends TestBase {
         stat.execute("drop table test");
     }
 
+    private class ConcurrentUpdateThread extends Thread {
+        private final AtomicInteger concurrentUpdateId, concurrentUpdateValue;
+
+        private final PreparedStatement psInsert, psDelete;
+
+        boolean haveDuplicateKeyException;
+
+        ConcurrentUpdateThread(Connection c, AtomicInteger concurrentUpdateId,
+                AtomicInteger concurrentUpdateValue) throws SQLException {
+            this.concurrentUpdateId = concurrentUpdateId;
+            this.concurrentUpdateValue = concurrentUpdateValue;
+            psInsert = c.prepareStatement("insert into test(id, value) values (?, ?)");
+            psDelete = c.prepareStatement("delete from test where value = ?");
+        }
+
+        @Override
+        public void run() {
+            for (int i = 0; i < 10000; i++) {
+                try {
+                    if (Math.random() > 0.05) {
+                        psInsert.setInt(1, concurrentUpdateId.incrementAndGet());
+                        psInsert.setInt(2, concurrentUpdateValue.get());
+                        psInsert.executeUpdate();
+                    } else {
+                        psDelete.setInt(1, concurrentUpdateValue.get());
+                        psDelete.executeUpdate();
+                    }
+                } catch (SQLException ex) {
+                    switch (ex.getErrorCode()) {
+                    case 23505:
+                        haveDuplicateKeyException = true;
+                        break;
+                    case 90131:
+                        // Unlikely but possible
+                        break;
+                    default:
+                        ex.printStackTrace();
+                    }
+                }
+                if (Math.random() > 0.95)
+                    concurrentUpdateValue.incrementAndGet();
+            }
+        }
+    }
+
+    private void testConcurrentUpdate() throws SQLException {
+        Connection c = getConnection("index");
+        Statement stat = c.createStatement();
+        stat.execute("create table test(id int primary key, value int)");
+        stat.execute("create unique index idx_value_name on test(value)");
+        PreparedStatement check = c.prepareStatement("select value from test");
+        ConcurrentUpdateThread[] threads = new ConcurrentUpdateThread[4];
+        AtomicInteger concurrentUpdateId = new AtomicInteger(), concurrentUpdateValue = new AtomicInteger();
+
+        // The same connection
+        for (int i = 0; i < threads.length; i++) {
+            threads[i] = new ConcurrentUpdateThread(c, concurrentUpdateId, concurrentUpdateValue);
+        }
+        testConcurrentUpdateRun(threads, check);
+        // Different connections
+        Connection[] connections = new Connection[threads.length];
+        for (int i = 0; i < threads.length; i++) {
+            Connection c2 = getConnection("index");
+            connections[i] = c2;
+            threads[i] = new ConcurrentUpdateThread(c2, concurrentUpdateId, concurrentUpdateValue);
+        }
+        testConcurrentUpdateRun(threads, check);
+        for (Connection c2 : connections) {
+            c2.close();
+        }
+        stat.execute("drop table test");
+        c.close();
+    }
+
+    void testConcurrentUpdateRun(ConcurrentUpdateThread[] threads, PreparedStatement check) throws SQLException {
+        for (ConcurrentUpdateThread t : threads) {
+            t.start();
+        }
+        boolean haveDuplicateKeyException = false;
+        for (ConcurrentUpdateThread t : threads) {
+            try {
+                t.join();
+                haveDuplicateKeyException |= t.haveDuplicateKeyException;
+            } catch (InterruptedException e) {
+            }
+        }
+        assertTrue("haveDuplicateKeys", haveDuplicateKeyException);
+        HashSet<Integer> set = new HashSet<>();
+        try (ResultSet rs = check.executeQuery()) {
+            while (rs.next()) {
+                if (!set.add(rs.getInt(1))) {
+                    fail("unique index violation");
+                }
+            }
+        }
+    }
+
     private void testNonUniqueHashIndex() throws SQLException {
         reconnect();
         stat.execute("create memory table test(id bigint, data bigint)");