MVStore: concurrency problems have been fixed.

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

@@ -194,7 +194,8 @@ public class SpatialTreeIndex extends BaseIndex implements SpatialIndex {
     @Override
     public void remove(Session session) {
         if (!treeMap.isClosed()) {
-            treeMap.removeMap();
+            MVStore store = session.getDatabase().getMvStore().getStore();
+            store.removeMap(treeMap);
         }
     }
 

h2/src/main/org/h2/mvstore/MVMap.java

@@ -505,24 +505,6 @@ public class MVMap<K, V> extends AbstractMap<K, V>
         }
     }
 
-    /**
-     * Remove all entries, and close the map.
-     */
-    public void removeMap() {
-        int todoMoveToMVStore;
-        if (this == store.getMetaMap()) {
-            return;
-        }
-        beforeWrite();
-        try {
-            root.removeAllRecursive();
-            store.removeMap(id);
-            close();
-        } finally {
-            afterWrite();
-        }
-    }
-
     /**
      * Close the map. Accessing the data is still possible (to allow concurrent
      * reads), but it is marked as closed.
@@ -1115,21 +1097,6 @@ public class MVMap<K, V> extends AbstractMap<K, V>
         return buff.toString();
     }
 
-    /**
-     * Rename the map.
-     *
-     * @param newMapName the name name
-     */
-    public void renameMap(String newMapName) {
-        int todoMoveToMVStore;
-        beforeWrite();
-        try {
-            store.renameMap(this, newMapName);
-        } finally {
-            afterWrite();
-        }
-    }
-
     void setWriteVersion(long writeVersion) {
         this.writeVersion = writeVersion;
     }

h2/src/main/org/h2/mvstore/MVStore.java

@@ -15,6 +15,7 @@ import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
@@ -484,20 +485,6 @@ public class MVStore {
         }
     }
 
-    /**
-     * Remove a map.
-     *
-     * @param id the map id
-     */
-    void removeMap(int id) {
-        String name = getMapName(id);
-        markMetaChanged();
-        meta.remove("map." + id);
-        meta.remove("name." + name);
-        meta.remove("root." + id);
-        maps.remove(id);
-    }
-
     /**
      * Check whether a given map exists.
      *
@@ -1058,19 +1045,23 @@ public class MVStore {
         Set<Chunk> removedChunks = New.hashSet();
         while (true) {
             ArrayList<Chunk> modified = New.arrayList();
-            ArrayList<Long> keys = new ArrayList<Long>(freedPageSpace.keySet());
-            for (Iterator<Long> it = keys.iterator(); it.hasNext();) {
-                long v = it.next();
+            Iterator<Entry<Long, HashMap<Integer, Chunk>>> it;
+            it = freedPageSpace.entrySet().iterator();
+            while (it.hasNext()) {
+                Entry<Long, HashMap<Integer, Chunk>> e = it.next();
+                long v = e.getKey();
                 if (v > storeVersion) {
                     continue;
                 }
-                Map<Integer, Chunk> freed = freedPageSpace.get(v);
+                Map<Integer, Chunk> freed = e.getValue();
                 for (Chunk f : freed.values()) {
                     Chunk c = chunks.get(f.id);
                     if (c == null) {
                         // already removed
                         continue;
                     }
+                    // no need to synchronize, as old entries
+                    // are not concurrently modified
                     c.maxLengthLive += f.maxLengthLive;
                     c.pageCountLive += f.pageCountLive;
                     if (c.pageCountLive < 0) {
@@ -1090,7 +1081,7 @@ public class MVStore {
                     }
                     modified.add(c);
                 }
-                freedPageSpace.remove(v);
+                it.remove();
             }
             for (Chunk c : modified) {
                 if (c.maxLengthLive == 0) {
@@ -1511,6 +1502,7 @@ public class MVStore {
                 freed = f2;
             }
         }
+        // synchronize, because pages could be freed concurrently
         synchronized (freed) {
             Chunk f = freed.get(chunkId);
             if (f == null) {
@@ -1759,14 +1751,12 @@ public class MVStore {
         for (MVMap<?, ?> m : maps.values()) {
             m.rollbackTo(version);
         }
-        synchronized (freedPageSpace) {
         for (long v = currentVersion; v >= version; v--) {
             if (freedPageSpace.size() == 0) {
                 break;
             }
             freedPageSpace.remove(v);
         }
-        }
         meta.rollbackTo(version);
         metaChanged = false;
         boolean loadFromFile = false;
@@ -1899,7 +1889,7 @@ public class MVStore {
      * @param map the map
      * @param newName the new name
      */
-    void renameMap(MVMap<?, ?> map, String newName) {
+    public void renameMap(MVMap<?, ?> map, String newName) {
         checkOpen();
         DataUtils.checkArgument(map != meta,
                 "Renaming the meta map is not allowed");
@@ -1917,6 +1907,25 @@ public class MVStore {
         meta.put("name." + newName, Integer.toString(id));
     }
 
+    /**
+     * Remove a map.
+     *
+     * @param map the map
+     */
+    public void removeMap(MVMap<?, ?> map) {
+        checkOpen();
+        DataUtils.checkArgument(map != meta,
+                "Removing the meta map is not allowed");
+        map.clear();
+        int id = map.getId();
+        String name = getMapName(id);
+        markMetaChanged();
+        meta.remove("map." + id);
+        meta.remove("name." + name);
+        meta.remove("root." + id);
+        maps.remove(id);
+    }
+
     /**
      * Get the name of the given map.
      *

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

@@ -9,6 +9,7 @@ package org.h2.mvstore.db;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Iterator;
+
 import org.h2.constant.ErrorCode;
 import org.h2.engine.Constants;
 import org.h2.engine.Database;
@@ -17,7 +18,6 @@ import org.h2.index.BaseIndex;
 import org.h2.index.Cursor;
 import org.h2.index.IndexType;
 import org.h2.message.DbException;
-import org.h2.mvstore.MVMap;
 import org.h2.mvstore.db.TransactionStore.Transaction;
 import org.h2.mvstore.db.TransactionStore.TransactionMap;
 import org.h2.result.Row;
@@ -55,10 +55,8 @@ public class MVPrimaryIndex extends BaseIndex {
         ValueDataType valueType = new ValueDataType(
                 db.getCompareMode(), db, sortTypes);
         mapName = "table." + getId();
-        MVMap.Builder<Value, Value> mapBuilder = new MVMap.Builder<Value, Value>().
-                keyType(keyType).
-                valueType(valueType);
-        dataMap = mvTable.getTransaction(null).openMap(mapName, mapBuilder);
+        dataMap = mvTable.getTransaction(null).openMap(
+                mapName, keyType, valueType);
         Value k = dataMap.lastKey();
         lastKey = k == null ? 0 : k.getLong();
     }
@@ -216,7 +214,8 @@ public class MVPrimaryIndex extends BaseIndex {
     public void remove(Session session) {
         TransactionMap<Value, Value> map = getMap(session);
         if (!map.isClosed()) {
-            map.removeMap();
+            Transaction t = mvTable.getTransaction(session);
+            t.removeMap(map);
         }
     }
 

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

@@ -9,6 +9,7 @@ package org.h2.mvstore.db;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Iterator;
+
 import org.h2.constant.ErrorCode;
 import org.h2.engine.Database;
 import org.h2.engine.Session;
@@ -16,7 +17,6 @@ import org.h2.index.BaseIndex;
 import org.h2.index.Cursor;
 import org.h2.index.IndexType;
 import org.h2.message.DbException;
-import org.h2.mvstore.MVMap;
 import org.h2.mvstore.db.TransactionStore.Transaction;
 import org.h2.mvstore.db.TransactionStore.TransactionMap;
 import org.h2.result.Row;
@@ -64,10 +64,8 @@ public class MVSecondaryIndex extends BaseIndex {
         ValueDataType keyType = new ValueDataType(
                 db.getCompareMode(), db, sortTypes);
         ValueDataType valueType = new ValueDataType(null, null, null);
-        MVMap.Builder<Value, Value> mapBuilder = new MVMap.Builder<Value, Value>().
-                keyType(keyType).
-                valueType(valueType);
-        dataMap = mvTable.getTransaction(null).openMap(mapName, mapBuilder);
+        dataMap = mvTable.getTransaction(null).openMap(
+                mapName, keyType, valueType);
         if (keyType != dataMap.map.getKeyType()) {
             throw DbException.throwInternalError("Incompatible key type");
         }
@@ -204,7 +202,8 @@ public class MVSecondaryIndex extends BaseIndex {
     public void remove(Session session) {
         TransactionMap<Value, Value> map = getMap(session);
         if (!map.isClosed()) {
-            map.removeMap();
+            Transaction t = mvTable.getTransaction(session);
+            t.removeMap(map);
         }
     }
 

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

@@ -11,6 +11,7 @@ import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.Set;
+
 import org.h2.api.DatabaseEventListener;
 import org.h2.command.ddl.Analyze;
 import org.h2.command.ddl.CreateTableData;
@@ -398,6 +399,7 @@ public class MVTable extends TableBase {
             index = new MVDelegateIndex(this, indexId,
                     indexName, primaryIndex, indexType);
         } else if (indexType.isSpatial()) {
+            int todo;
             index = new SpatialTreeIndex(this, indexId, indexName, cols,
                     indexType, true, create, session);
         } else {

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

@@ -15,7 +15,6 @@ import java.util.Map;
 import org.h2.mvstore.Cursor;
 import org.h2.mvstore.DataUtils;
 import org.h2.mvstore.MVMap;
-import org.h2.mvstore.MVMap.Builder;
 import org.h2.mvstore.MVStore;
 import org.h2.mvstore.WriteBuffer;
 import org.h2.mvstore.type.DataType;
@@ -630,9 +629,7 @@ public class TransactionStore {
          * @return the transaction map
          */
         public <K, V> TransactionMap<K, V> openMap(String name) {
-            checkNotClosed();
-            return new TransactionMap<K, V>(this, name, new ObjectDataType(),
-                    new ObjectDataType());
+            return openMap(name, null, null);
         }
 
         /**
@@ -641,20 +638,31 @@ public class TransactionStore {
          * @param <K> the key type
          * @param <V> the value type
          * @param name the name of the map
-         * @param builder the builder
+         * @param keyType the key data type
+         * @param valueType the value data type
          * @return the transaction map
          */
-        public <K, V> TransactionMap<K, V> openMap(String name, Builder<K, V> builder) {
+        public <K, V> TransactionMap<K, V> openMap(String name, DataType keyType, DataType valueType) {
             checkNotClosed();
-            DataType keyType = builder.getKeyType();
             if (keyType == null) {
                 keyType = new ObjectDataType();
             }
-            DataType valueType = builder.getValueType();
             if (valueType == null) {
                 valueType = new ObjectDataType();
             }
-            return new TransactionMap<K, V>(this, name, keyType, valueType);
+            VersionedValueType vt = new VersionedValueType(valueType);
+            MVMap.Builder<K, VersionedValue> builder = new MVMap.Builder<K, VersionedValue>()
+                    .keyType(keyType).valueType(vt);
+            MVMap<K, VersionedValue> map = store.store.openMap(name, builder);
+            int mapId = map.getId();
+            return new TransactionMap<K, V>(this, map, mapId);
+        }
+        
+        public <K, V> TransactionMap<K, V> openMap(String name, MVMap.Builder<K, VersionedValue> builder) {
+            checkNotClosed();
+            MVMap<K, VersionedValue> map = store.store.openMap(name, builder);
+            int mapId = map.getId();
+            return new TransactionMap<K, V>(this, map, mapId);
         }
 
         /**
@@ -719,6 +727,13 @@ public class TransactionStore {
             }
         }
         
+        /**
+         * Remove the map.
+         */
+        public <K, V> void removeMap(TransactionMap<K, V> map) {
+            store.store.removeMap(map.map);
+        }
+
     }
 
     /**
@@ -749,17 +764,7 @@ public class TransactionStore {
          */
         private long readLogId = Long.MAX_VALUE;
 
-        TransactionMap(Transaction transaction, String name, DataType keyType,
-                DataType valueType) {
-            this.transaction = transaction;
-            VersionedValueType vt = new VersionedValueType(valueType);
-            MVMap.Builder<K, VersionedValue> builder = new MVMap.Builder<K, VersionedValue>()
-                    .keyType(keyType).valueType(vt);
-            map = transaction.store.store.openMap(name, builder);
-            mapId = map.getId();
-        }
-
-        private TransactionMap(Transaction transaction, MVMap<K, VersionedValue> map, int mapId) {
+        TransactionMap(Transaction transaction, MVMap<K, VersionedValue> map, int mapId) {
             this.transaction = transaction;
             this.map = map;
             this.mapId = mapId;
@@ -1080,17 +1085,6 @@ public class TransactionStore {
             }
         }
 
-
-        /**
-         * Rename the map.
-         *
-         * @param newMapName the new map name
-         */
-        public void renameMap(String newMapName) {
-            // TODO rename maps transactionally
-            map.renameMap(newMapName);
-        }
-
         /**
          * Check whether this map is closed.
          *
@@ -1100,14 +1094,6 @@ public class TransactionStore {
             return map.isClosed();
         }
 
-        /**
-         * Remove the map.
-         */
-        public void removeMap() {
-            // TODO remove in a transaction
-            map.removeMap();
-        }
-
         /**
          * Clear the map.
          */
@@ -1204,25 +1190,36 @@ public class TransactionStore {
         }
 
         /**
-         * Iterate over all keys.
+         * Iterate over keys.
          *
          * @param from the first key to return
          * @return the iterator
          */
-        public Iterator<K> keyIterator(final K from) {
+        public Iterator<K> keyIterator(K from) {
             return keyIterator(from, false);
         }
 
         /**
-         * Iterate over all keys.
+         * Iterate over keys.
          *
          * @param from the first key to return
          * @param includeUncommitted whether uncommitted entries should be included
          * @return the iterator
          */
-        public Iterator<K> keyIterator(final K from, final boolean includeUncommitted) {
+        public Iterator<K> keyIterator(K from, boolean includeUncommitted) {
+            Cursor<K> it = map.keyIterator(from);
+            return wrapIterator(it, false);
+        }
+
+        /**
+         * Iterate over keys.
+         *
+         * @param cursor the wrapped cursor
+         * @param includeUncommitted whether uncommitted entries should be included
+         * @return the iterator
+         */
+        private Iterator<K> wrapIterator(final Cursor<K> cursor, final boolean includeUncommitted) {
             return new Iterator<K>() {
-                private final Cursor<K> cursor = map.keyIterator(from);
                 private K current;
 
                 {

h2/src/main/org/h2/mvstore/rtree/MVRTreeMap.java

@@ -15,7 +15,6 @@ import org.h2.mvstore.MVMap;
 import org.h2.mvstore.Page;
 import org.h2.mvstore.type.DataType;
 import org.h2.mvstore.type.ObjectDataType;
-import org.h2.mvstore.type.StringDataType;
 import org.h2.util.New;
 
 /**
@@ -744,7 +743,7 @@ public class MVRTreeMap<V> extends MVMap<SpatialKey, V> {
          * @param valueType the key type
          * @return this
          */
-        public Builder<V> valueType(StringDataType valueType) {
+        public Builder<V> valueType(DataType valueType) {
             this.valueType = valueType;
             return this;
         }

h2/src/test/org/h2/test/store/TestConcurrent.java

@@ -95,7 +95,7 @@ public class TestConcurrent extends TestMVStore {
                         }
                         MVMap<Integer, Integer> m = list.get(x);
                         m.clear();
-                        m.removeMap();
+                        s.removeMap(m);
                     }
                 }
             };
@@ -108,7 +108,7 @@ public class TestConcurrent extends TestMVStore {
                 }
                 MVMap<Integer, Integer> m = list.get(x);
                 m.clear();
-                m.removeMap();
+                s.removeMap(m);
                 if (x % 5 == 0) {
                     s.incrementVersion();
                 }
@@ -152,7 +152,7 @@ public class TestConcurrent extends TestMVStore {
         for (int i = 0; i < count || counter.get() < count; i++) {
             MVMap<Integer, Integer> m = s.openMap("d" + i);
             m.put(1, 10);
-            m.removeMap();
+            s.removeMap(m);
             if (task.isFinished()) {
                 break;
             }

h2/src/test/org/h2/test/store/TestMVStore.java

@@ -110,7 +110,7 @@ public class TestMVStore extends TestBase {
         assertEquals(1, map.get(1).intValue());
         s.store();
 
-        map.removeMap();
+        s.removeMap(map);
         s.store();
 
         map = s.openMap("data");
@@ -222,7 +222,7 @@ public class TestMVStore extends TestBase {
         }
         for (int i = 0; i < 10; i += 2) {
             m = s.openMap("data" + i);
-            m.removeMap();
+            s.removeMap(m);
             s.store();
         }
         long sizeOld = s.getFileStore().size();
@@ -503,7 +503,7 @@ public class TestMVStore extends TestBase {
         MVMap<Integer, Integer> m = s.openMap("test");
         m.put(1, 1);
         s.store();
-        m.removeMap();
+        s.removeMap(m);
         s.store();
         s.close();
         s = openStore(fileName);
@@ -518,7 +518,7 @@ public class TestMVStore extends TestBase {
         map = s.openMap("hello");
         map.put(1, 10);
         long old = s.incrementVersion();
-        map.renameMap("world");
+        s.renameMap(map, "world");
         map.put(2, 20);
         assertEquals("world", map.getName());
         s.rollbackTo(old);
@@ -635,7 +635,7 @@ public class TestMVStore extends TestBase {
         long size = fs.getFile().size();
         for (int i = 0; i < 10; i++) {
             map = s.openMap("test" + i);
-            map.removeMap();
+            s.removeMap(map);
             s.store();
             s.commit();
             s.compact(100);