SERVER-17030 handle WCE in index builds

6 files changed, 147 insertions, 124 deletions

diff --git a/src/mongo/db/catalog/index_catalog.cpp b/src/mongo/db/catalog/index_catalog.cpp
index 72d7bec02ad..134aae5e197 100644
--- a/src/mongo/db/catalog/index_catalog.cpp
+++ b/src/mongo/db/catalog/index_catalog.cpp
@@ -110,7 +110,8 @@ namespace mongo {
                                                                _getAccessMethodName(txn,
                                                                                     keyPattern),
                                                                spec );
-            IndexCatalogEntry* entry = _setupInMemoryStructures( txn, descriptor );
+            const bool initFromDisk = true;
+            IndexCatalogEntry* entry = _setupInMemoryStructures( txn, descriptor, initFromDisk );
 
             fassert( 17340, entry->isReady( txn )  );
         }
@@ -126,8 +127,40 @@ namespace mongo {
         return Status::OK();
     }
 
+namespace {
+    class IndexCleanupOnRollback : public RecoveryUnit::Change {
+    public:
+        /**
+         * None of these pointers are owned by this class.
+         */
+        IndexCleanupOnRollback(OperationContext* txn,
+                               Collection* collection,
+                               IndexCatalogEntryContainer* entries,
+                               const IndexDescriptor* desc)
+            : _txn(txn),
+              _collection(collection),
+              _entries(entries),
+              _desc(desc) {
+        }
+
+        virtual void commit() {}
+
+        virtual void rollback() {
+            _entries->remove(_desc);
+            _collection->infoCache()->reset(_txn);
+        }
+
+    private:
+        OperationContext* _txn;
+        Collection* _collection;
+        IndexCatalogEntryContainer* _entries;
+        const IndexDescriptor* _desc;
+    };
+} // namespace
+
     IndexCatalogEntry* IndexCatalog::_setupInMemoryStructures(OperationContext* txn,
-                                                              IndexDescriptor* descriptor) {
+                                                              IndexDescriptor* descriptor,
+                                                              bool initFromDisk) {
         auto_ptr<IndexDescriptor> descriptorCleanup( descriptor );
 
         auto_ptr<IndexCatalogEntry> entry( new IndexCatalogEntry( _collection->ns().ns(),
@@ -141,6 +174,13 @@ namespace mongo {
 
         IndexCatalogEntry* save = entry.get();
         _entries.add( entry.release() );
+        
+        if (!initFromDisk) {
+            txn->recoveryUnit()->registerChange(new IndexCleanupOnRollback(txn,
+                                                                           _collection,
+                                                                           &_entries,
+                                                                           descriptor));
+        }
 
         invariant( save == _entries.find( descriptor ) );
         invariant( save == _entries.find( descriptor->indexName() ) );
@@ -286,65 +326,6 @@ namespace mongo {
         _inProgressIndexes.erase(it);
     }
 
-namespace {
-    class IndexCleanupOnRollback : public RecoveryUnit::Change {
-    public:
-        /**
-         * None of these pointers are owned by this class.
-         */
-        IndexCleanupOnRollback(OperationContext* txn,
-                               Collection* collection,
-                               IndexCatalogEntryContainer* entries,
-                               const IndexDescriptor* desc)
-            : _txn(txn),
-              _collection(collection),
-              _entries(entries),
-              _desc(desc) {
-        }
-
-        virtual void commit() {}
-
-        virtual void rollback() {
-            _entries->remove(_desc);
-            _collection->infoCache()->reset(_txn);
-        }
-
-    private:
-        OperationContext* _txn;
-        Collection* _collection;
-        IndexCatalogEntryContainer* _entries;
-        const IndexDescriptor* _desc;
-    };
-
-    class IndexRemoveChange : public RecoveryUnit::Change {
-    public:
-        IndexRemoveChange(OperationContext* txn,
-                          Collection* collection,
-                          IndexCatalogEntryContainer* entries,
-                          IndexCatalogEntry* entry)
-            : _txn(txn),
-              _collection(collection),
-              _entries(entries),
-              _entry(entry) {
-        }
-
-        virtual void commit() {
-            delete _entry;
-        }
-
-        virtual void rollback() {
-            _entries->add(_entry);
-            _collection->infoCache()->reset(_txn);
-        }
-
-    private:
-        OperationContext* _txn;
-        Collection* _collection;
-        IndexCatalogEntryContainer* _entries;
-        IndexCatalogEntry* _entry;
-    };
-} // namespace
-
     Status IndexCatalog::createIndexOnEmptyCollection(OperationContext* txn, BSONObj spec) {
         invariant(txn->lockState()->isCollectionLockedForMode(_collection->ns().toString(),
                                                               MODE_X));
@@ -381,11 +362,6 @@ namespace {
         invariant( descriptor );
         invariant( entry == _entries.find( descriptor ) );
 
-        txn->recoveryUnit()->registerChange(new IndexCleanupOnRollback(txn,
-                                                                       _collection,
-                                                                       &_entries,
-                                                                       entry->descriptor()));
-
         status = entry->accessMethod()->initializeAsEmpty(txn);
         if (!status.isOK())
             return status;
@@ -436,49 +412,33 @@ namespace {
         _inProgress = true;
 
         /// ----------   setup in memory structures  ----------------
-
-        _entry = _catalog->_setupInMemoryStructures(_txn, descriptorCleaner.release());
+        const bool initFromDisk = false;
+        _entry = _catalog->_setupInMemoryStructures(_txn,
+                                                    descriptorCleaner.release(),
+                                                    initFromDisk);
 
         return Status::OK();
     }
 
     IndexCatalog::IndexBuildBlock::~IndexBuildBlock() {
-        if ( !_inProgress ) {
-            // taken care of already when success() is called
-            return;
-        }
-
-        try {
-            fail();
-        }
-        catch ( const AssertionException& exc ) {
-            log() << "exception in ~IndexBuildBlock trying to cleanup: " << exc;
-            log() << " going to fassert to preserve state";
-            fassertFailed( 17345 );
-        }
+        // Don't need to call fail() here, as rollback will clean everything up for us.
     }
 
     void IndexCatalog::IndexBuildBlock::fail() {
-        try {
-            fassert( 17204, _catalog->_collection->ok() ); // defensive
+        fassert( 17204, _catalog->_collection->ok() ); // defensive
 
-            _inProgress = false;
+        _inProgress = false;
 
-            IndexCatalogEntry* entry = _catalog->_entries.find( _indexName );
-            invariant( entry == _entry );
+        IndexCatalogEntry* entry = _catalog->_entries.find( _indexName );
+        invariant( entry == _entry );
 
-            if ( entry ) {
-                _catalog->_dropIndex(_txn, entry);
-            }
-            else {
-                _catalog->_deleteIndexFromDisk( _txn,
-                                                _indexName,
-                                                _indexNamespace );
-            }
+        if ( entry ) {
+            _catalog->_dropIndex(_txn, entry);
         }
-        catch (const DBException& exc) {
-            error() << "exception while cleaning up in-progress index build: " << exc.what();
-            fassertFailedWithStatus(17493, exc.toStatus());
+        else {
+            _catalog->_deleteIndexFromDisk( _txn,
+                                            _indexName,
+                                            _indexNamespace );
         }
     }
 
@@ -797,6 +757,36 @@ namespace {
         return _dropIndex(txn, entry);
     }
 
+namespace {
+    class IndexRemoveChange : public RecoveryUnit::Change {
+    public:
+        IndexRemoveChange(OperationContext* txn,
+                          Collection* collection,
+                          IndexCatalogEntryContainer* entries,
+                          IndexCatalogEntry* entry)
+            : _txn(txn),
+              _collection(collection),
+              _entries(entries),
+              _entry(entry) {
+        }
+
+        virtual void commit() {
+            delete _entry;
+        }
+
+        virtual void rollback() {
+            _entries->add(_entry);
+            _collection->infoCache()->reset(_txn);
+        }
+
+    private:
+        OperationContext* _txn;
+        Collection* _collection;
+        IndexCatalogEntryContainer* _entries;
+        IndexCatalogEntry* _entry;
+    };
+} // namespace
+
     Status IndexCatalog::_dropIndex(OperationContext* txn,
                                     IndexCatalogEntry* entry ) {
         /**
@@ -1059,8 +1049,11 @@ namespace {
 
         // Delete the IndexCatalogEntry that owns this descriptor.  After deletion, 'oldDesc' is
         // invalid and should not be dereferenced.
-        const bool removed = _entries.remove( oldDesc );
-        invariant( removed );
+        IndexCatalogEntry* oldEntry = _entries.release(oldDesc);
+        txn->recoveryUnit()->registerChange(new IndexRemoveChange(txn, 
+                                                                  _collection,
+                                                                  &_entries,
+                                                                  oldEntry));
 
         // Ask the CollectionCatalogEntry for the new index spec.
         BSONObj spec = _collection->getCatalogEntry()->getIndexSpec( txn, indexName ).getOwned();
@@ -1070,11 +1063,12 @@ namespace {
         IndexDescriptor* newDesc = new IndexDescriptor( _collection,
                                                         _getAccessMethodName( txn, keyPattern ),
                                                         spec );
-        const IndexCatalogEntry* entry = _setupInMemoryStructures( txn, newDesc );
-        invariant( entry->isReady( txn ) );
+        const bool initFromDisk = false;
+        const IndexCatalogEntry* newEntry = _setupInMemoryStructures( txn, newDesc, initFromDisk );
+        invariant( newEntry->isReady( txn ) );
 
         // Return the new descriptor.
-        return entry->descriptor();
+        return newEntry->descriptor();
     }
 
     // ---------------------------
diff --git a/src/mongo/db/catalog/index_catalog.h b/src/mongo/db/catalog/index_catalog.h
index 5b689eea0ca..115dab8a29e 100644
--- a/src/mongo/db/catalog/index_catalog.h
+++ b/src/mongo/db/catalog/index_catalog.h
@@ -347,8 +347,11 @@ namespace mongo {
                                    const std::string& indexNamespace );
 
         // descriptor ownership passes to _setupInMemoryStructures
+        // initFromDisk: Avoids registering a change to undo this operation when set to true.
+        //               You must set this flag if calling this function outside of a UnitOfWork.
         IndexCatalogEntry* _setupInMemoryStructures(OperationContext* txn,
-                                                    IndexDescriptor* descriptor );
+                                                    IndexDescriptor* descriptor,
+                                                    bool initFromDisk);
 
         // Apply a set of transformations to the user-provided index object 'spec' to make it
         // conform to the standard for insertion.  This function adds the 'v' field if it didn't
diff --git a/src/mongo/db/catalog/index_create.cpp b/src/mongo/db/catalog/index_create.cpp
index 1fbd0f4452d..60314c24b20 100644
--- a/src/mongo/db/catalog/index_create.cpp
+++ b/src/mongo/db/catalog/index_create.cpp
@@ -43,6 +43,7 @@
 #include "mongo/db/background.h"
 #include "mongo/db/catalog/collection.h"
 #include "mongo/db/clientcursor.h"
+#include "mongo/db/concurrency/write_conflict_exception.h"
 #include "mongo/db/curop.h"
 #include "mongo/db/query/internal_plans.h"
 #include "mongo/db/repl/oplog.h"
@@ -72,6 +73,22 @@ namespace mongo {
         MultiIndexBlock* const _indexer;
     };
 
+    /**
+     * On rollback in init(), cleans up _indexes so that ~MultiIndexBlock doesn't try to clean
+     * up _indexes manually (since the changes were already rolled back).
+     * Due to this, it is thus legal to call init() again after it fails.
+     */
+    class MultiIndexBlock::CleanupIndexesVectorOnRollback : public RecoveryUnit::Change {
+    public:
+        explicit CleanupIndexesVectorOnRollback(MultiIndexBlock* indexer) : _indexer(indexer) {}
+
+        virtual void commit() {}
+        virtual void rollback() { _indexer->_indexes.clear(); }
+
+    private:
+        MultiIndexBlock* const _indexer;
+    };
+
     MultiIndexBlock::MultiIndexBlock(OperationContext* txn, Collection* collection)
         : _collection(collection),
           _txn(txn),
@@ -84,24 +101,30 @@ namespace mongo {
     MultiIndexBlock::~MultiIndexBlock() {
         if (!_needToCleanup || _indexes.empty())
             return;
-
-        try {
-            WriteUnitOfWork wunit(_txn);
-            // This cleans up all index builds. Because that may need to write, it is done inside
-            // of a WUOW. Nothing inside this block can fail, and it is made fatal if it does.
-            for (size_t i = 0; i < _indexes.size(); i++) {
-                _indexes[i].block->fail();
+        while (true) {
+            try {
+                WriteUnitOfWork wunit(_txn);
+                // This cleans up all index builds. 
+                // Because that may need to write, it is done inside
+                // of a WUOW. Nothing inside this block can fail, and it is made fatal if it does.
+                for (size_t i = 0; i < _indexes.size(); i++) {
+                    _indexes[i].block->fail();
+                }
+                wunit.commit();
+                return;
             }
-            wunit.commit();
-            return;
-        }
-        catch (const std::exception& e) {
-            error() << "Caught exception while cleaning up partially built indexes: " << e.what();
-        }
-        catch (...) {
-            error() << "Caught unknown exception while cleaning up partially built indexes.";
+            catch (const WriteConflictException& e) {
+                continue;
+            }
+            catch (const std::exception& e) {
+                error() << "Caught exception while cleaning up partially built indexes: "
+                        << e.what();
+            }
+            catch (...) {
+                error() << "Caught unknown exception while cleaning up partially built indexes.";
+            }
+            fassertFailed(18644);
         }
-        fassertFailed(18644);
     }
 
     void MultiIndexBlock::removeExistingIndexes(std::vector<BSONObj>* specs) const {
@@ -118,6 +141,10 @@ namespace mongo {
 
     Status MultiIndexBlock::init(const std::vector<BSONObj>& indexSpecs) {
         WriteUnitOfWork wunit(_txn);
+
+        invariant(_indexes.empty());
+        _txn->recoveryUnit()->registerChange(new CleanupIndexesVectorOnRollback(this));
+
         const string& ns = _collection->ns().ns();
 
         Status status = _collection->getIndexCatalog()->checkUnfinished();
diff --git a/src/mongo/db/catalog/index_create.h b/src/mongo/db/catalog/index_create.h
index 4a4be247e77..c4c3badd901 100644
--- a/src/mongo/db/catalog/index_create.h
+++ b/src/mongo/db/catalog/index_create.h
@@ -197,6 +197,7 @@ namespace mongo {
 
     private:
         class SetNeedToCleanupOnRollback;
+        class CleanupIndexesVectorOnRollback;
 
         struct IndexToBuild {
             IndexToBuild() : real(NULL) {}
diff --git a/src/mongo/db/index_builder.cpp b/src/mongo/db/index_builder.cpp
index 199129304ec..dca08c69de0 100644
--- a/src/mongo/db/index_builder.cpp
+++ b/src/mongo/db/index_builder.cpp
@@ -197,9 +197,9 @@ namespace {
 
                 if (allowBackgroundBuilding) {
                     dbLock->relockWithMode(MODE_X);
-                    Database* db = dbHolder().get(txn, ns.db());
-                    fassert(28553, db);
-                    fassert(28554, db->getCollection(ns.ns()));
+                    Database* reloadDb = dbHolder().get(txn, ns.db());
+                    fassert(28553, reloadDb);
+                    fassert(28554, reloadDb->getCollection(ns.ns()));
                     indexer.unregisterIndexBuild(descriptor);
                 }
 
diff --git a/src/mongo/dbtests/repltests.cpp b/src/mongo/dbtests/repltests.cpp
index 1daaae6ec71..dc94738218a 100644
--- a/src/mongo/dbtests/repltests.cpp
+++ b/src/mongo/dbtests/repltests.cpp
@@ -199,14 +199,12 @@ namespace ReplTests {
                 b.append("host", "localhost");
                 b.appendTimestamp("syncedTo", 0);
                 ReplSource a(&_txn, b.obj());
-                WriteUnitOfWork wunit(&_txn);
                 for( vector< BSONObj >::iterator i = ops.begin(); i != ops.end(); ++i ) {
                     if ( 0 ) {
                         mongo::unittest::log() << "op: " << *i << endl;
                     }
                     a.applyOperation( &_txn, ctx.db(), *i );
                 }
-                wunit.commit();
             }
         }
         void printAll( const char *ns ) {