addressing code review comments

h2/src/main/org/h2/command/dml/Select.java

@@ -594,7 +594,8 @@ public class Select extends Query {
             }
         }
         ArrayList<Row> forUpdateRows = null;
-        if (isForUpdateMvcc) {
+        boolean lockRows = this.isForUpdateMvcc;
+        if (lockRows) {
             forUpdateRows = Utils.newSmallArrayList();
         }
         int sampleSize = getSampleSizeValue(session);
@@ -604,7 +605,7 @@ public class Select extends Query {
             return lazyResult;
         }
         while (lazyResult.next()) {
-            if (isForUpdateMvcc) {
+            if (lockRows) {
                 topTableFilter.lockRowAdd(forUpdateRows);
             }
             result.addRow(lazyResult.currentRow());
@@ -613,8 +614,7 @@ public class Select extends Query {
                 break;
             }
         }
-        if (isForUpdateMvcc) {
-            assert forUpdateRows != null;
+        if (lockRows) {
             topTableFilter.lockRows(forUpdateRows);
         }
         return null;

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

@@ -150,7 +150,7 @@ public class Cursor<K, V> implements Iterator<K> {
      * @param p the page to start from
      * @param key the key to search, null means search for the first key
      */
-    public static CursorPos traverseDown(Page p, Object key) {
+    private static CursorPos traverseDown(Page p, Object key) {
         CursorPos cursorPos = null;
         while (!p.isLeaf()) {
             assert p.getKeyCount() > 0;

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

@@ -131,6 +131,7 @@ public class MVMap<K, V> extends AbstractMap<K, V>
      */
     @Override
     public V put(K key, V value) {
+        DataUtils.checkArgument(value != null, "The value may not be null");
         return put(key, value, DecisionMaker.PUT);
     }
 
@@ -142,7 +143,6 @@ public class MVMap<K, V> extends AbstractMap<K, V>
      * @return the old value if the key existed, or null otherwise
      */
     public final V put(K key, V value, DecisionMaker<? super V> decisionMaker) {
-        DataUtils.checkArgument(value != null, "The value may not be null");
         return operate(key, value, decisionMaker);
     }
 

h2/src/main/org/h2/mvstore/tx/CommitDecisionMaker.java

@@ -18,7 +18,7 @@ final class CommitDecisionMaker extends MVMap.DecisionMaker<VersionedValue> {
     private long undoKey;
     private MVMap.Decision decision;
 
-    public void setUndoKey(long undoKey) {
+    void setUndoKey(long undoKey) {
         this.undoKey = undoKey;
         reset();
     }
@@ -26,26 +26,21 @@ final class CommitDecisionMaker extends MVMap.DecisionMaker<VersionedValue> {
     @Override
     public MVMap.Decision decide(VersionedValue existingValue, VersionedValue providedValue) {
         assert decision == null;
-        if (existingValue == null) {
-            // map entry was treated as committed already and
-            // removed already by another commited by now transaction
+        if (existingValue == null ||
+            // map entry was treated as already committed, and then
+            // it has been removed by another transaction (commited and closed  by now )
+            existingValue.getOperationId() != undoKey) {
+            // this is not a final undo log entry for this key,
+            // or map entry was treated as already committed and then
+            // overwritten by another transaction
+            // see TxDecisionMaker.decide()
+
             decision = MVMap.Decision.ABORT;
-        } else {
-            if (existingValue.getOperationId() == undoKey) {
-                // this is final undo log entry for this key
-                if (existingValue.value == null) {
+        } else /* this is final undo log entry for this key */ if (existingValue.value == null) {
             decision = MVMap.Decision.REMOVE;
         } else {
             decision = MVMap.Decision.PUT;
         }
-            } else {
-                // this is not a final undo log entry for this key,
-                // or map entry was treated as committed already and
-                // overwritten already by another transaction
-                // see TxDecisionMaker.decide()
-                decision = MVMap.Decision.ABORT;
-            }
-        }
         return decision;
     }
 

h2/src/main/org/h2/mvstore/tx/RollbackDecisionMaker.java

@@ -31,14 +31,6 @@ final class RollbackDecisionMaker extends MVMap.DecisionMaker<Object[]> {
     public MVMap.Decision decide(Object[] existingValue, Object[] providedValue) {
         assert decision == null;
         assert existingValue != null;
-/*
-        if (existingValue == null) {
-            // this should only be possible during initialization
-            // when previously database was abruptly killed
-//                assert !store.isInitialized();
-            decision = MVMap.Decision.ABORT;
-        } else {
-*/
         VersionedValue valueToRestore = (VersionedValue) existingValue[2];
         long operationId;
         if (valueToRestore == null ||
@@ -54,7 +46,6 @@ final class RollbackDecisionMaker extends MVMap.DecisionMaker<Object[]> {
             }
         }
         decision = MVMap.Decision.REMOVE;
-//        }
         return decision;
     }
 

h2/src/main/org/h2/mvstore/tx/TransactionMap.java

@@ -99,7 +99,6 @@ public class TransactionMap<K, V> {
      */
     public long sizeAsLong() {
         TransactionStore store = transaction.store;
-        MVMap<Long, Object[]> undo = transaction.store.undoLog;
 
         BitSet committingTransactions;
         MVMap.RootReference mapRootReference;
@@ -135,14 +134,13 @@ public class TransactionMap<K, V> {
         }
         // the undo log is smaller than the map -
         // scan the undo log and subtract invisible entries
-        MVMap<Object, Integer> temp = transaction.store
-                .createTempMap();
+        MVMap<Object, Integer> temp = store.createTempMap();
         try {
-            Cursor cursor = new Cursor<Long, Object[]>(undoRootPage, null);
+            Cursor<Long, Object[]> cursor = new Cursor<>(undoRootPage, null);
             while (cursor.hasNext()) {
                 cursor.next();
-                Object[] op = (Object[]) cursor.getValue();
-                int m = (Integer) op[0];
+                Object[] op = cursor.getValue();
+                int m = (int) op[0];
                 if (m != mapId) {
                     // a different map - ignore
                     continue;
@@ -233,12 +231,13 @@ public class TransactionMap<K, V> {
      * @param value the value
      * @return the old value
      */
-    @SuppressWarnings("unchecked")
     public V putCommitted(K key, V value) {
         DataUtils.checkArgument(value != null, "The value may not be null");
         VersionedValue newValue = new VersionedValue(0L, value);
         VersionedValue oldValue = map.put(key, newValue);
-        return (V) (oldValue == null ? null : oldValue.value);
+        @SuppressWarnings("unchecked")
+        V result = (V) (oldValue == null ? null : oldValue.value);
+        return result;
     }
 
     private V set(K key, V value) {
@@ -246,7 +245,6 @@ public class TransactionMap<K, V> {
         return set(key, decisionMaker);
     }
 
-    @SuppressWarnings("unchecked")
     private V set(K key, TxDecisionMaker decisionMaker) {
         TransactionStore store = transaction.store;
         Transaction blockingTransaction;
@@ -255,7 +253,10 @@ public class TransactionMap<K, V> {
         do {
             sequenceNumWhenStarted = store.openTransactions.get().getVersion();
             assert transaction.getBlockerId() == 0;
-
+            // although second parameter (value) is not really used,
+            // since TxDecisionMaker has it embedded,
+            // MVRTreeMap has weird traversal logic based on it,
+            // and any non-null value will do
             result = map.put(key, VersionedValue.DUMMY, decisionMaker);
 
             MVMap.Decision decision = decisionMaker.getDecision();
@@ -265,7 +266,9 @@ public class TransactionMap<K, V> {
             if (decision != MVMap.Decision.ABORT || blockingTransaction == null) {
                 transaction.blockingMap = null;
                 transaction.blockingKey = null;
-                return result == null ? null : (V) result.value;
+                @SuppressWarnings("unchecked")
+                V res = result == null ? null : (V) result.value;
+                return res;
             }
             decisionMaker.reset();
             transaction.blockingMap = map;

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

@@ -39,7 +39,7 @@ public class TransactionStore {
      * The persisted map of prepared transactions.
      * Key: transactionId, value: [ status, name ].
      */
-    final MVMap<Integer, Object[]> preparedTransactions;
+    private final MVMap<Integer, Object[]> preparedTransactions;
 
     /**
      * The undo log.
@@ -228,7 +228,7 @@ public class TransactionStore {
      * @param logId the log id
      * @return the operation id
      */
-    static long getOperationId(int transactionId, long logId) {
+    private static long getOperationId(int transactionId, long logId) {
         DataUtils.checkArgument(transactionId >= 0 && transactionId < (1 << (64 - LOG_ID_BITS)),
                 "Transaction id out of range: {0}", transactionId);
         DataUtils.checkArgument(logId >= 0 && logId <= LOG_ID_MASK,
@@ -362,11 +362,11 @@ public class TransactionStore {
     }
 
     /**
-     * Add an undoLog entry.
+     * Add an undo log entry.
      *
      * @param transactionId id of the transaction
      * @param logId sequential number of the log record within transaction
-     * @param undoLogRecord Object[]
+     * @param undoLogRecord Object[mapId, key, previousValue]
      */
     long addUndoLogRecord(int transactionId, long logId, Object[] undoLogRecord) {
         Long undoKey = getOperationId(transactionId, logId);
@@ -550,9 +550,9 @@ public class TransactionStore {
      * and amount of unsaved changes is sizable.
      *
      * @param t the transaction
-     * @param hasChanges true if transaction has done any updated
-     *                  (even if fully rolled back),
-     *                   false if just data access
+     * @param hasChanges true if transaction has done any updates
+     *                  (even if they are fully rolled back),
+     *                   false if it just performed a data access
      */
     synchronized void endTransaction(Transaction t, boolean hasChanges) {
         t.closeIt();
@@ -646,7 +646,7 @@ public class TransactionStore {
                         logId = getLogId(undoKey);
                         continue;
                     }
-                    int mapId = ((Integer) op[0]).intValue();
+                    int mapId = (int)op[0];
                     MVMap<Object, VersionedValue> m = openMap(mapId);
                     if (m != null) { // could be null if map was removed later on
                         VersionedValue oldValue = (VersionedValue) op[2];

h2/src/main/org/h2/mvstore/tx/TxDecisionMaker.java

@@ -32,7 +32,6 @@ public abstract class TxDecisionMaker extends MVMap.DecisionMaker<VersionedValue
     @Override
     public MVMap.Decision decide(VersionedValue existingValue, VersionedValue providedValue) {
         assert decision == null;
-        assert providedValue != null;
         long id;
         int blockingId;
         // if map does not have that entry yet
@@ -52,7 +51,7 @@ public abstract class TxDecisionMaker extends MVMap.DecisionMaker<VersionedValue
             decision = MVMap.Decision.PUT;
         } else if(fetchTransaction(blockingId) == null) {
             // condition above means transaction has been committed/rplled back and closed by now
-            return setDecision(MVMap.Decision.REPEAT);
+            decision = MVMap.Decision.REPEAT;
         } else {
             // this entry comes from a different transaction, and this transaction is not committed yet
             // should wait on blockingTransaction that was determined earlier
@@ -129,7 +128,6 @@ public abstract class TxDecisionMaker extends MVMap.DecisionMaker<VersionedValue
         @Override
         public MVMap.Decision decide(VersionedValue existingValue, VersionedValue providedValue) {
             assert getDecision() == null;
-            assert providedValue != null;
             int blockingId;
             // if map does not have that entry yet
             if (existingValue == null) {

h2/src/main/org/h2/table/TableFilter.java

@@ -7,7 +7,6 @@ package org.h2.table;
 
 import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.Iterator;
 
 import org.h2.api.ErrorCode;
 import org.h2.command.Parser;