SERVER-14668: pull collection creation even higher on update path to avoid deadlock

5 files changed, 136 insertions, 40 deletions

diff --git a/src/mongo/db/commands/find_and_modify.cpp b/src/mongo/db/commands/find_and_modify.cpp
index e7ec1b17b24..5b9fbdc7c3d 100644
--- a/src/mongo/db/commands/find_and_modify.cpp
+++ b/src/mongo/db/commands/find_and_modify.cpp
@@ -69,7 +69,7 @@ namespace mongo {
             find_and_modify::addPrivilegesRequiredForFindAndModify(this, dbname, cmdObj, out);
         }
         /* this will eventually replace run,  once sort is handled */
-        bool runNoDirectClient( OperationContext* txn, const string& dbname, BSONObj& cmdObj, int, string& errmsg, BSONObjBuilder& result, bool) {
+        bool runNoDirectClient( OperationContext* txn, const string& dbname, BSONObj& cmdObj, int, string& errmsg, BSONObjBuilder& result, bool fromRepl) {
             verify( cmdObj["sort"].eoo() );
 
             const string ns = dbname + '.' + cmdObj.firstElement().valuestr();
@@ -97,10 +97,36 @@ namespace mongo {
                 return false;
             }
 
-            return runNoDirectClient( txn, ns ,
-                                      query , fields , update , 
-                                      upsert , returnNew , remove , 
-                                      result , errmsg );
+            bool ok = runNoDirectClient( txn, ns,
+                                         query, fields, update,
+                                         upsert, returnNew, remove,
+                                         result, errmsg );
+
+            if ( !ok && errmsg == "no-collection" ) {
+                {
+                    Lock::DBLock lk(txn->lockState(), dbname, MODE_X);
+                    Client::Context ctx(txn, ns, false /* don't check version */);
+                    Database* db = ctx.db();
+                    if ( db->getCollection( txn, ns ) ) {
+                        // someone else beat us to it, that's ok
+                        // we might race while we unlock if someone drops
+                        // but that's ok, we'll just do nothing and error out
+                    }
+                    else {
+                        WriteUnitOfWork wuow(txn);
+                        uassertStatusOK( userCreateNS( txn, db,
+                                                       ns, BSONObj(),
+                                                       !fromRepl ) );
+                        wuow.commit();
+                    }
+                }
+                errmsg = "";
+                ok = runNoDirectClient( txn, ns,
+                                        query, fields, update,
+                                        upsert, returnNew, remove,
+                                        result, errmsg );
+            }
+            return ok;
         }
 
         static void _appendHelper(BSONObjBuilder& result,
@@ -139,6 +165,21 @@ namespace mongo {
 
             const WhereCallbackReal whereCallback = WhereCallbackReal(txn, StringData(ns));
 
+            if ( !collection ) {
+                if ( !upsert ) {
+                    // no collectio and no upsert, so can't possible do anything
+                    _appendHelper( result, BSONObj(), false, fields, whereCallback );
+                    return true;
+                }
+                // no collection, but upsert, so we want to create it
+                // problem is we only have IX on db and collection :(
+                // so we tell our caller who can do it
+                errmsg = "no-collection";
+                return false;
+            }
+
+
+
             BSONObj doc;
             bool found = false;
             {
diff --git a/src/mongo/db/commands/mr.cpp b/src/mongo/db/commands/mr.cpp
index d4ced8c5806..537543f5d75 100644
--- a/src/mongo/db/commands/mr.cpp
+++ b/src/mongo/db/commands/mr.cpp
@@ -533,6 +533,9 @@ namespace mongo {
 
             if (_config.outputOptions.outNonAtomic)
                 return postProcessCollectionNonAtomic(txn, op, pm);
+
+            invariant( !txn->lockState()->isLocked() );
+
             Lock::GlobalWrite lock(txn->lockState()); // TODO(erh): this is how it was, but seems it doesn't need to be global
             return postProcessCollectionNonAtomic(txn, op, pm);
         }
diff --git a/src/mongo/db/commands/write_commands/batch_executor.cpp b/src/mongo/db/commands/write_commands/batch_executor.cpp
index 4e0eac5a579..160a660bdee 100644
--- a/src/mongo/db/commands/write_commands/batch_executor.cpp
+++ b/src/mongo/db/commands/write_commands/batch_executor.cpp
@@ -1143,38 +1143,79 @@ namespace mongo {
             return;
         }
 
-        ///////////////////////////////////////////
-        Lock::DBLock dbLock(txn->lockState(), nsString.db(), MODE_IX);
-        Lock::CollectionLock colLock(txn->lockState(),
-                                     nsString.ns(),
-                                     isMulti ? MODE_X : MODE_IX);
-        ///////////////////////////////////////////
+        bool createCollection = false;
+        for ( int fakeLoop = 0; fakeLoop < 1; fakeLoop++ ) {
+
+            if ( createCollection ) {
+                Lock::DBLock lk(txn->lockState(), nsString.db(), MODE_X);
+                Client::Context ctx(txn, nsString.ns(), false /* don't check version */);
+                Database* db = ctx.db();
+                if ( db->getCollection( txn, nsString.ns() ) ) {
+                    // someone else beat us to it
+                }
+                else {
+                    WriteUnitOfWork wuow(txn);
+                    uassertStatusOK( userCreateNS( txn, db,
+                                                   nsString.ns(), BSONObj(),
+                                                   !request.isFromReplication() ) );
+                    wuow.commit();
+                }
+            }
 
-        if (!checkShardVersion(txn, &shardingState, *updateItem.getRequest(), result))
-            return;
+            ///////////////////////////////////////////
+            Lock::DBLock dbLock(txn->lockState(), nsString.db(), MODE_IX);
+            Lock::CollectionLock colLock(txn->lockState(),
+                                         nsString.ns(),
+                                         isMulti ? MODE_X : MODE_IX);
+            ///////////////////////////////////////////
 
-        Client::Context ctx(txn, nsString.ns(), false /* don't check version */);
+            if (!checkShardVersion(txn, &shardingState, *updateItem.getRequest(), result))
+                return;
 
-        try {
-            UpdateResult res = executor.execute(ctx.db());
+            Client::Context ctx(txn, nsString.ns(), false /* don't check version */);
 
-            const long long numDocsModified = res.numDocsModified;
-            const long long numMatched = res.numMatched;
-            const BSONObj resUpsertedID = res.upserted;
+            if ( ctx.db()->getCollection( txn, nsString.ns() ) == NULL ) {
+                if ( createCollection ) {
+                    // we raced with some, accept defeat
+                    result->getStats().nModified = 0;
+                    result->getStats().n = 0;
+                    return;
+                }
 
-            // We have an _id from an insert
-            const bool didInsert = !resUpsertedID.isEmpty();
+                if ( !request.isUpsert() ) {
+                    // not an upsert, not collection, nothing to do
+                    result->getStats().nModified = 0;
+                    result->getStats().n = 0;
+                    return;
+                }
 
-            result->getStats().nModified = didInsert ? 0 : numDocsModified;
-            result->getStats().n = didInsert ? 1 : numMatched;
-            result->getStats().upsertedID = resUpsertedID;
-        }
-        catch (const DBException& ex) {
-            status = ex.toStatus();
-            if (ErrorCodes::isInterruption(status.code())) {
-                throw;
+                // upsert, mark that we should create collection
+                fakeLoop = -1;
+                createCollection = true;
+                continue;
+            }
+
+            try {
+                UpdateResult res = executor.execute(ctx.db());
+
+                const long long numDocsModified = res.numDocsModified;
+                const long long numMatched = res.numMatched;
+                const BSONObj resUpsertedID = res.upserted;
+
+                // We have an _id from an insert
+                const bool didInsert = !resUpsertedID.isEmpty();
+
+                result->getStats().nModified = didInsert ? 0 : numDocsModified;
+                result->getStats().n = didInsert ? 1 : numMatched;
+                result->getStats().upsertedID = resUpsertedID;
+            }
+            catch (const DBException& ex) {
+                status = ex.toStatus();
+                if (ErrorCodes::isInterruption(status.code())) {
+                    throw;
+                }
+                result->setError(toWriteError(status));
             }
-            result->setError(toWriteError(status));
         }
     }
 
diff --git a/src/mongo/db/instance.cpp b/src/mongo/db/instance.cpp
index 26e0622648e..b1e27280ddc 100644
--- a/src/mongo/db/instance.cpp
+++ b/src/mongo/db/instance.cpp
@@ -642,7 +642,21 @@ namespace mongo {
         {
             Lock::DBLock dbLock(txn->lockState(), ns.db(), MODE_X);
             Client::Context ctx(txn, ns);
-            UpdateResult res = executor.execute(ctx.db());
+            Database* db = ctx.db();
+            if ( db->getCollection( txn, ns ) ) {
+                // someone else beat us to it, that's ok
+                // we might race while we unlock if someone drops
+                // but that's ok, we'll just do nothing and error out
+            }
+            else {
+                WriteUnitOfWork wuow(txn);
+                uassertStatusOK( userCreateNS( txn, db,
+                                               ns.ns(), BSONObj(),
+                                               true ) );
+                wuow.commit();
+            }
+
+            UpdateResult res = executor.execute(db);
             lastError.getSafe()->recordUpdate( res.existing , res.numMatched , res.upserted );
         }
     }
diff --git a/src/mongo/db/ops/update_executor.cpp b/src/mongo/db/ops/update_executor.cpp
index 696630876f3..3673382b9bd 100644
--- a/src/mongo/db/ops/update_executor.cpp
+++ b/src/mongo/db/ops/update_executor.cpp
@@ -118,15 +118,12 @@ namespace mongo {
         if (!collection && _request->isUpsert()) {
             OperationContext* const txn = _request->getOpCtx();
 
-            //  Upgrade to an exclusive lock. While this may possibly lead to a deadlock,
-            //  collection creation is rare and a retry will definitively succeed in this
-            //  case. Add a fail point to allow reliably triggering the deadlock situation.
-
-            MONGO_FAIL_POINT_BLOCK(implicitCollectionCreationDelay, data) {
-                LOG(0) << "Sleeping for creation of collection " + nsString.ns();
-                sleepmillis(1000);
-                LOG(0) << "About to upgrade to exclusive lock on " + nsString.ns();
-            }
+            // We have to have an exclsive lock on the db to be allowed to create the collection.
+            // Callers should either get an X or create the collection.
+            const Locker* locker = txn->lockState();
+            invariant( locker->isW() ||
+                       locker->isLockHeldForMode( ResourceId( RESOURCE_DATABASE, nsString.db() ),
+                                                  MODE_X ) );
 
             Lock::DBLock lk(txn->lockState(), nsString.db(), MODE_X);