diff options
author | Ali Mir <ali.mir@mongodb.com> | 2023-04-19 21:36:36 +0000 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2023-04-19 23:49:45 +0000 |
commit | aad09dc4ea618f95b25319a47d14f6ad46947c11 (patch) | |
tree | ffb438152adc5c73e9c217f2a05102304404b8e1 /src/mongo | |
parent | c94c4f3abd0f9c113067e360269301b14363d53e (diff) | |
download | mongo-aad09dc4ea618f95b25319a47d14f6ad46947c11.tar.gz |
SERVER-71443 Remove or document instances of UninterruptibleLockGuard in replication components
Diffstat (limited to 'src/mongo')
-rw-r--r-- | src/mongo/db/repl/oplog_buffer_collection.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/repl/storage_interface_impl.cpp | 82 | ||||
-rw-r--r-- | src/mongo/db/transaction/transaction_participant.cpp | 7 |
3 files changed, 49 insertions, 48 deletions
diff --git a/src/mongo/db/repl/oplog_buffer_collection.cpp b/src/mongo/db/repl/oplog_buffer_collection.cpp index 99dd1429751..7e17e97d63e 100644 --- a/src/mongo/db/repl/oplog_buffer_collection.cpp +++ b/src/mongo/db/repl/oplog_buffer_collection.cpp @@ -41,6 +41,7 @@ #include "mongo/db/catalog/clustered_collection_util.h" #include "mongo/db/catalog/collection_options.h" #include "mongo/db/catalog/document_validation.h" +#include "mongo/db/catalog_raii.h" #include "mongo/db/dbdirectclient.h" #include "mongo/db/ops/write_ops_exec.h" #include "mongo/db/repl/storage_interface.h" @@ -257,6 +258,9 @@ std::size_t OplogBufferCollection::getCount() const { void OplogBufferCollection::clear(OperationContext* opCtx) { stdx::lock_guard<Latch> lk(_mutex); + // We acquire the appropriate locks for the temporary oplog buffer collection here, + // so that we perform the drop and create under the same locks. + AutoGetCollection autoColl(opCtx, NamespaceString(kDefaultOplogCollectionNamespace), MODE_X); _dropCollection(opCtx); _createCollection(opCtx); _size = 0; @@ -460,8 +464,6 @@ void OplogBufferCollection::_createCollection(OperationContext* opCtx) { // overhead and improve _id query efficiency. options.clusteredIndex = clustered_util::makeDefaultClusteredIdIndex(); - // TODO (SERVER-71443): Fix to be interruptible or document exception. - UninterruptibleLockGuard noInterrupt(opCtx->lockState()); // NOLINT. auto status = _storageInterface->createCollection(opCtx, _nss, options); if (status.code() == ErrorCodes::NamespaceExists) return; @@ -469,8 +471,6 @@ void OplogBufferCollection::_createCollection(OperationContext* opCtx) { } void OplogBufferCollection::_dropCollection(OperationContext* opCtx) { - // TODO (SERVER-71443): Fix to be interruptible or document exception. - UninterruptibleLockGuard noInterrupt(opCtx->lockState()); // NOLINT. uassertStatusOK(_storageInterface->dropCollection(opCtx, _nss)); } diff --git a/src/mongo/db/repl/storage_interface_impl.cpp b/src/mongo/db/repl/storage_interface_impl.cpp index d0026a9bb58..4cf1fdb421b 100644 --- a/src/mongo/db/repl/storage_interface_impl.cpp +++ b/src/mongo/db/repl/storage_interface_impl.cpp @@ -127,8 +127,6 @@ StatusWith<int> StorageInterfaceImpl::getRollbackID(OperationContext* opCtx) { } StatusWith<int> StorageInterfaceImpl::initializeRollbackID(OperationContext* opCtx) { - // TODO (SERVER-71443): Fix to be interruptible or document exception. - UninterruptibleLockGuard noInterrupt(opCtx->lockState()); // NOLINT. auto status = createCollection(opCtx, _rollbackIdNss, CollectionOptions()); if (!status.isOK()) { return status; @@ -450,35 +448,35 @@ Status StorageInterfaceImpl::createCollection(OperationContext* opCtx, const CollectionOptions& options, const bool createIdIndex, const BSONObj& idIndexSpec) { - return writeConflictRetry(opCtx, "StorageInterfaceImpl::createCollection", nss.ns(), [&] { - AutoGetDb databaseWriteGuard(opCtx, nss.dbName(), MODE_IX); - auto db = databaseWriteGuard.ensureDbExists(opCtx); - invariant(db); - - // Check if there already exist a Collection/view on the given namespace 'nss'. The answer - // may change at any point after this call as we make this call without holding the - // collection lock. But, it is fine as we properly handle while registering the uncommitted - // collection with CollectionCatalog. This check is just here to prevent it from being - // created in the common case. - Status status = mongo::catalog::checkIfNamespaceExists(opCtx, nss); - if (!status.isOK()) { - return status; - } + try { + return writeConflictRetry(opCtx, "StorageInterfaceImpl::createCollection", nss.ns(), [&] { + AutoGetDb databaseWriteGuard(opCtx, nss.dbName(), MODE_IX); + auto db = databaseWriteGuard.ensureDbExists(opCtx); + invariant(db); + + // Check if there already exist a Collection/view on the given namespace 'nss'. The + // answer may change at any point after this call as we make this call without holding + // the collection lock. But, it is fine as we properly handle while registering the + // uncommitted collection with CollectionCatalog. This check is just here to prevent it + // from being created in the common case. + Status status = mongo::catalog::checkIfNamespaceExists(opCtx, nss); + if (!status.isOK()) { + return status; + } - Lock::CollectionLock lk(opCtx, nss, MODE_IX); - WriteUnitOfWork wuow(opCtx); - try { + Lock::CollectionLock lk(opCtx, nss, MODE_IX); + WriteUnitOfWork wuow(opCtx); auto coll = db->createCollection(opCtx, nss, options, createIdIndex, idIndexSpec); invariant(coll); - // This commit call can throw if a view already exists while registering the collection. + // This commit call can throw if a view already exists while registering the + // collection. wuow.commit(); - } catch (const AssertionException& ex) { - return ex.toStatus(); - } - - return Status::OK(); - }); + return Status::OK(); + }); + } catch (const DBException& ex) { + return ex.toStatus(); + } } Status StorageInterfaceImpl::createIndexesOnEmptyCollection( @@ -511,21 +509,25 @@ Status StorageInterfaceImpl::createIndexesOnEmptyCollection( } Status StorageInterfaceImpl::dropCollection(OperationContext* opCtx, const NamespaceString& nss) { - return writeConflictRetry(opCtx, "StorageInterfaceImpl::dropCollection", nss.ns(), [&] { - AutoGetDb autoDb(opCtx, nss.dbName(), MODE_IX); - Lock::CollectionLock collLock(opCtx, nss, MODE_X); - if (!autoDb.getDb()) { - // Database does not exist - nothing to do. + try { + return writeConflictRetry(opCtx, "StorageInterfaceImpl::dropCollection", nss.ns(), [&] { + AutoGetDb autoDb(opCtx, nss.dbName(), MODE_IX); + Lock::CollectionLock collLock(opCtx, nss, MODE_X); + if (!autoDb.getDb()) { + // Database does not exist - nothing to do. + return Status::OK(); + } + WriteUnitOfWork wunit(opCtx); + const auto status = autoDb.getDb()->dropCollectionEvenIfSystem(opCtx, nss); + if (!status.isOK()) { + return status; + } + wunit.commit(); return Status::OK(); - } - WriteUnitOfWork wunit(opCtx); - const auto status = autoDb.getDb()->dropCollectionEvenIfSystem(opCtx, nss); - if (!status.isOK()) { - return status; - } - wunit.commit(); - return Status::OK(); - }); + }); + } catch (const DBException& ex) { + return ex.toStatus(); + } } Status StorageInterfaceImpl::truncateCollection(OperationContext* opCtx, diff --git a/src/mongo/db/transaction/transaction_participant.cpp b/src/mongo/db/transaction/transaction_participant.cpp index 9357ac0b54b..b900979115c 100644 --- a/src/mongo/db/transaction/transaction_participant.cpp +++ b/src/mongo/db/transaction/transaction_participant.cpp @@ -1612,8 +1612,7 @@ Timestamp TransactionParticipant::Participant::prepareTransaction( // This shouldn't cause deadlocks with other prepared txns, because the acquisition // of RSTL lock inside abortTransaction will be no-op since we already have it. // This abortGuard gets dismissed before we release the RSTL while transitioning to - // prepared. - // TODO (SERVER-71610): Fix to be interruptible or document exception. + // the prepared state. UninterruptibleLockGuard noInterrupt(opCtx->lockState()); // NOLINT. abortTransaction(opCtx); } catch (...) { @@ -1899,8 +1898,8 @@ void TransactionParticipant::Participant::commitPreparedTransaction( // We can no longer uassert without terminating. unlockGuard.dismiss(); - // Once entering "committing with prepare" we cannot throw an exception. - // TODO (SERVER-71610): Fix to be interruptible or document exception. + // Once entering "committing with prepare" we cannot throw an exception, + // and therefore our lock acquisitions cannot be interruptible. UninterruptibleLockGuard noInterrupt(opCtx->lockState()); // NOLINT. // On secondary, we generate a fake empty oplog slot, since it's not used by opObserver. |