Merge lp:~pbeaman/akiban-persistit/fix_1010079_prune_long_mvv_corruption into lp:akiban-persistit

[Peter Beaman]

Branch fixes two issues related to pruning of long-record MVVs:

1. When refreshing state of the original buffer after copy has been pruned, the FastIndex for the original needs to be invalidated if any keys were removed.

2. Buffer#pruneMvvValues must not use the thread-local Value object if called from Exchange#storeInternal. (We never saw a corruption due to this, but know it would break.)

Other changes include a simple test, a new stress test script and a minor change in JournalManager so to get rid of a stale value of BaseAddress. This is needed to get through the mixture_txn_2.plan.

(Note: in another branch I will propose a better way of restarting the same Persistit instance in tests.)

[Nathan Williams (nwilliams)]

The new flag to prevent the double usage of thread local is fine (though if I were being picky I'd like a new method instead of a boolean) but it is also indirect. I can't think of a better way at the moment, but it might be less error prone to pass the thread locals down from some top level place (e.g. Exchange) instead of using them wherever, as we have no other way to protect ourselves.

There isn't a change in this diff in JournalManager. Need to push or is that not relevant anymore?

Are my eyes playing tricks on me or is the new .plan file the same block repeated 12 time? Another thing is that this uses, as many of the stress tests do, 8k pages. Probably a change for another time though.

review: Needs Information

[Peter Beaman (pbeaman)]

During the drive home I realized there isn't much reason to commit mixture_txn_2.plan. It was just a hack to repeat the test lots of times while keeping the debugger attached. So I just removed it.

That eliminates the need for the change in JournalManager - best not to change it right now.

I'm hoping that in the Exchange refactoring we'll be able to reduce the number of Value objects required and get rid of the ThreadLocal altogether. I appreciate your comments but would like to defer action until we do the new design.

[Nathan Williams (nwilliams)]

Works for now.

review: Approve

[Akiban Build User (build-akiban)]

There was one failure during build/test:

* unknown exception (check log)

review: Needs Fixing

[Nathan Williams (nwilliams)]

Passed persistit-build fine, but timed out waiting for AMI to run server build on. Will restart.

=== modified file 'src/main/java/com/persistit/Buffer.java'
--- src/main/java/com/persistit/Buffer.java	2012-06-04 19:12:21 +0000
+++ src/main/java/com/persistit/Buffer.java	2012-06-08 00:57:19 +0000
@@ -3073,8 +3073,7 @@
 
             int size = (decodeTailBlockSize(tbData) + ~TAILBLOCK_MASK) & TAILBLOCK_MASK;
             if (size <= 0) {
-                _persistit.fatal("Buffer has invalid tailblock length " + size + " at " + tail + " in "
-                        + this, null);
+                _persistit.fatal("Buffer has invalid tailblock length " + size + " at " + tail + " in " + this, null);
             }
 
             if ((tbData & TAILBLOCK_INUSE_MASK) != 0) {
@@ -3571,11 +3570,10 @@
      * @return
      * @throws PersistitException
      */
-    boolean pruneMvvValues(final Tree tree) throws PersistitException {
+    boolean pruneMvvValues(final Tree tree, boolean pruneLongMVVs) throws PersistitException {
 
         boolean changed = false;
         try {
-            boolean bumped = false;
             boolean hasLongMvvRecords = false;
 
             if (!isMine()) {
@@ -3630,15 +3628,9 @@
                             incCountIfMvv(_bytes, offset, newSize);
                         }
 
-                        boolean prunedAntiValue = pruneAntiValue(valueByte, p, tree);
-                        changed |= prunedAntiValue;
-                        if (prunedAntiValue) {
+                        if (pruneAntiValue(valueByte, p, tree)) {
                             changed = true;
                             p -= KEYBLOCK_LENGTH;
-                            if (!bumped) {
-                                bumpGeneration();
-                                bumped = true;
-                            }
                         }
                     }
                 }
@@ -3649,7 +3641,7 @@
                 deallocatePrunedVersions(_persistit, _vol, prunedVersions);
                 prunedVersions.clear();
 
-                if (hasLongMvvRecords) {
+                if (pruneLongMVVs && hasLongMvvRecords) {
                     List<PersistitException> deferredExceptions = new ArrayList<PersistitException>();
                     List<Long> oldChainsToDeallocate = new ArrayList<Long>();
 
@@ -3664,9 +3656,9 @@
                         _alloc = copy._alloc;
                         _slack = copy._slack;
                         _mvvCount = copy._mvvCount;
-                        _keyBlockEnd = copy._keyBlockEnd;
-                        if (copy.getGeneration() > getGeneration()) {
-                            bumpGeneration();
+                        if (_keyBlockEnd != copy._keyBlockEnd) {
+                            _keyBlockEnd = copy._keyBlockEnd;
+                            invalidateFastIndex();
                         }
                         setDirtyAtTimestamp(copyTimestamp);
                         deallocatePrunedVersions(_persistit, _vol, prunedVersions);
@@ -3700,7 +3692,6 @@
             final List<PersistitException> deferredExceptions, final List<Long> toDeallocate) {
 
         boolean changed = false;
-        boolean bumped = false;
         for (int p = KEY_BLOCK_START; p < _keyBlockEnd; p += KEYBLOCK_LENGTH) {
             final int kbData = getInt(p);
             final int tail = decodeKeyBlockTail(kbData);
@@ -3743,10 +3734,6 @@
                 if (pruneAntiValue(valueByte, p, tree)) {
                     changed = true;
                     p -= KEYBLOCK_LENGTH;
-                    if (!bumped) {
-                        bumpGeneration();
-                        bumped = true;
-                    }
                 }
             }
         }

=== modified file 'src/main/java/com/persistit/Exchange.java'
--- src/main/java/com/persistit/Exchange.java	2012-05-30 04:09:46 +0000
+++ src/main/java/com/persistit/Exchange.java	2012-06-08 00:57:19 +0000
@@ -1548,7 +1548,7 @@
                     if (splitRequired && !treeClaimAcquired) {
                         if (!didPrune && buffer.isDataPage()) {
                             didPrune = true;
-                            if (buffer.pruneMvvValues(_tree)) {
+                            if (buffer.pruneMvvValues(_tree, false)) {
                                 continue;
                             }
                         }
@@ -3616,7 +3616,7 @@
             search(key, true);
             buffer = _levelCache[0]._buffer;
             if (buffer != null) {
-                return buffer.pruneMvvValues(_tree);
+                return buffer.pruneMvvValues(_tree, true);
             } else {
                 return false;
             }
@@ -3638,7 +3638,7 @@
 
             while (buffer != null) {
                 checkPageType(buffer, Buffer.PAGE_TYPE_DATA, false);
-                pruned |= buffer.pruneMvvValues(_tree);
+                pruned |= buffer.pruneMvvValues(_tree, true);
                 final int foundAt = buffer.findKey(key2);
                 if (!buffer.isAfterRightEdge(foundAt)) {
                     break;
@@ -3664,7 +3664,7 @@
         Buffer buffer = null;
         try {
             buffer = _pool.get(_volume, page, true, true);
-            return buffer.pruneMvvValues(_tree);
+            return buffer.pruneMvvValues(_tree, true);
         } finally {
             if (buffer != null) {
                 buffer.release();

=== modified file 'src/main/java/com/persistit/IntegrityCheck.java'
--- src/main/java/com/persistit/IntegrityCheck.java	2012-05-25 18:50:59 +0000
+++ src/main/java/com/persistit/IntegrityCheck.java	2012-06-08 00:57:19 +0000
@@ -1134,7 +1134,7 @@
                 _counters._mvvPageCount++;
                 if (_prune && !_currentVolume.isReadOnly() && _counters._pruningErrorCount < MAX_PRUNING_ERRORS) {
                     try {
-                        buffer.pruneMvvValues(tree);
+                        buffer.pruneMvvValues(tree, true);
                         _counters._prunedPageCount++;
                     } catch (PersistitException e) {
                         _counters._pruningErrorCount++;

=== added file 'src/test/java/com/persistit/Bug1010079Test.java'
--- src/test/java/com/persistit/Bug1010079Test.java	1970-01-01 00:00:00 +0000
+++ src/test/java/com/persistit/Bug1010079Test.java	2012-06-08 00:57:19 +0000
@@ -0,0 +1,108 @@
+/**
+ * Copyright © 2012 Akiban Technologies, Inc.  All rights reserved.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, version 3 (only) of the
+ * License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ * 
+ * This program may also be available under different license terms. For more
+ * information, see www.akiban.com or contact [e-mail].
+ */
+
+package com.persistit;
+
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+/**
+ * https://bugs.launchpad.net/akiban-persistit/+bug/1010079
+ * 
+ * Various symptoms found while running stress test mixture_txn_1, including
+ * CorruptVolumeException, CorruptValueException, and several different asserts.
+ * These all stem from new code introduced in
+ * lp:~pbeaman/akiban-persistit/fix_1006576_long_record_pruning. This bug is
+ * related to 1005206 and 1006576. Diagnosis:
+ * 
+ * New code added to prune LongRecord MVVs operates on a copy of the original
+ * buffer and then atomically refreshes the original buffer from the copy. That
+ * operation fails to invalidate the FastIndex of the original buffer in the
+ * event a key is removed due to a LongRecord MVV becoming a primordial
+ * anti-value.
+ * 
+ * In addition, pruneLongMvvValues uses a Value object obtained from a
+ * ThreadLocal. However, this same Value is already in use by
+ * Exchange#storeInternal.
+ * 
+ * @author peter
+ * 
+ */
+public class Bug1010079Test extends MVCCTestBase {
+
+    @Test
+    public void induceCorruption() throws Exception {
+        /*
+         * 1. Create a page with a long record MVV.
+         * 
+         * 2. Read the value to set up the LevelCache
+         * 
+         * 3. Abort the transaction so that long record MVV will become an
+         * AntiValue
+         * 
+         * 4. Prune the Buffer to liberate the long record chain.
+         * 
+         * 5. Store a value in the page (not yet certain why this is needed to
+         * induce the bug, but it is.)
+         * 
+         * 6. Attempt to the read the first value back in.
+         * 
+         * Theory of bug is that the read attempt will not see a generation
+         * change, will use the LevelCache, and will attempt to read a long
+         * record from a page that has now become a data page.
+         */
+
+        /*
+         * Disable background pruning
+         */
+        _persistit.getCleanupManager().setPollInterval(-1);
+
+        /*
+         * Bump the generation number -- perhaps unnecessary
+         */
+        for (int i = 0; i < 10; i++) {
+            ex1.to(i).store();
+            ex1.remove();
+        }
+
+        /*
+         * Store a single Long MVV
+         */
+        trx1.begin();
+        storeLongMVV(ex1, 1);
+        ex1.fetch();
+        trx1.rollback();
+        trx1.end();
+
+        _persistit.getTransactionIndex().updateActiveTransactionCache();
+
+        ex1.prune();
+
+        ex1.getValue().put(RED_FOX);
+        ex1.to(0).store();
+
+        /*
+         * This method throws a CorruptVolumeException
+         */
+        ex1.to(1).fetch();
+        assertTrue("Value should have been removed by rollback", !ex1.getValue().isDefined());
+    }
+}

=== modified file 'src/test/java/com/persistit/MVCCPruneBufferTest.java'
--- src/test/java/com/persistit/MVCCPruneBufferTest.java	2012-06-04 14:59:07 +0000
+++ src/test/java/com/persistit/MVCCPruneBufferTest.java	2012-06-08 00:57:19 +0000
@@ -48,7 +48,7 @@
         int available = buffer1.getAvailableSize();
         int keys = buffer1.getKeyCount();
         buffer1.claim(true);
-        buffer1.pruneMvvValues(null);
+        buffer1.pruneMvvValues(null, true);
         assertEquals(keys, buffer1.getKeyCount());
         assertTrue(buffer1.getMvvCount() > 0);
         assertEquals(available, buffer1.getAvailableSize());
@@ -58,7 +58,7 @@
 
         _persistit.getTransactionIndex().updateActiveTransactionCache();
 
-        buffer1.pruneMvvValues(null);
+        buffer1.pruneMvvValues(null, true);
         assertEquals(0, _persistit.getCleanupManager().getAcceptedCount());
         assertTrue("Pruning should have removed primordial Anti-values", keys > buffer1.getKeyCount());
         // mvvCount is 1 because there is still a leading primordial AntiValue
@@ -83,7 +83,7 @@
         int available = buffer1.getAvailableSize();
         int keys = buffer1.getKeyCount();
         buffer1.claim(true);
-        buffer1.pruneMvvValues(null);
+        buffer1.pruneMvvValues(null, true);
         buffer1.release();
         assertEquals(keys, buffer1.getKeyCount());
         assertTrue(buffer1.getMvvCount() > 0);
@@ -107,7 +107,7 @@
         for (long page = 1; page < pageCount; page++) {
             final Buffer buffer = ex1.getBufferPool().get(ex1.getVolume(), page, true, true);
             try {
-                buffer.pruneMvvValues(null);
+                buffer.pruneMvvValues(null, true);
             } finally {
                 buffer.release();
             }
@@ -128,7 +128,7 @@
         for (long page = 1; page < pageCount; page++) {
             final Buffer buffer = ex1.getBufferPool().get(ex1.getVolume(), page, true, true);
             try {
-                buffer.pruneMvvValues(ex1.getTree());
+                buffer.pruneMvvValues(ex1.getTree(), true);
             } finally {
                 buffer.release();
             }