SERVER-16676 don't trash indexes for RecordStores that can compact in-place

12 files changed, 67 insertions, 54 deletions

diff --git a/jstests/core/compact_keeps_indexes.js b/jstests/core/compact_keeps_indexes.js
new file mode 100644
index 00000000000..4047c048e17
--- /dev/null
+++ b/jstests/core/compact_keeps_indexes.js
@@ -0,0 +1,21 @@
+// SERVER-16676 Make sure compact doesn't leave the collection with bad indexes
+
+(function() {
+    'use strict';
+
+    var coll = db.compact_keeps_indexes;
+
+    coll.insert({_id:1, x:1});
+    coll.ensureIndex({x:1});
+
+    assert.eq(coll.getIndexes().length, 2);
+
+    var res = coll.runCommand('compact');
+    if (res.code != 115) { // CommandNotSupported
+        assert.commandWorked(res);
+    }
+
+    assert.eq(coll.getIndexes().length, 2);
+    assert.eq(coll.find({_id:1}).itcount(), 1);
+    assert.eq(coll.find({x:1}).itcount(), 1);
+}())
diff --git a/src/mongo/db/catalog/collection_compact.cpp b/src/mongo/db/catalog/collection_compact.cpp
index 352868694f9..264716ba889 100644
--- a/src/mongo/db/catalog/collection_compact.cpp
+++ b/src/mongo/db/catalog/collection_compact.cpp
@@ -106,11 +106,22 @@ namespace mongo {
         dassert(txn->lockState()->isCollectionLockedForMode(ns().toString(), MODE_X));
 
         if ( !_recordStore->compactSupported() )
-            return StatusWith<CompactStats>( ErrorCodes::BadValue,
+            return StatusWith<CompactStats>( ErrorCodes::CommandNotSupported,
                                              str::stream() <<
                                              "cannot compact collection with record store: " <<
                                              _recordStore->name() );
 
+        if (_recordStore->compactsInPlace()) {
+            // Since we are compacting in-place, we don't need to touch the indexes.
+            // TODO SERVER-16856 compact indexes
+            CompactStats stats;
+            Status status = _recordStore->compact(txn, NULL, compactOptions, &stats);
+            if (!status.isOK())
+                return StatusWith<CompactStats>(status);
+
+            return StatusWith<CompactStats>(stats);
+        }
+
         if ( _indexCatalog.numIndexesInProgress( txn ) )
             return StatusWith<CompactStats>( ErrorCodes::BadValue,
                                              "cannot compact when indexes in progress" );
@@ -166,7 +177,9 @@ namespace mongo {
 
         MyCompactAdaptor adaptor(this, &indexer);
 
-        _recordStore->compact( txn, &adaptor, compactOptions, &stats );
+        status = _recordStore->compact( txn, &adaptor, compactOptions, &stats);
+        if (!status.isOK())
+            return StatusWith<CompactStats>(status);
 
         log() << "starting index commits";
         status = indexer.doneInserting();
diff --git a/src/mongo/db/storage/devnull/devnull_kv_engine.cpp b/src/mongo/db/storage/devnull/devnull_kv_engine.cpp
index 768688d3569..830b3f35b74 100644
--- a/src/mongo/db/storage/devnull/devnull_kv_engine.cpp
+++ b/src/mongo/db/storage/devnull/devnull_kv_engine.cpp
@@ -141,12 +141,6 @@ namespace mongo {
                                               RecordId end,
                                               bool inclusive) { }
 
-        virtual bool compactSupported() const { return false; }
-        virtual Status compact( OperationContext* txn,
-                                RecordStoreCompactAdaptor* adaptor,
-                                const CompactOptions* options,
-                                CompactStats* stats ) { return Status::OK(); }
-
         virtual Status validate( OperationContext* txn,
                                  bool full, bool scanData,
                                  ValidateAdaptor* adaptor,
diff --git a/src/mongo/db/storage/in_memory/in_memory_record_store.cpp b/src/mongo/db/storage/in_memory/in_memory_record_store.cpp
index c3df05879dc..f4a2dbb2812 100644
--- a/src/mongo/db/storage/in_memory/in_memory_record_store.cpp
+++ b/src/mongo/db/storage/in_memory/in_memory_record_store.cpp
@@ -396,17 +396,6 @@ namespace mongo {
         }
     }
 
-    bool InMemoryRecordStore::compactSupported() const {
-        return false;
-    }
-    Status InMemoryRecordStore::compact(OperationContext* txn,
-                                        RecordStoreCompactAdaptor* adaptor,
-                                        const CompactOptions* options,
-                                        CompactStats* stats) {
-        // TODO might be possible to do something here
-        invariant(!"compact not yet implemented");
-    }
-
     Status InMemoryRecordStore::validate(OperationContext* txn,
                                          bool full,
                                          bool scanData,
diff --git a/src/mongo/db/storage/in_memory/in_memory_record_store.h b/src/mongo/db/storage/in_memory/in_memory_record_store.h
index c371a12135f..8e1e93b708e 100644
--- a/src/mongo/db/storage/in_memory/in_memory_record_store.h
+++ b/src/mongo/db/storage/in_memory/in_memory_record_store.h
@@ -99,12 +99,6 @@ namespace mongo {
 
         virtual void temp_cappedTruncateAfter( OperationContext* txn, RecordId end, bool inclusive );
 
-        virtual bool compactSupported() const;
-        virtual Status compact( OperationContext* txn,
-                                RecordStoreCompactAdaptor* adaptor,
-                                const CompactOptions* options,
-                                CompactStats* stats );
-
         virtual Status validate( OperationContext* txn,
                                  bool full,
                                  bool scanData,
diff --git a/src/mongo/db/storage/mmap_v1/heap_record_store_btree.h b/src/mongo/db/storage/mmap_v1/heap_record_store_btree.h
index 4553620c6d9..188ce725124 100644
--- a/src/mongo/db/storage/mmap_v1/heap_record_store_btree.h
+++ b/src/mongo/db/storage/mmap_v1/heap_record_store_btree.h
@@ -117,13 +117,6 @@ namespace mongo {
 
         virtual bool compactSupported() const { invariant(false); }
 
-        virtual Status compact(OperationContext* txn,
-                               RecordStoreCompactAdaptor* adaptor,
-                               const CompactOptions* options,
-                               CompactStats* stats) {
-            invariant(false);
-        }
-
         virtual Status validate(OperationContext* txn,
                                 bool full,
                                 bool scanData,
diff --git a/src/mongo/db/storage/mmap_v1/record_store_v1_capped.cpp b/src/mongo/db/storage/mmap_v1/record_store_v1_capped.cpp
index 98adeed89a9..3648deff9d0 100644
--- a/src/mongo/db/storage/mmap_v1/record_store_v1_capped.cpp
+++ b/src/mongo/db/storage/mmap_v1/record_store_v1_capped.cpp
@@ -177,7 +177,7 @@ namespace mongo {
                     return StatusWith<DiskLoc>( status );
                 deleteRecord( txn, fr );
 
-                compact(txn);
+                _compact(txn);
                 if ((++passes % 5000) == 0) {
                     StringBuilder sb;
                     log() << "passes = " << passes << " in CappedRecordStoreV1::allocRecord:"
@@ -267,7 +267,7 @@ namespace mongo {
        this is O(n^2) but we call it for capped tables where typically n==1 or 2!
        (or 3...there will be a little unused sliver at the end of the extent.)
     */
-    void CappedRecordStoreV1::compact(OperationContext* txn) {
+    void CappedRecordStoreV1::_compact(OperationContext* txn) {
         DDD( "CappedRecordStoreV1::compact enter" );
 
         vector<DiskLoc> drecs;
@@ -490,7 +490,7 @@ namespace mongo {
             Status status = _deleteCallback->aboutToDeleteCapped( txn, currId );
             uassertStatusOK( status );
             deleteRecord( txn, currId );
-            compact(txn);
+            _compact(txn);
 
             // This is the case where we have not yet had to remove any
             // documents to make room for other documents, and we are allocating
@@ -596,7 +596,7 @@ namespace mongo {
         else {
             d->nextDeleted() = cappedFirstDeletedInCurExtent();
             setFirstDeletedInCurExtent( txn, dloc );
-            // always compact() after this so order doesn't matter
+            // always _compact() after this so order doesn't matter
         }
     }
 
@@ -663,13 +663,6 @@ namespace mongo {
         return iterators.release();
     }
 
-    Status CappedRecordStoreV1::compact( OperationContext* txn,
-                                         RecordStoreCompactAdaptor* adaptor,
-                                         const CompactOptions* options,
-                                         CompactStats* stats ) {
-        invariant(false);
-    }
-
     void CappedRecordStoreV1::_maybeComplain( OperationContext* txn, int len ) const {
         RARELY {
             std::stringstream buf;
diff --git a/src/mongo/db/storage/mmap_v1/record_store_v1_capped.h b/src/mongo/db/storage/mmap_v1/record_store_v1_capped.h
index 880421a32aa..323630b3221 100644
--- a/src/mongo/db/storage/mmap_v1/record_store_v1_capped.h
+++ b/src/mongo/db/storage/mmap_v1/record_store_v1_capped.h
@@ -68,13 +68,6 @@ namespace mongo {
 
         virtual std::vector<RecordIterator*> getManyIterators( OperationContext* txn ) const;
 
-        virtual bool compactSupported() const { return false; }
-
-        virtual Status compact( OperationContext* txn,
-                                RecordStoreCompactAdaptor* adaptor,
-                                const CompactOptions* options,
-                                CompactStats* stats );
-
         // Start from firstExtent by default.
         DiskLoc firstRecord( OperationContext* txn,
                              const DiskLoc &startExtent = DiskLoc() ) const;
@@ -99,7 +92,7 @@ namespace mongo {
 
     private:
         // -- start copy from cap.cpp --
-        void compact(OperationContext* txn);
+        void _compact(OperationContext* txn);
         DiskLoc cappedFirstDeletedInCurExtent() const;
         void setFirstDeletedInCurExtent( OperationContext* txn, const DiskLoc& loc );
         void cappedCheckMigrate(OperationContext* txn);
diff --git a/src/mongo/db/storage/mmap_v1/record_store_v1_simple.h b/src/mongo/db/storage/mmap_v1/record_store_v1_simple.h
index e55b19bce4e..c04e4397c2c 100644
--- a/src/mongo/db/storage/mmap_v1/record_store_v1_simple.h
+++ b/src/mongo/db/storage/mmap_v1/record_store_v1_simple.h
@@ -62,6 +62,7 @@ namespace mongo {
         }
 
         virtual bool compactSupported() const { return true; }
+        virtual bool compactsInPlace() const { return false; }
         virtual Status compact( OperationContext* txn,
                                 RecordStoreCompactAdaptor* adaptor,
                                 const CompactOptions* options,
diff --git a/src/mongo/db/storage/record_store.h b/src/mongo/db/storage/record_store.h
index c94061d1701..de0703dbbed 100644
--- a/src/mongo/db/storage/record_store.h
+++ b/src/mongo/db/storage/record_store.h
@@ -259,12 +259,32 @@ namespace mongo {
                                               RecordId end,
                                               bool inclusive) = 0;
 
-        // does this RecordStore support the compact operation
-        virtual bool compactSupported() const = 0;
+        /**
+         * does this RecordStore support the compact operation?
+         *
+         * If you return true, you must provide implementations of all compact methods.
+         */
+        virtual bool compactSupported() const { return false; }
+
+        /**
+         * Does compact() leave RecordIds alone or can they change.
+         *
+         * Only called if compactSupported() returns true.
+         */
+        virtual bool compactsInPlace() const { invariant(false); }
+
+        /**
+         * Attempt to reduce the storage space used by this RecordStore.
+         *
+         * Only called if compactSupported() returns true.
+         * No RecordStoreCompactAdaptor will be passed if compactsInPlace() returns true.
+         */
         virtual Status compact( OperationContext* txn,
                                 RecordStoreCompactAdaptor* adaptor,
                                 const CompactOptions* options,
-                                CompactStats* stats ) = 0;
+                                CompactStats* stats ) {
+            invariant(false);
+        }
 
         /**
          * @param full - does more checks
diff --git a/src/mongo/db/storage/rocks/rocks_record_store.h b/src/mongo/db/storage/rocks/rocks_record_store.h
index 9aa951bc764..952dd6413f4 100644
--- a/src/mongo/db/storage/rocks/rocks_record_store.h
+++ b/src/mongo/db/storage/rocks/rocks_record_store.h
@@ -144,6 +144,7 @@ namespace mongo {
         virtual Status truncate( OperationContext* txn );
 
         virtual bool compactSupported() const { return true; }
+        virtual bool compactsInPlace() const { return true; }
 
         virtual Status compact( OperationContext* txn,
                                 RecordStoreCompactAdaptor* adaptor,
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h
index 74dc4ec30bd..6a9e79f24f2 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h
@@ -142,6 +142,7 @@ namespace mongo {
         virtual Status truncate( OperationContext* txn );
 
         virtual bool compactSupported() const { return true; }
+        virtual bool compactsInPlace() const { return true; }
 
         virtual Status compact( OperationContext* txn,
                                 RecordStoreCompactAdaptor* adaptor,