diff options
author | Geert Bosch <geert@mongodb.com> | 2019-02-14 09:50:31 -0500 |
---|---|---|
committer | Geert Bosch <geert@mongodb.com> | 2019-04-16 18:18:21 -0400 |
commit | 8914838187ee4124ce5512093109f457d23281b6 (patch) | |
tree | e38a62584e9ae042ae2e186d95d524be76a26313 | |
parent | a9f798957c907ec118a635ff4a0cc9a1a55c7114 (diff) | |
download | mongo-8914838187ee4124ce5512093109f457d23281b6.tar.gz |
SERVER-39517 Only use Collection MODE_X for index creation and drop
-rw-r--r-- | jstests/core/txns/indexing_not_blocked_by_txn.js | 37 | ||||
-rw-r--r-- | src/mongo/db/catalog/drop_indexes.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/catalog/index_build_block.cpp | 22 | ||||
-rw-r--r-- | src/mongo/db/catalog/index_catalog_impl.h | 2 | ||||
-rw-r--r-- | src/mongo/db/commands/create_indexes.cpp | 102 | ||||
-rw-r--r-- | src/mongo/db/concurrency/d_concurrency.cpp | 5 | ||||
-rw-r--r-- | src/mongo/db/index/index_access_method.cpp | 6 | ||||
-rw-r--r-- | src/mongo/dbtests/validate_tests.cpp | 7 |
8 files changed, 103 insertions, 86 deletions
diff --git a/jstests/core/txns/indexing_not_blocked_by_txn.js b/jstests/core/txns/indexing_not_blocked_by_txn.js new file mode 100644 index 00000000000..47bfaebe83d --- /dev/null +++ b/jstests/core/txns/indexing_not_blocked_by_txn.js @@ -0,0 +1,37 @@ +/* + * This test ensures that creating and dropping an index on a collection + * does not conflict with multi-statement transactions on different collections + * as a result of taking strong database locks. Additionally tests that a + * createIndexes for an index that already exist does not conflict with an open + * transaction on that same collection. + * @tags: [uses_transactions, assumes_unsharded_collection] + */ +(function() { + "use strict"; + var dbName = 'indexing_not_blocked_by_txn'; + var mydb = db.getSiblingDB(dbName); + const wcMajority = {writeConcern: {w: "majority"}}; + + mydb.foo.drop(wcMajority); + mydb.bar.drop(wcMajority); + assert.commandWorked(mydb.createCollection("foo", wcMajority)); + assert.commandWorked(mydb.foo.createIndex({x: 1})); + assert.commandWorked(mydb.createCollection("bar", wcMajority)); + + var session = db.getMongo().startSession(); + var sessionDb = session.getDatabase(dbName); + + session.startTransaction(); + assert.commandWorked(sessionDb.foo.insert({x: 1})); + + // Creating already existing index is a no-op that shouldn't take strong locks. + assert.commandWorked(mydb.foo.createIndex({x: 1})); + + // Creating an index on a different collection should not conflict. + assert.commandWorked(mydb.bar.createIndex({x: 1})); + // Dropping shouldn't either. + assert.commandWorked(mydb.bar.dropIndex({x: 1})); + + assert.commandWorked(session.commitTransaction_forTesting()); + session.endSession(); +}()); diff --git a/src/mongo/db/catalog/drop_indexes.cpp b/src/mongo/db/catalog/drop_indexes.cpp index 56143447308..ef6074eaaff 100644 --- a/src/mongo/db/catalog/drop_indexes.cpp +++ b/src/mongo/db/catalog/drop_indexes.cpp @@ -200,7 +200,7 @@ Status dropIndexes(OperationContext* opCtx, const BSONObj& cmdObj, BSONObjBuilder* result) { return writeConflictRetry(opCtx, "dropIndexes", nss.db(), [opCtx, &nss, &cmdObj, result] { - AutoGetDb autoDb(opCtx, nss.db(), MODE_X); + AutoGetCollection autoColl(opCtx, nss, MODE_IX, MODE_X); bool userInitiatedWritesAndNotPrimary = opCtx->writesAreReplicated() && !repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, nss); @@ -215,9 +215,9 @@ Status dropIndexes(OperationContext* opCtx, } // If db/collection does not exist, short circuit and return. - Database* db = autoDb.getDb(); - Collection* collection = db ? db->getCollection(opCtx, nss) : nullptr; - if (!db || !collection) { + Database* db = autoColl.getDb(); + Collection* collection = autoColl.getCollection(); + if (!collection) { if (db && ViewCatalog::get(db)->lookup(opCtx, nss.ns())) { return Status(ErrorCodes::CommandNotSupportedOnView, str::stream() << "Cannot drop indexes on view " << nss); diff --git a/src/mongo/db/catalog/index_build_block.cpp b/src/mongo/db/catalog/index_build_block.cpp index af1bd77af68..46231723f62 100644 --- a/src/mongo/db/catalog/index_build_block.cpp +++ b/src/mongo/db/catalog/index_build_block.cpp @@ -50,7 +50,7 @@ IndexCatalogImpl::IndexBuildBlock::IndexBuildBlock(IndexCatalogImpl* catalog, const NamespaceString& nss, const BSONObj& spec, IndexBuildMethod method) - : _catalog(catalog), _ns(nss.ns()), _spec(spec.getOwned()), _method(method), _entry(nullptr) {} + : _catalog(catalog), _nss(nss), _spec(spec.getOwned()), _method(method), _entry(nullptr) {} void IndexCatalogImpl::IndexBuildBlock::deleteTemporaryTables(OperationContext* opCtx) { if (_indexBuildInterceptor) { @@ -137,14 +137,7 @@ void IndexCatalogImpl::IndexBuildBlock::fail(OperationContext* opCtx, invariant(opCtx->lockState()->inAWriteUnitOfWork()); fassert(17204, collection->ok()); // defensive - NamespaceString ns(_indexNamespace); - // TODO(SERVER-39520): Once createCollection does not need database IX lock, 'system.views' will - // be no longer a special case. - if (ns.coll().startsWith(NamespaceString::kSystemDotViewsCollectionName)) { - invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_IX)); - } else { - invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_X)); - } + invariant(opCtx->lockState()->isCollectionLockedForMode(_nss, MODE_X)); if (_entry) { invariant(_catalog->_dropIndex(opCtx, _entry).isOK()); @@ -161,14 +154,7 @@ void IndexCatalogImpl::IndexBuildBlock::success(OperationContext* opCtx, Collect invariant(opCtx->lockState()->inAWriteUnitOfWork()); fassert(17207, collection->ok()); - NamespaceString ns(_indexNamespace); - // TODO(SERVER-39520): Once createCollection does not need database IX lock, 'system.views' will - // be no longer a special case. - if (ns.coll().startsWith(NamespaceString::kSystemDotViewsCollectionName)) { - invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_IX)); - } else { - invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_X)); - } + invariant(opCtx->lockState()->isCollectionLockedForMode(_nss, MODE_X)); if (_indexBuildInterceptor) { // An index build should never be completed with writes remaining in the interceptor. @@ -178,7 +164,7 @@ void IndexCatalogImpl::IndexBuildBlock::success(OperationContext* opCtx, Collect invariant(_indexBuildInterceptor->areAllConstraintsChecked(opCtx)); } - log() << "index build: done building index " << _indexName << " on ns " << _ns; + log() << "index build: done building index " << _indexName << " on ns " << _nss; collection->indexBuildSuccess(opCtx, _entry); diff --git a/src/mongo/db/catalog/index_catalog_impl.h b/src/mongo/db/catalog/index_catalog_impl.h index 05d790b46b3..f7736b642fa 100644 --- a/src/mongo/db/catalog/index_catalog_impl.h +++ b/src/mongo/db/catalog/index_catalog_impl.h @@ -298,7 +298,7 @@ public: private: IndexCatalogImpl* const _catalog; - const std::string _ns; + const NamespaceString& _nss; BSONObj _spec; IndexBuildMethod _method; diff --git a/src/mongo/db/commands/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp index 5156edad90b..368298d286c 100644 --- a/src/mongo/db/commands/create_indexes.cpp +++ b/src/mongo/db/commands/create_indexes.cpp @@ -265,32 +265,31 @@ bool runCreateIndexes(OperationContext* opCtx, return true; }; - // Before potentially taking an exclusive database lock, check if all indexes already exist - // while holding an intent lock. Only continue if new indexes need to be built and the - // database should be re-locked in exclusive mode. + // Before potentially taking an exclusive database or collection lock, check if all indexes + // already exist while holding an intent lock. Only continue if new indexes need to be built + // and the collection or database should be re-locked in exclusive mode. { - AutoGetCollection autoColl(opCtx, ns, MODE_IX); - if (auto collection = autoColl.getCollection()) { + boost::optional<AutoGetCollection> autoColl(boost::in_place_init, opCtx, ns, MODE_IX); + if (auto collection = autoColl->getCollection()) { auto specsCopy = resolveDefaultsAndRemoveExistingIndexes(opCtx, collection, specs); if (specsCopy.size() == 0) { return indexesAlreadyExist(collection->getIndexCatalog()->numIndexesTotal(opCtx)); } + } else { + // We'll need to create a collection and maybe even a new database. Temporarily + // releases the Database lock while holding a Global IX lock. This prevents replication + // from changing, but requires abandoning the current snapshot in case indexes change + // during the period of time where no database lock is held. + autoColl.reset(); + opCtx->recoveryUnit()->abandonSnapshot(); + dbLock.relockWithMode(MODE_X); } } - // Relocking temporarily releases the Database lock while holding a Global IX lock. This - // prevents the replication state from changing, but requires abandoning the current - // snapshot in case indexes change during the period of time where no database lock is held. - opCtx->recoveryUnit()->abandonSnapshot(); - dbLock.relockWithMode(MODE_X); - - // Allow the strong lock acquisition above to be interrupted, but from this point forward do - // not allow locks or re-locks to be interrupted. - UninterruptibleLockGuard noInterrupt(opCtx->lockState()); - auto databaseHolder = DatabaseHolder::get(opCtx); auto db = databaseHolder->getDb(opCtx, ns.db()); if (!db) { + invariant(opCtx->lockState()->isDbLockedForMode(ns.db(), MODE_X)); db = databaseHolder->openDb(opCtx, ns.db()); } @@ -300,10 +299,18 @@ bool runCreateIndexes(OperationContext* opCtx, dss.checkDbVersion(opCtx, dssLock); } + opCtx->recoveryUnit()->abandonSnapshot(); + boost::optional<Lock::CollectionLock> exclusiveCollectionLock( + boost::in_place_init, opCtx, ns.ns(), MODE_X); + + // Allow the strong lock acquisition above to be interrupted, but from this point forward do + // not allow locks or re-locks to be interrupted. + UninterruptibleLockGuard noInterrupt(opCtx->lockState()); + Collection* collection = db->getCollection(opCtx, ns); - if (collection) { - result.appendBool("createdCollectionAutomatically", false); - } else { + bool createCollectionAutomatically = collection == nullptr; + result.appendBool("createdCollectionAutomatically", createCollectionAutomatically); + if (createCollectionAutomatically) { if (ViewCatalog::get(db)->lookup(opCtx, ns.ns())) { errmsg = "Cannot create indexes on a view"; uasserted(ErrorCodes::CommandNotSupportedOnView, errmsg); @@ -317,7 +324,6 @@ bool runCreateIndexes(OperationContext* opCtx, invariant(collection); wunit.commit(); }); - result.appendBool("createdCollectionAutomatically", true); } // Use AutoStatsTracker to update Top. @@ -354,6 +360,10 @@ bool runCreateIndexes(OperationContext* opCtx, // The 'indexer' can throw, so ensure the build cleanup occurs. ON_BLOCK_EXIT([&] { + if (!exclusiveCollectionLock) { + opCtx->recoveryUnit()->abandonSnapshot(); + exclusiveCollectionLock.emplace(opCtx, ns.ns(), MODE_X); + } if (MONGO_FAIL_POINT(leaveIndexBuildUnfinishedForShutdown)) { // Set a flag to leave the persisted index build state intact when cleanUpAfterBuild() // is called below. The index build will be found on server startup. @@ -363,7 +373,6 @@ bool runCreateIndexes(OperationContext* opCtx, // commit() clears the state. indexer.abortWithoutCleanup(opCtx); } - invariant(opCtx->lockState()->isDbLockedForMode(dbname, MODE_X)); indexer.cleanUpAfterBuild(opCtx, collection); }); @@ -376,32 +385,19 @@ bool runCreateIndexes(OperationContext* opCtx, MultiIndexBlock::makeTimestampedIndexOnInitFn(opCtx, collection))); }); - // If we're a background index, replace exclusive db lock with an intent lock, so that - // other readers and writers can proceed during this phase. + // Don't hold an exclusive collection lock during background indexing, so that other readers + // and writers can proceed during this phase. A BackgroundOperation has been registered on the + // namespace, so the collection cannot be removed after yielding the lock. if (indexer.isBackgroundBuilding()) { + invariant(BackgroundOperation::inProgForNs(ns)); opCtx->recoveryUnit()->abandonSnapshot(); - dbLock.relockWithMode(MODE_IX); - } - - auto relockOnErrorGuard = makeGuard([&] { - // Must have exclusive DB lock before we clean up the index build via the - // destructor of 'indexer'. - if (indexer.isBackgroundBuilding()) { - try { - // This function cannot throw today, but we will preemptively prepare for - // that day, to avoid data corruption due to lack of index cleanup. - opCtx->recoveryUnit()->abandonSnapshot(); - dbLock.relockWithMode(MODE_X); - } catch (...) { - std::terminate(); - } - } - }); + exclusiveCollectionLock.reset(); + } // Collection scan and insert into index, followed by a drain of writes received in the // background. { - Lock::CollectionLock colLock(opCtx, ns.ns(), MODE_IX); + Lock::CollectionLock colLock(opCtx, ns.ns(), MODE_IS); uassertStatusOK(indexer.insertAllDocumentsInCollection(opCtx, collection)); } @@ -429,7 +425,6 @@ bool runCreateIndexes(OperationContext* opCtx, { opCtx->recoveryUnit()->abandonSnapshot(); Lock::CollectionLock colLock(opCtx, ns.ns(), MODE_S); - uassertStatusOK(indexer.drainBackgroundWrites(opCtx)); } @@ -438,26 +433,21 @@ bool runCreateIndexes(OperationContext* opCtx, MONGO_FAIL_POINT_PAUSE_WHILE_SET(hangAfterIndexBuildSecondDrain); } - relockOnErrorGuard.dismiss(); - - // Need to return db lock back to exclusive, to complete the index build. + // Need to get exclusive collection lock back to complete the index build. if (indexer.isBackgroundBuilding()) { opCtx->recoveryUnit()->abandonSnapshot(); - dbLock.relockWithMode(MODE_X); - - auto db = databaseHolder->getDb(opCtx, ns.db()); - if (db) { - auto& dss = DatabaseShardingState::get(db); - auto dssLock = DatabaseShardingState::DSSLock::lock(opCtx, &dss); - dss.checkDbVersion(opCtx, dssLock); - } + exclusiveCollectionLock.emplace(opCtx, ns.ns(), MODE_X); + } - invariant(db); - invariant(db->getCollection(opCtx, ns)); + db = databaseHolder->getDb(opCtx, ns.db()); + invariant(db->getCollection(opCtx, ns)); + { + auto& dss = DatabaseShardingState::get(db); + auto dssLock = DatabaseShardingState::DSSLock::lock(opCtx, &dss); + dss.checkDbVersion(opCtx, dssLock); } - // Perform the third and final drain after releasing a shared lock and reacquiring an - // exclusive lock on the database. + // Perform the third and final drain while holding the exclusive collection lock. uassertStatusOK(indexer.drainBackgroundWrites(opCtx)); // This is required before completion. diff --git a/src/mongo/db/concurrency/d_concurrency.cpp b/src/mongo/db/concurrency/d_concurrency.cpp index bdd07a1d5e0..f49c225a807 100644 --- a/src/mongo/db/concurrency/d_concurrency.cpp +++ b/src/mongo/db/concurrency/d_concurrency.cpp @@ -265,8 +265,9 @@ void Lock::DBLock::relockWithMode(LockMode newMode) { // 2PL would delay the unlocking invariant(!_opCtx->lockState()->inAWriteUnitOfWork()); - // Not allowed to change global intent - invariant(!isSharedLockMode(_mode) || isSharedLockMode(newMode)); + // Not allowed to change global intent, so check when going from shared to exclusive. + if (isSharedLockMode(_mode) && !isSharedLockMode(newMode)) + invariant(_opCtx->lockState()->isWriteLocked()); _opCtx->lockState()->unlock(_id); _mode = newMode; diff --git a/src/mongo/db/index/index_access_method.cpp b/src/mongo/db/index/index_access_method.cpp index f017dd20f88..0b3c6abcce5 100644 --- a/src/mongo/db/index/index_access_method.cpp +++ b/src/mongo/db/index/index_access_method.cpp @@ -583,7 +583,11 @@ Status AbstractIndexAccessMethod::BulkBuilderImpl::insert(OperationContext* opCt BSONObjSet keys = SimpleBSONObjComparator::kInstance.makeBSONObjSet(); MultikeyPaths multikeyPaths; - _real->getKeys(obj, options.getKeysMode, &keys, &_multikeyMetadataKeys, &multikeyPaths); + try { + _real->getKeys(obj, options.getKeysMode, &keys, &_multikeyMetadataKeys, &multikeyPaths); + } catch (...) { + return exceptionToStatus(); + } if (!multikeyPaths.empty()) { if (_indexMultikeyPaths.empty()) { diff --git a/src/mongo/dbtests/validate_tests.cpp b/src/mongo/dbtests/validate_tests.cpp index 290176b500f..9fb912f9538 100644 --- a/src/mongo/dbtests/validate_tests.cpp +++ b/src/mongo/dbtests/validate_tests.cpp @@ -679,8 +679,8 @@ public: wunit.commit(); } - // Create a partial geo index that indexes the document. This should throw an error. - ASSERT_THROWS(dbtests::createIndexFromSpec(&_opCtx, + // Create a partial geo index that indexes the document. This should return an error. + ASSERT_NOT_OK(dbtests::createIndexFromSpec(&_opCtx, coll->ns().ns(), BSON("name" << "partial_index" @@ -694,8 +694,7 @@ public: << "background" << false << "partialFilterExpression" - << BSON("a" << BSON("$eq" << 2)))), - AssertionException); + << BSON("a" << BSON("$eq" << 2))))); // Create a partial geo index that does not index the document. auto status = dbtests::createIndexFromSpec(&_opCtx, |