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

[Peter Beaman]

Fix a critical bug that was added with lp:~pbeaman/akiban-persistit/fix_945244_incomplete_pruning.

New code added with that branch failed to restore a Value to normal, non-long-record-mode after using it. When that value is used again, it is in an incorrect state.

This branch fixes the specific problem and adds a test to cover it.

Longer term the handling of "long record mode" in the Value object is murky at best and will be cleaned up when we refactor the Exchange class.

[Nathan Williams (nwilliams)]

Ack, nasty one.

I think this is right. I said that last time, but I've believed it both times!

About the test:
Do we want the pollInterval to be -1 instead of MAX? If correct as-is, feel free to Approve.

review: Needs Information

[Peter Beaman (pbeaman)]

Yes, -1 is correct. Confusion was caused by my earlier mistake when I set some of these to MAX. I'll press the big-A button.

[Akiban Build User (build-akiban)]

There were 2 failures during build/test:

* job server-mt failed at build number 1747: http://172.16.20.104:8080/job/server-mt/1747/

* view must-pass failed: server-mt is red

review: Needs Fixing

[Nathan Williams (nwilliams)]

All the way through and then network hiccup on server-mt.

=== modified file 'src/main/java/com/persistit/Buffer.java'
--- src/main/java/com/persistit/Buffer.java	2012-05-25 18:50:59 +0000
+++ src/main/java/com/persistit/Buffer.java	2012-05-31 02:25:21 +0000
@@ -3729,6 +3729,7 @@
                         // Rewrite the tail block header
                         putInt(tail, encodeTailBlock(newTailSize, klength));
                         valueByte = newSize > 0 ? _bytes[offset] & 0xFF : -1;
+                        value.changeLongRecordMode(false);
                     }
                 }
                 boolean prunedAntiValue = pruneAntiValue(valueByte, p, tree);

=== modified file 'src/main/java/com/persistit/Value.java'
--- src/main/java/com/persistit/Value.java	2012-05-25 18:50:59 +0000
+++ src/main/java/com/persistit/Value.java	2012-05-31 02:25:21 +0000
@@ -683,9 +683,12 @@
      *            The <code>Value</code> to which state should be copied.
      */
     public void copyTo(Value target) {
-        if (target == this)
+        if (target == this) {
             return;
-        target.changeLongRecordMode(isLongRecordMode());
+        }
+        Debug.$assert0.t(!isLongRecordMode());
+        Debug.$assert0.t(!target.isLongRecordMode());
+
         target.ensureFit(_size);
         System.arraycopy(_bytes, 0, target._bytes, 0, _size);
         target._size = _size;
@@ -775,12 +778,14 @@
      *         larger array.
      */
     public boolean ensureFit(int length) {
+
         if (length > 0 && length * SIZE_GROWTH_DENOMINATOR < _size) {
             length = _size / SIZE_GROWTH_DENOMINATOR;
         }
         final int newSize = _size + length;
-        if (newSize <= _bytes.length)
+        if (newSize <= _bytes.length) {
             return false;
+        }
         int newArraySize = ((newSize + SIZE_GRANULARITY - 1) / SIZE_GRANULARITY) * SIZE_GRANULARITY;
         if (newArraySize > _maximumSize)
             newArraySize = _maximumSize;
@@ -4064,6 +4069,7 @@
 
     void changeLongRecordMode(boolean mode) {
         if (mode != _longMode) {
+
             if (_longBytes == null || _longBytes.length < Buffer.LONGREC_SIZE) {
                 _longBytes = new byte[Buffer.LONGREC_SIZE];
                 _longSize = Buffer.LONGREC_SIZE;
@@ -4080,11 +4086,15 @@
             _longSize = tempSize;
             _longMode = mode;
 
-            if (mode) {
-                Debug.$assert1.t(_bytes.length == Buffer.LONGREC_SIZE);
-            } else {
-                Debug.$assert1.t(_longBytes.length == Buffer.LONGREC_SIZE);
-            }
+            assertLongRecordModeIsCorrect();
+        }
+    }
+
+    private void assertLongRecordModeIsCorrect() {
+        if (_longMode) {
+            Debug.$assert1.t(_bytes.length == Buffer.LONGREC_SIZE);
+        } else {
+            Debug.$assert1.t(_longBytes == null || _longBytes.length == Buffer.LONGREC_SIZE);
         }
     }
 
@@ -5071,8 +5081,8 @@
     }
 
     /**
-     * Construct a list of <code>Version</code> objects, each denoting one of the
-     * multi-value versions currently held in this Value object.
+     * Construct a list of <code>Version</code> objects, each denoting one of
+     * the multi-value versions currently held in this Value object.
      * 
      * @return the list of <code>Version<code>s
      * @throws PersistitException

=== modified file 'src/test/java/com/persistit/MVCCPruneBufferTest.java'
--- src/test/java/com/persistit/MVCCPruneBufferTest.java	2012-05-25 18:50:59 +0000
+++ src/test/java/com/persistit/MVCCPruneBufferTest.java	2012-05-31 02:25:21 +0000
@@ -183,6 +183,27 @@
         ex1.prune();
         assertTrue("Should no longer be an MVV", !ex1.isValueLongMVV());
     }
+    
+    @Test
+    public void induceBug1006576() throws Exception {
+        _persistit.getCleanupManager().setPollInterval(Long.MAX_VALUE);
+        trx1.begin();
+        storeLongMVV(ex1, "x");
+        storeLongMVV(ex1, "y");
+        trx1.commit();
+        trx1.end();
+        _persistit.getTransactionIndex().cleanup();
+        ex1.prune();
+        assertTrue("Should no longer be an MVV", !ex1.isValueLongMVV());
+        
+        trx1.begin();
+        storeLongMVV(ex1, "x");
+        trx1.commit();
+        trx1.end();
+        _persistit.getTransactionIndex().cleanup();
+        ex1.prune();
+        assertTrue("Should no longer be an MVV", !ex1.isValueLongMVV());
+    }
 
     @Test
     public void testComplexPruning() throws Exception {