From 1a42e98facd32e981104074d0087ddec55758425 Mon Sep 17 00:00:00 2001 From: Gregory Wlodarek Date: Wed, 16 Oct 2019 14:07:41 +0000 Subject: SERVER-33272 The DatabaseHolder::close() function no longer requires a global write lock and neither does the dropDatabase command. --- .../dropdatabase_respect_maxtimems.js | 70 ++++++++++++++-------- jstests/noPassthrough/lock_stats.js | 11 +++- src/mongo/db/catalog/collection_catalog.cpp | 2 +- src/mongo/db/catalog/database_holder_impl.cpp | 3 +- src/mongo/db/catalog/drop_database.cpp | 2 - src/mongo/db/catalog/drop_database_test.cpp | 5 +- src/mongo/db/commands/sleep_command.cpp | 12 +++- 7 files changed, 67 insertions(+), 38 deletions(-) diff --git a/jstests/noPassthrough/dropdatabase_respect_maxtimems.js b/jstests/noPassthrough/dropdatabase_respect_maxtimems.js index db93575c993..a6f16456a7a 100644 --- a/jstests/noPassthrough/dropdatabase_respect_maxtimems.js +++ b/jstests/noPassthrough/dropdatabase_respect_maxtimems.js @@ -7,17 +7,30 @@ const rst = ReplSetTest({nodes: 1}); rst.startSet(); rst.initiate(); -const adminDB = rst.getPrimary().getDB("admin"); -const txnDB = rst.getPrimary().getDB("txn"); const dropDB = rst.getPrimary().getDB("drop"); -(function assertColletionDropCanBeInterrupted() { - assert.commandWorked(txnDB.foo.insert({})); +const waitForCommand = function(waitingFor, opFilter) { + let opId = -1; + assert.soon(function() { + print(`Checking for ${waitingFor}`); + const curopRes = dropDB.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 assertCollectionDropCanBeInterrupted() { assert.commandWorked(dropDB.bar.insert({})); - const session = txnDB.getMongo().startSession({causalConsistency: false}); - const sessionDB = session.getDatabase("txn"); + const session = dropDB.getMongo().startSession({causalConsistency: false}); + const sessionDB = session.getDatabase("drop"); session.startTransaction(); - assert.commandWorked(sessionDB.foo.insert({})); + assert.commandWorked(sessionDB.bar.insert({})); assert.commandFailedWithCode(dropDB.runCommand({dropDatabase: 1, maxTimeMS: 100}), ErrorCodes.MaxTimeMSExpired); @@ -26,38 +39,47 @@ const dropDB = rst.getPrimary().getDB("drop"); })(); (function assertDatabaseDropCanBeInterrupted() { - assert.commandWorked(txnDB.foo.insert({})); + load("jstests/libs/check_log.js"); + assert.commandWorked(dropDB.bar.insert({})); assert.commandWorked(rst.getPrimary().adminCommand( {configureFailPoint: "dropDatabaseHangAfterAllCollectionsDrop", mode: "alwaysOn"})); // This will get blocked by the failpoint when collection drop phase finishes. - let dropDatabaseShell = startParallelShell( - "assert.commandFailedWithCode(db.getSiblingDB(\"drop\").runCommand({dropDatabase: 1, maxTimeMS: 5000}), ErrorCodes.MaxTimeMSExpired);", - rst.getPrimary().port); + let dropDatabaseShell = startParallelShell(() => { + assert.commandFailedWithCode( + db.getSiblingDB("drop").runCommand({dropDatabase: 1, maxTimeMS: 5000}), + ErrorCodes.MaxTimeMSExpired); + }, rst.getPrimary().port); - assert.soon(function() { - const sessionFilter = {active: true, "command.dropDatabase": 1}; - const res = adminDB.aggregate([{$currentOp: {}}, {$match: sessionFilter}]); - return res.hasNext(); - }, "Timeout waiting for dropDatabase to start"); + checkLog.contains( + dropDB.getMongo(), + "dropDatabase - fail point dropDatabaseHangAfterAllCollectionsDrop enabled. Blocking until fail point is disabled."); - const session = txnDB.getMongo().startSession({causalConsistency: false}); - const sessionDB = session.getDatabase("txn"); - session.startTransaction(); - assert.commandWorked(sessionDB.foo.insert({})); + let sleepCommand = startParallelShell(() => { + // Make dropDatabase timeout. + assert.commandFailedWithCode( + db.getSiblingDB("drop").adminCommand( + {sleep: 1, secs: 500, lockTarget: "drop", lock: "ir", $comment: "Lock sleep"}), + ErrorCodes.Interrupted); + }, rst.getPrimary().port); + + checkLog.contains(dropDB.getMongo(), "test only command sleep invoked"); // dropDatabase now gets unblocked by the failpoint but will immediately - // get blocked by acquiring the GlobalWrite lock for dropping the database. + // get blocked by acquiring the database lock for dropping the database. assert.commandWorked(rst.getPrimary().adminCommand( {configureFailPoint: "dropDatabaseHangAfterAllCollectionsDrop", mode: "off"})); - // This should timeout. dropDatabaseShell(); - assert.commandWorked(session.commitTransaction_forTesting()); - session.endSession(); + // Interrupt the sleep command. + const sleepID = waitForCommand( + "sleepCmd", op => (op["ns"] == "admin.$cmd" && op["command"]["$comment"] == "Lock sleep")); + assert.commandWorked(dropDB.getSiblingDB("admin").killOp(sleepID)); + + sleepCommand(); })(); rst.stopSet(); diff --git a/jstests/noPassthrough/lock_stats.js b/jstests/noPassthrough/lock_stats.js index 1274dd326c4..404f5a589ae 100644 --- a/jstests/noPassthrough/lock_stats.js +++ b/jstests/noPassthrough/lock_stats.js @@ -6,14 +6,19 @@ 'use strict'; function testBlockTime(blockTimeMillis) { + assert.commandWorked(db.getSiblingDB('t1').createCollection('test')); + // 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. - var s = startParallelShell( - 'assert.commandWorked(db.getSiblingDB(\'nonexisting\').dropDatabase());', conn.port); + let awaitRename = startParallelShell(() => { + assert.commandWorked( + db.adminCommand({renameCollection: 't1.test', to: 't2.test', dropTarget: true})); + }, conn.port); // Wait until we see somebody waiting to acquire the lock, defend against unset stats. assert.soon((function() { @@ -32,7 +37,7 @@ function testBlockTime(blockTimeMillis) { db.fsyncUnlock(); // Wait for the parallel shell to finish, so its stats will have been recorded. - s(); + awaitRename(); // 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/collection_catalog.cpp b/src/mongo/db/catalog/collection_catalog.cpp index c32c11405fc..a080463f486 100644 --- a/src/mongo/db/catalog/collection_catalog.cpp +++ b/src/mongo/db/catalog/collection_catalog.cpp @@ -212,7 +212,7 @@ void CollectionCatalog::setCollectionNamespace(OperationContext* opCtx, } void CollectionCatalog::onCloseDatabase(OperationContext* opCtx, std::string dbName) { - invariant(opCtx->lockState()->isW()); + invariant(opCtx->lockState()->isDbLockedForMode(dbName, MODE_X)); auto rid = ResourceId(RESOURCE_DATABASE, dbName); removeResource(rid, dbName); } diff --git a/src/mongo/db/catalog/database_holder_impl.cpp b/src/mongo/db/catalog/database_holder_impl.cpp index 943350b0c89..e528491248d 100644 --- a/src/mongo/db/catalog/database_holder_impl.cpp +++ b/src/mongo/db/catalog/database_holder_impl.cpp @@ -198,9 +198,8 @@ void DatabaseHolderImpl::dropDb(OperationContext* opCtx, Database* db) { } void DatabaseHolderImpl::close(OperationContext* opCtx, StringData ns) { - invariant(opCtx->lockState()->isW()); - const StringData dbName = _todb(ns); + invariant(opCtx->lockState()->isDbLockedForMode(dbName, MODE_X)); stdx::lock_guard lk(_m); diff --git a/src/mongo/db/catalog/drop_database.cpp b/src/mongo/db/catalog/drop_database.cpp index ccaa58bcfc0..65e56764762 100644 --- a/src/mongo/db/catalog/drop_database.cpp +++ b/src/mongo/db/catalog/drop_database.cpp @@ -126,7 +126,6 @@ Status dropDatabase(OperationContext* opCtx, const std::string& dbName) { repl::OpTime latestDropPendingOpTime; { - Lock::GlobalWrite lk(opCtx); AutoGetDb autoDB(opCtx, dbName, MODE_X); Database* const db = autoDB.getDb(); if (!db) { @@ -299,7 +298,6 @@ Status dropDatabase(OperationContext* opCtx, const std::string& dbName) { MONGO_FAIL_POINT_PAUSE_WHILE_SET(dropDatabaseHangAfterAllCollectionsDrop); } - Lock::GlobalWrite lk(opCtx); AutoGetDb autoDB(opCtx, dbName, MODE_X); auto db = autoDB.getDb(); if (!db) { diff --git a/src/mongo/db/catalog/drop_database_test.cpp b/src/mongo/db/catalog/drop_database_test.cpp index 16fc13ecb44..4cc9320cd97 100644 --- a/src/mongo/db/catalog/drop_database_test.cpp +++ b/src/mongo/db/catalog/drop_database_test.cpp @@ -197,7 +197,6 @@ void _createCollection(OperationContext* opCtx, const NamespaceString& nss) { * Removes database from catalog, bypassing dropDatabase(). */ void _removeDatabaseFromCatalog(OperationContext* opCtx, StringData dbName) { - Lock::GlobalWrite lk(opCtx); AutoGetDb autoDB(opCtx, dbName, MODE_X); auto db = autoDB.getDb(); // dropDatabase can call awaitReplication more than once, so do not attempt to drop the database @@ -382,7 +381,7 @@ TEST_F(DropDatabaseTest, }); { - Lock::GlobalWrite lk(_opCtx.get()); + Lock::DBLock lk(_opCtx.get(), _nss.db(), MODE_X); _testDropDatabase(_opCtx.get(), _opObserver, _nss, true); } @@ -410,7 +409,7 @@ TEST_F(DropDatabaseTest, _createCollection(_opCtx.get(), dpns); { - Lock::GlobalWrite lk(_opCtx.get()); + Lock::DBLock lk(_opCtx.get(), _nss.db(), MODE_X); ASSERT_OK(dropDatabase(_opCtx.get(), _nss.db().toString())); } diff --git a/src/mongo/db/commands/sleep_command.cpp b/src/mongo/db/commands/sleep_command.cpp index 594422fb77e..d2fd98c3180 100644 --- a/src/mongo/db/commands/sleep_command.cpp +++ b/src/mongo/db/commands/sleep_command.cpp @@ -58,7 +58,8 @@ public: "If neither 'secs' nor 'millis' is set, command will sleep for 10 seconds. " "If both are set, command will sleep for the sum of 'secs' and 'millis.'\n" " w: (deprecated: use 'lock' instead) if true, takes a write lock.\n" - " lock: r, w, none. If r or w, db will block under a lock. Defaults to r." + " lock: r, ir, w, iw, none. If r or w, db will block under a lock.\n" + " If ir or iw, db will block under an intent lock. Defaults to ir." " 'lock' and 'w' may not both be set.\n" " secs: Amount of time to sleep, in seconds.\n" " millis: Amount of time to sleep, in ms.\n" @@ -159,9 +160,14 @@ public: opCtx->sleepFor(Milliseconds(msRemaining)); } else if (lock == "w") { _sleepInLock(opCtx, msRemaining.count(), MODE_X, lockTarget); - } else { - uassert(34347, "'lock' must be one of 'r', 'w', 'none'.", lock == "r"); + } else if (lock == "iw") { + _sleepInLock(opCtx, msRemaining.count(), MODE_IX, lockTarget); + } else if (lock == "r") { _sleepInLock(opCtx, msRemaining.count(), MODE_S, lockTarget); + } else { + uassert( + 34347, "'lock' must be one of 'r', 'ir', 'w', 'iw', 'none'.", lock == "ir"); + _sleepInLock(opCtx, msRemaining.count(), MODE_IS, lockTarget); } } } -- cgit v1.2.1