diff options
-rw-r--r-- | jstests/noPassthrough/lock_stats.js | 35 | ||||
-rw-r--r-- | src/mongo/db/catalog/rename_collection.cpp | 45 | ||||
-rw-r--r-- | src/mongo/db/catalog/rename_collection_test.cpp | 28 |
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); |