From 6d34f15ca68a6c3e7ec67e5c75cce4d6bbf51aee Mon Sep 17 00:00:00 2001 From: Kaloian Manassiev Date: Tue, 16 May 2023 16:41:18 +0000 Subject: SERVER-77123 Make the readConcern for TransactionResources come from prerequisites --- src/mongo/db/bulk_write_shard_test.cpp | 1 + src/mongo/db/commands/drop_indexes_cmd.cpp | 2 +- src/mongo/db/repl/read_concern_args.cpp | 10 ++--- src/mongo/db/shard_role.cpp | 59 ++++++++++++++++-------------- src/mongo/db/shard_role.h | 3 +- src/mongo/db/shard_role_test.cpp | 1 + src/mongo/db/transaction_resources.cpp | 55 +++++++++++++++++++++++----- src/mongo/db/transaction_resources.h | 28 ++++++-------- 8 files changed, 96 insertions(+), 63 deletions(-) diff --git a/src/mongo/db/bulk_write_shard_test.cpp b/src/mongo/db/bulk_write_shard_test.cpp index c0ef402a428..c8278938d92 100644 --- a/src/mongo/db/bulk_write_shard_test.cpp +++ b/src/mongo/db/bulk_write_shard_test.cpp @@ -30,6 +30,7 @@ #include "mongo/db/catalog/collection_uuid_mismatch_info.h" #include "mongo/db/catalog/create_collection.h" #include "mongo/db/catalog/database_holder.h" +#include "mongo/db/catalog_raii.h" #include "mongo/db/commands/bulk_write.h" #include "mongo/db/commands/bulk_write_gen.h" #include "mongo/db/dbdirectclient.h" diff --git a/src/mongo/db/commands/drop_indexes_cmd.cpp b/src/mongo/db/commands/drop_indexes_cmd.cpp index 57611e1fb33..818a8b9de34 100644 --- a/src/mongo/db/commands/drop_indexes_cmd.cpp +++ b/src/mongo/db/commands/drop_indexes_cmd.cpp @@ -31,7 +31,6 @@ #include #include "mongo/db/auth/authorization_session.h" -#include "mongo/db/catalog/collection.h" #include "mongo/db/catalog/collection_catalog.h" #include "mongo/db/catalog/database.h" #include "mongo/db/catalog/drop_indexes.h" @@ -42,6 +41,7 @@ #include "mongo/db/commands.h" #include "mongo/db/concurrency/exception_util.h" #include "mongo/db/curop.h" +#include "mongo/db/db_raii.h" #include "mongo/db/drop_indexes_gen.h" #include "mongo/db/index/index_descriptor.h" #include "mongo/db/index_builds_coordinator.h" diff --git a/src/mongo/db/repl/read_concern_args.cpp b/src/mongo/db/repl/read_concern_args.cpp index 5906aad2b51..080a2ce5ff1 100644 --- a/src/mongo/db/repl/read_concern_args.cpp +++ b/src/mongo/db/repl/read_concern_args.cpp @@ -27,9 +27,6 @@ * it in the license file. */ - -#include "mongo/platform/basic.h" - #include "mongo/db/repl/read_concern_args.h" #include "mongo/bson/util/bson_extract.h" @@ -41,15 +38,15 @@ #define MONGO_LOGV2_DEFAULT_COMPONENT ::mongo::logv2::LogComponent::kReplication - -using std::string; - namespace mongo { namespace repl { +namespace { const OperationContext::Decoration handle = OperationContext::declareDecoration(); +} // namespace + ReadConcernArgs& ReadConcernArgs::get(OperationContext* opCtx) { return handle(opCtx); } @@ -58,7 +55,6 @@ const ReadConcernArgs& ReadConcernArgs::get(const OperationContext* opCtx) { return handle(opCtx); } - // The "kImplicitDefault" read concern, used by internal operations, is deliberately empty (no // 'level' specified). This allows internal operations to specify a read concern, while still // allowing it to be either local or available on sharded secondaries. diff --git a/src/mongo/db/shard_role.cpp b/src/mongo/db/shard_role.cpp index 0041ebe9355..4e5707931e3 100644 --- a/src/mongo/db/shard_role.cpp +++ b/src/mongo/db/shard_role.cpp @@ -60,11 +60,9 @@ auto getTransactionResources = OperationContext::declareDecoration< std::unique_ptr>(); shard_role_details::TransactionResources& getOrMakeTransactionResources(OperationContext* opCtx) { - auto& readConcern = repl::ReadConcernArgs::get(opCtx); auto& optTransactionResources = getTransactionResources(opCtx); if (!optTransactionResources) { - optTransactionResources = - std::make_unique(readConcern); + optTransactionResources = std::make_unique(); } return *optTransactionResources; @@ -102,8 +100,12 @@ ResolvedNamespaceOrViewAcquisitionRequestsMap resolveNamespaceOrViewAcquisitionR auto coll = catalog.lookupCollectionByNamespace(opCtx, *ar.nss); checkCollectionUUIDMismatch(opCtx, *ar.nss, coll, ar.uuid); - AcquisitionPrerequisites prerequisites( - *ar.nss, ar.uuid, ar.placementConcern, ar.operationType, ar.viewMode); + AcquisitionPrerequisites prerequisites(*ar.nss, + ar.uuid, + ar.readConcern, + ar.placementConcern, + ar.operationType, + ar.viewMode); ResolvedNamespaceOrViewAcquisitionRequest resolvedAcquisitionRequest{ prerequisites, nullptr, boost::none}; @@ -131,8 +133,12 @@ ResolvedNamespaceOrViewAcquisitionRequestsMap resolveNamespaceOrViewAcquisitionR checkCollectionUUIDMismatch(opCtx, *ar.nss, coll, *ar.uuid); } - AcquisitionPrerequisites prerequisites( - coll->ns(), coll->uuid(), ar.placementConcern, ar.operationType, ar.viewMode); + AcquisitionPrerequisites prerequisites(coll->ns(), + coll->uuid(), + ar.readConcern, + ar.placementConcern, + ar.operationType, + ar.viewMode); ResolvedNamespaceOrViewAcquisitionRequest resolvedAcquisitionRequest{ prerequisites, nullptr, boost::none}; @@ -640,7 +646,7 @@ ResolvedNamespaceOrViewAcquisitionRequestsMap generateSortedAcquisitionRequests( const auto& nss = ar.nss ? *ar.nss : coll->ns(); AcquisitionPrerequisites prerequisites( - nss, ar.uuid, ar.placementConcern, ar.operationType, ar.viewMode); + nss, ar.uuid, ar.readConcern, ar.placementConcern, ar.operationType, ar.viewMode); ResolvedNamespaceOrViewAcquisitionRequest resolvedAcquisitionRequest{ prerequisites, nullptr, boost::none}; @@ -811,6 +817,7 @@ ScopedCollectionAcquisition acquireCollectionForLocalCatalogOnlyWithPotentialDat auto prerequisites = AcquisitionPrerequisites(nss, boost::none, + repl::ReadConcernArgs::get(opCtx), AcquisitionPrerequisites::kLocalCatalogOnlyWithPotentialDataLoss, AcquisitionPrerequisites::OperationType::kWrite, AcquisitionPrerequisites::ViewMode::kMustBeCollection); @@ -819,18 +826,16 @@ ScopedCollectionAcquisition acquireCollectionForLocalCatalogOnlyWithPotentialDat invariant(std::holds_alternative(collOrView)); auto& coll = std::get(collOrView); + if (coll) + prerequisites.uuid = boost::optional(coll->uuid()); - shard_role_details::AcquiredCollection& acquiredCollection = txnResources.addAcquiredCollection( - {AcquisitionPrerequisites(nss, - coll ? boost::optional(coll->uuid()) : boost::none, - AcquisitionPrerequisites::kLocalCatalogOnlyWithPotentialDataLoss, - AcquisitionPrerequisites::OperationType::kWrite, - AcquisitionPrerequisites::ViewMode::kMustBeCollection), - std::move(dbLock), - std::move(collLock), - boost::none, - boost::none, - std::move(coll)}); + shard_role_details::AcquiredCollection& acquiredCollection = + txnResources.addAcquiredCollection({prerequisites, + std::move(dbLock), + std::move(collLock), + boost::none, + boost::none, + std::move(coll)}); return ScopedCollectionAcquisition(opCtx, acquiredCollection); } @@ -925,11 +930,11 @@ boost::optional yieldTransactionResourcesFromOperat "Yielding view acquisitions is forbidden", transactionResources->acquiredViews.empty()); - invariant(!transactionResources->lockSnapshot); + invariant(!transactionResources->yieldedLocker); Locker::LockSnapshot lockSnapshot; opCtx->lockState()->saveLockStateAndUnlock(&lockSnapshot); - transactionResources->lockSnapshot.emplace(std::move(lockSnapshot)); + transactionResources->yieldedLocker.emplace(std::move(lockSnapshot)); transactionResources->yielded = true; return YieldedTransactionResources(std::move(transactionResources)); @@ -950,10 +955,10 @@ void restoreTransactionResourcesToOperationContext(OperationContext* opCtx, auto restoreFn = [&]() { // Reacquire locks. - if (yieldedResources._yieldedResources->lockSnapshot) { - opCtx->lockState()->restoreLockState(opCtx, - *yieldedResources._yieldedResources->lockSnapshot); - yieldedResources._yieldedResources->lockSnapshot.reset(); + if (yieldedResources._yieldedResources->yieldedLocker) { + opCtx->lockState()->restoreLockState( + opCtx, *yieldedResources._yieldedResources->yieldedLocker); + yieldedResources._yieldedResources->yieldedLocker.reset(); } // Reestablish a consistent catalog snapshot (multi document transactions don't yield). @@ -1036,10 +1041,10 @@ void restoreTransactionResourcesToOperationContext(OperationContext* opCtx, // the point we had left. We do this to prevent large multi-writes from // repeatedly failing due to StaleConfig and exhausting the mongos retry // attempts. Yield the locks. - yieldedResources._yieldedResources->lockSnapshot.emplace(); + yieldedResources._yieldedResources->yieldedLocker.emplace(); opCtx->recoveryUnit()->abandonSnapshot(); opCtx->lockState()->saveLockStateAndUnlock( - &(*yieldedResources._yieldedResources->lockSnapshot)); + yieldedResources._yieldedResources->yieldedLocker.get_ptr()); // Wait for the critical section to finish. OperationShardingState::waitForCriticalSectionToComplete( opCtx, *ex->getCriticalSectionSignal()) diff --git a/src/mongo/db/shard_role.h b/src/mongo/db/shard_role.h index 866fd91ebe2..2d778d3049c 100644 --- a/src/mongo/db/shard_role.h +++ b/src/mongo/db/shard_role.h @@ -29,8 +29,7 @@ #pragma once -#include "mongo/db/catalog_raii.h" -#include "mongo/db/db_raii.h" +#include "mongo/db/catalog/collection_catalog.h" #include "mongo/db/repl/read_concern_args.h" #include "mongo/db/s/scoped_collection_metadata.h" #include "mongo/db/transaction_resources.h" diff --git a/src/mongo/db/shard_role_test.cpp b/src/mongo/db/shard_role_test.cpp index 0a3d3fe93c1..88cdf8494c2 100644 --- a/src/mongo/db/shard_role_test.cpp +++ b/src/mongo/db/shard_role_test.cpp @@ -30,6 +30,7 @@ #include "mongo/db/catalog/collection_uuid_mismatch_info.h" #include "mongo/db/catalog/create_collection.h" #include "mongo/db/catalog/database_holder.h" +#include "mongo/db/catalog_raii.h" #include "mongo/db/dbdirectclient.h" #include "mongo/db/repl/replication_coordinator_mock.h" #include "mongo/db/s/collection_sharding_runtime.h" diff --git a/src/mongo/db/transaction_resources.cpp b/src/mongo/db/transaction_resources.cpp index 4c286f76c63..24472bfbf12 100644 --- a/src/mongo/db/transaction_resources.cpp +++ b/src/mongo/db/transaction_resources.cpp @@ -35,13 +35,55 @@ const PlacementConcern AcquisitionPrerequisites::kPretendUnsharded = PlacementConcern{boost::none, boost::none}; namespace shard_role_details { +namespace { -TransactionResources::TransactionResources(repl::ReadConcernArgs readConcern) - : readConcern(std::move(readConcern)) {} +/** + * This method ensures that two read concerns are equivalent for the purposes of acquiring a + * transactional snapshot. Equivalence means that they don't acquire snapshot at conflicting levels, + * such as one operation asking for local and a subsequent one for majority. Similarly, we can't + * have two subsequent acquisitions asking for snapshots at two different timestamps. + */ +void assertReadConcernsAreEquivalent(const repl::ReadConcernArgs& rc1, + const repl::ReadConcernArgs& rc2) { + tassert(771230, + str::stream() << "Acquired two different collections on the same transaction with " + "read concerns that are not equivalent (" + << rc1.toString() << " != " << rc2.toString() << ")", + rc1.getLevel() == rc2.getLevel() && + rc1.getArgsAtClusterTime() == rc2.getArgsAtClusterTime()); +} + +} // namespace + +TransactionResources::TransactionResources() = default; + +TransactionResources::~TransactionResources() { + invariant(!locker); + invariant(!yieldedLocker); + invariant(!yieldedRecoveryUnit); + invariant(acquiredCollections.empty()); + invariant(acquiredViews.empty()); +} + +AcquiredCollection& TransactionResources::addAcquiredCollection( + AcquiredCollection&& acquiredCollection) { + if (!readConcern) { + readConcern = acquiredCollection.prerequisites.readConcern; + } + assertReadConcernsAreEquivalent(*readConcern, acquiredCollection.prerequisites.readConcern); + + return acquiredCollections.emplace_back(std::move(acquiredCollection)); +} + +const AcquiredView& TransactionResources::addAcquiredView(AcquiredView&& acquiredView) { + return acquiredViews.emplace_back(std::move(acquiredView)); +} void TransactionResources::releaseAllResourcesOnCommitOrAbort() noexcept { + readConcern.reset(); locker.reset(); - lockSnapshot.reset(); + yieldedLocker.reset(); + yieldedRecoveryUnit.reset(); acquiredCollections.clear(); acquiredViews.clear(); } @@ -57,12 +99,5 @@ void TransactionResources::assertNoAcquiredCollections() const { fassertFailedWithStatus(737660, Status{ErrorCodes::InternalError, ss.str()}); } -TransactionResources::~TransactionResources() { - invariant(!locker); - invariant(!lockSnapshot); - invariant(acquiredCollections.empty()); - invariant(acquiredViews.empty()); -} - } // namespace shard_role_details } // namespace mongo diff --git a/src/mongo/db/transaction_resources.h b/src/mongo/db/transaction_resources.h index 951e7c43d9a..c6c5e923ea1 100644 --- a/src/mongo/db/transaction_resources.h +++ b/src/mongo/db/transaction_resources.h @@ -76,11 +76,13 @@ struct AcquisitionPrerequisites { AcquisitionPrerequisites(NamespaceString nss, boost::optional uuid, + repl::ReadConcernArgs readConcern, PlacementConcernVariant placementConcern, OperationType operationType, ViewMode viewMode) : nss(std::move(nss)), uuid(std::move(uuid)), + readConcern(std::move(readConcern)), placementConcern(std::move(placementConcern)), operationType(operationType), viewMode(viewMode) {} @@ -88,6 +90,7 @@ struct AcquisitionPrerequisites { NamespaceString nss; boost::optional uuid; + repl::ReadConcernArgs readConcern; PlacementConcernVariant placementConcern; OperationType operationType; ViewMode viewMode; @@ -180,7 +183,7 @@ struct AcquiredView { * the read concern of the operation). */ struct TransactionResources { - TransactionResources(repl::ReadConcernArgs readConcern); + TransactionResources(); TransactionResources(TransactionResources&&) = delete; TransactionResources& operator=(TransactionResources&&) = delete; @@ -190,15 +193,8 @@ struct TransactionResources { ~TransactionResources(); - AcquiredCollection& addAcquiredCollection(AcquiredCollection&& acquiredCollection) { - return acquiredCollections.emplace_back(std::move(acquiredCollection)); - } - - void releaseCollection(UUID uuid); - - const AcquiredView& addAcquiredView(AcquiredView&& acquiredView) { - return acquiredViews.emplace_back(std::move(acquiredView)); - } + AcquiredCollection& addAcquiredCollection(AcquiredCollection&& acquiredCollection); + const AcquiredView& addAcquiredView(AcquiredView&& acquiredView); void releaseAllResourcesOnCommitOrAbort() noexcept; @@ -209,22 +205,22 @@ struct TransactionResources { */ void assertNoAcquiredCollections() const; - // The read concern with which the whole operation started. Remains the same for the duration of - // the entire operation. - repl::ReadConcernArgs readConcern; - // Indicates whether yield has been performed on these resources bool yielded{false}; //////////////////////////////////////////////////////////////////////////////////////// // Global resources (cover all collections for the operation) + // The read concern with which the transaction runs. All acquisitions must match that read + // concern. + boost::optional readConcern; + // Set of locks acquired by the operation or nullptr if yielded. std::unique_ptr locker; // If '_locker' has been yielded, contains a snapshot of the locks which have been yielded. // Otherwise boost::none. - boost::optional lockSnapshot; + boost::optional yieldedLocker; // The storage engine snapshot associated with this transaction (when yielded). struct YieldedRecoveryUnit { @@ -232,7 +228,7 @@ struct TransactionResources { WriteUnitOfWork::RecoveryUnitState recoveryUnitState; }; - boost::optional yieldRecoveryUnit; + boost::optional yieldedRecoveryUnit; //////////////////////////////////////////////////////////////////////////////////////// // Per-collection resources -- cgit v1.2.1