SERVER-44027 Remove global X lock for renameCollection between DBs

3 files changed, 64 insertions, 44 deletions

diff --git a/jstests/noPassthrough/lock_stats.js b/jstests/noPassthrough/lock_stats.js
index 404f5a589ae..8667ba40cb6 100644
--- a/jstests/noPassthrough/lock_stats.js
+++ b/jstests/noPassthrough/lock_stats.js
@@ -5,19 +5,33 @@
 (function() {
 'use strict';
 
-function testBlockTime(blockTimeMillis) {
-    assert.commandWorked(db.getSiblingDB('t1').createCollection('test'));
+const waitForCommand = function(waitingFor, opFilter) {
+    let opId = -1;
+    assert.soon(function() {
+        print(`Checking for ${waitingFor}`);
+        const curopRes = db.getSiblingDB("admin").currentOp();
+        assert.commandWorked(curopRes);
+        const foundOp = curopRes["inprog"].filter(opFilter);
+
+        if (foundOp.length == 1) {
+            opId = foundOp[0]["opid"];
+        }
+        return (foundOp.length == 1);
+    });
+    return opId;
+};
 
+function testBlockTime(blockTimeMillis) {
     // Lock the database, and in parallel start an operation that needs the lock, so it blocks.
     assert.commandWorked(db.fsyncLock());
     var startStats = db.serverStatus().locks.Global;
     var startTime = new Date();
     var minBlockedMillis = blockTimeMillis;
 
-    // This is just some command that requires a MODE_X global lock that conflicts.
-    let awaitRename = startParallelShell(() => {
-        assert.commandWorked(
-            db.adminCommand({renameCollection: 't1.test', to: 't2.test', dropTarget: true}));
+    let awaitSleepCmd = startParallelShell(() => {
+        assert.commandFailedWithCode(
+            db.adminCommand({sleep: 1, secs: 500, lock: "w", $comment: "Lock sleep"}),
+            ErrorCodes.Interrupted);
     }, conn.port);
 
     // Wait until we see somebody waiting to acquire the lock, defend against unset stats.
@@ -32,12 +46,17 @@ function testBlockTime(blockTimeMillis) {
         return stats.acquireWaitCount.W > startStats.acquireWaitCount.W;
     }));
 
+    const sleepID = waitForCommand(
+        "sleepCmd", op => (op["ns"] == "admin.$cmd" && op["command"]["$comment"] == "Lock sleep"));
+
     // Sleep for minBlockedMillis, so the acquirer would have to wait at least that long.
     sleep(minBlockedMillis);
     db.fsyncUnlock();
 
-    // Wait for the parallel shell to finish, so its stats will have been recorded.
-    awaitRename();
+    // Interrupt the sleep command.
+    assert.commandWorked(db.getSiblingDB("admin").killOp(sleepID));
+
+    awaitSleepCmd();
 
     // The fsync command from the shell cannot have possibly been blocked longer than this.
     var maxBlockedMillis = new Date() - startTime;
diff --git a/src/mongo/db/catalog/rename_collection.cpp b/src/mongo/db/catalog/rename_collection.cpp
index f42bac3e5cb..603dd15f9f1 100644
--- a/src/mongo/db/catalog/rename_collection.cpp
+++ b/src/mongo/db/catalog/rename_collection.cpp
@@ -446,9 +446,15 @@ Status renameBetweenDBs(OperationContext* opCtx,
                         const RenameCollectionOptions& options) {
     invariant(source.db() != target.db());
 
-    boost::optional<Lock::GlobalWrite> globalWriteLock;
-    if (!opCtx->lockState()->isW())
-        globalWriteLock.emplace(opCtx);
+    boost::optional<Lock::DBLock> sourceCollLock;
+    if (!opCtx->lockState()->isCollectionLockedForMode(source, MODE_S)) {
+        sourceCollLock.emplace(opCtx, source.db(), MODE_S);
+    }
+
+    boost::optional<Lock::DBLock> targetDBLock;
+    if (!opCtx->lockState()->isDbLockedForMode(target.db(), MODE_X)) {
+        targetDBLock.emplace(opCtx, target.db(), MODE_X);
+    }
 
     {
         auto dss = DatabaseShardingState::get(opCtx, source.db());
@@ -642,26 +648,21 @@ Status renameBetweenDBs(OperationContext* opCtx,
     }
 
     {
-        // Copy over all the data from source collection to temporary collection.
-        // We do not need global write exclusive access after obtaining the collection locks on the
-        // source and temporary collections. After copying the documents, each remaining stage of
-        // the cross-database rename will be responsible for its own lock management.
-        // Therefore, unless the caller has already acquired the global write lock prior to invoking
-        // this function, we relinquish global write access to the database after acquiring the
-        // collection locks.
-        // Collection locks must be obtained while holding the global lock to avoid any possibility
-        // of a deadlock.
-        AutoGetCollectionForRead autoSourceColl(opCtx, source);
-        AutoGetCollection autoTmpColl(opCtx, tmpName, MODE_IX);
+        NamespaceStringOrUUID tmpCollUUID =
+            NamespaceStringOrUUID(std::string(tmpName.db()), tmpColl->uuid());
         statsTracker.reset();
 
-        if (opCtx->getServiceContext()->getStorageEngine()->supportsDBLocking()) {
-            if (globalWriteLock) {
-                opCtx->lockState()->downgrade(resourceIdGlobal, MODE_IX);
-                invariant(!opCtx->lockState()->isW());
-            } else {
-                invariant(opCtx->lockState()->isW());
-            }
+        // Copy over all the data from source collection to temporary collection. For this we can
+        // drop the exclusive database lock on the target and grab an intent lock on the temporary
+        // collection.
+        targetDBLock.reset();
+
+        AutoGetCollection autoTmpColl(opCtx, tmpCollUUID, MODE_IX);
+        tmpColl = autoTmpColl.getCollection();
+        if (!tmpColl) {
+            return Status(ErrorCodes::NamespaceNotFound,
+                          str::stream() << "Temporary collection '" << tmpName
+                                        << "' was removed while renaming collection across DBs");
         }
 
         auto cursor = sourceColl->getCursor(opCtx);
@@ -702,7 +703,7 @@ Status renameBetweenDBs(OperationContext* opCtx,
                 return status;
         }
     }
-    globalWriteLock.reset();
+    sourceCollLock.reset();
 
     // Getting here means we successfully built the target copy. We now do the final
     // in-place rename and remove the source collection.
diff --git a/src/mongo/db/catalog/rename_collection_test.cpp b/src/mongo/db/catalog/rename_collection_test.cpp
index 9458d8566ba..eda9deaa375 100644
--- a/src/mongo/db/catalog/rename_collection_test.cpp
+++ b/src/mongo/db/catalog/rename_collection_test.cpp
@@ -125,7 +125,7 @@ public:
     std::vector<std::string> oplogEntries;
 
     bool onInsertsThrows = false;
-    bool onInsertsIsGlobalWriteLockExclusive = false;
+    bool onInsertsIsTargetDatabaseExclusivelyLocked = false;
 
     bool onRenameCollectionCalled = false;
     OptionalCollectionUUID onRenameCollectionDropTarget;
@@ -161,10 +161,9 @@ void OpObserverMock::onInserts(OperationContext* opCtx,
         uasserted(ErrorCodes::OperationFailed, "insert failed");
     }
 
-    // Check global lock state.
-    auto lockState = opCtx->lockState();
-    ASSERT_TRUE(lockState->isWriteLocked());
-    onInsertsIsGlobalWriteLockExclusive = lockState->isW();
+    ASSERT_TRUE(opCtx->lockState()->isCollectionLockedForMode(nss, MODE_X));
+    onInsertsIsTargetDatabaseExclusivelyLocked =
+        opCtx->lockState()->isDbLockedForMode(nss.db(), MODE_X);
 
     _logOp(opCtx, nss, "inserts");
     OpObserverNoop::onInserts(opCtx, nss, uuid, begin, end, fromMigrate);
@@ -1089,28 +1088,28 @@ TEST_F(RenameCollectionTest, RenameCollectionAcrossDatabaseDropsTemporaryCollect
     _checkOplogEntries(_opObserver->oplogEntries, {"create", "index", "drop"});
 }
 
-TEST_F(RenameCollectionTest,
-       RenameCollectionAcrossDatabaseDowngradesGlobalWriteLockToNonExclusive) {
+TEST_F(RenameCollectionTest, RenameCollectionAcrossDatabasesWithoutLocks) {
     _createCollection(_opCtx.get(), _sourceNss);
     _insertDocument(_opCtx.get(), _sourceNss, BSON("_id" << 0));
     ASSERT_OK(renameCollection(_opCtx.get(), _sourceNss, _targetNssDifferentDb, {}));
-    ASSERT_FALSE(_opObserver->onInsertsIsGlobalWriteLockExclusive);
+    ASSERT_FALSE(_opObserver->onInsertsIsTargetDatabaseExclusivelyLocked);
 }
 
-TEST_F(RenameCollectionTest,
-       RenameCollectionAcrossDatabaseKeepsGlobalWriteLockExclusiveIfCallerHasGlobalWriteLock) {
+TEST_F(RenameCollectionTest, RenameCollectionAcrossDatabasesWithLocks) {
     // This simulates the case when renameCollection is called using the applyOps command (different
     // from secondary oplog application).
     _createCollection(_opCtx.get(), _sourceNss);
     _insertDocument(_opCtx.get(), _sourceNss, BSON("_id" << 0));
-    Lock::GlobalWrite globalWrite(_opCtx.get());
+    Lock::DBLock sourceLk(_opCtx.get(), _sourceNss.db(), MODE_X);
+    Lock::DBLock targetLk(_opCtx.get(), _targetNssDifferentDb.db(), MODE_X);
     ASSERT_OK(renameCollection(_opCtx.get(), _sourceNss, _targetNssDifferentDb, {}));
-    ASSERT_TRUE(_opObserver->onInsertsIsGlobalWriteLockExclusive);
+    ASSERT_TRUE(_opObserver->onInsertsIsTargetDatabaseExclusivelyLocked);
 }
 
 TEST_F(RenameCollectionTest, CollectionPointerRemainsValidThroughRename) {
     _createCollection(_opCtx.get(), _sourceNss);
-    Lock::GlobalWrite globalWrite(_opCtx.get());
+    Lock::DBLock sourceLk(_opCtx.get(), _sourceNss.db(), MODE_X);
+    Lock::DBLock targetLk(_opCtx.get(), _targetNss.db(), MODE_X);
 
     // Get a pointer to the source collection, and ensure that it reports the expected namespace
     // string.
@@ -1147,7 +1146,8 @@ TEST_F(RenameCollectionTest, CatalogPointersRenameValidThroughRenameForApplyOps)
 
 TEST_F(RenameCollectionTest, CollectionCatalogMappingRemainsIntactThroughRename) {
     _createCollection(_opCtx.get(), _sourceNss);
-    Lock::GlobalWrite globalWrite(_opCtx.get());
+    Lock::DBLock sourceLk(_opCtx.get(), _sourceNss.db(), MODE_X);
+    Lock::DBLock targetLk(_opCtx.get(), _targetNss.db(), MODE_X);
     auto& catalog = CollectionCatalog::get(_opCtx.get());
     Collection* sourceColl = _getCollection_inlock(_opCtx.get(), _sourceNss);
     ASSERT(sourceColl);