diff options
23 files changed, 253 insertions, 283 deletions
diff --git a/src/mongo/db/catalog/catalog_control.cpp b/src/mongo/db/catalog/catalog_control.cpp index f4c8cd93c53..3e94b7db845 100644 --- a/src/mongo/db/catalog/catalog_control.cpp +++ b/src/mongo/db/catalog/catalog_control.cpp @@ -147,7 +147,8 @@ PreviousCatalogState closeCatalog(OperationContext* opCtx) { auto databaseHolder = DatabaseHolder::get(opCtx); auto catalog = CollectionCatalog::get(opCtx); for (auto&& dbName : allDbs) { - for (auto&& coll : catalog->range(dbName)) { + for (auto collIt = catalog->begin(opCtx, dbName); collIt != catalog->end(opCtx); ++collIt) { + auto coll = *collIt; if (!coll) { break; } diff --git a/src/mongo/db/catalog/collection_catalog.cpp b/src/mongo/db/catalog/collection_catalog.cpp index 26036969b0f..cba44c2ac04 100644 --- a/src/mongo/db/catalog/collection_catalog.cpp +++ b/src/mongo/db/catalog/collection_catalog.cpp @@ -64,7 +64,6 @@ const ServiceContext::Decoration<LatestCollectionCatalog> getCatalog = // batched write is ongoing without having to take locks. std::shared_ptr<CollectionCatalog> batchedCatalogWriteInstance; AtomicWord<bool> ongoingBatchedWrite{false}; -absl::flat_hash_set<Collection*> batchedCatalogClonedCollections; const RecoveryUnit::Snapshot::Decoration<std::shared_ptr<const CollectionCatalog>> stashedCatalog = RecoveryUnit::Snapshot::declareDecoration<std::shared_ptr<const CollectionCatalog>>(); @@ -159,7 +158,7 @@ public: catalog._collections = catalog._collections.set(collection->ns(), collection); catalog._catalog = catalog._catalog.set(collection->uuid(), collection); auto dbIdPair = std::make_pair(collection->ns().dbName(), collection->uuid()); - catalog._orderedCollections = catalog._orderedCollections.set(dbIdPair, collection); + catalog._orderedCollections[dbIdPair] = collection; catalog._pendingCommitNamespaces = catalog._pendingCommitNamespaces.erase(collection->ns()); catalog._pendingCommitUUIDs = catalog._pendingCommitUUIDs.erase(collection->uuid()); @@ -261,6 +260,7 @@ public: commitTime](CollectionCatalog& catalog) { // Override existing Collection on this namespace catalog._registerCollection(opCtx, + uuid, std::move(collection), /*twoPhase=*/false, /*ts=*/commitTime); @@ -370,64 +370,83 @@ private: UncommittedCatalogUpdates& _uncommittedCatalogUpdates; }; -CollectionCatalog::iterator::iterator(const DatabaseName& dbName, - OrderedCollectionMap::iterator it, - const OrderedCollectionMap& map) - : _map{map}, _mapIter{it} { - _skipUncommitted(); +CollectionCatalog::iterator::iterator(OperationContext* opCtx, + const DatabaseName& dbName, + const CollectionCatalog& catalog) + : _opCtx(opCtx), _dbName(dbName), _catalog(&catalog) { + + _mapIter = _catalog->_orderedCollections.lower_bound(std::make_pair(_dbName, minUuid)); + + // Start with the first collection that is visible outside of its transaction. + while (!_exhausted() && !_mapIter->second->isCommitted()) { + _mapIter++; + } + + if (!_exhausted()) { + _uuid = _mapIter->first.second; + } } +CollectionCatalog::iterator::iterator( + OperationContext* opCtx, + std::map<std::pair<DatabaseName, UUID>, std::shared_ptr<Collection>>::const_iterator mapIter, + const CollectionCatalog& catalog) + : _opCtx(opCtx), _mapIter(mapIter), _catalog(&catalog) {} + CollectionCatalog::iterator::value_type CollectionCatalog::iterator::operator*() { - if (_mapIter == _map.end()) { + if (_exhausted()) { return nullptr; } + return _mapIter->second.get(); } +UUID CollectionCatalog::iterator::uuid() const { + invariant(_uuid); + return *_uuid; +} + CollectionCatalog::iterator CollectionCatalog::iterator::operator++() { - invariant(_mapIter != _map.end()); _mapIter++; - _skipUncommitted(); - return *this; -} -bool CollectionCatalog::iterator::operator==(const iterator& other) const { - invariant(_map == other._map); + // Skip any collections that are not yet visible outside of their respective transactions. + while (!_exhausted() && !_mapIter->second->isCommitted()) { + _mapIter++; + } - if (other._mapIter == other._map.end()) { - return _mapIter == _map.end(); - } else if (_mapIter == _map.end()) { - return other._mapIter == other._map.end(); + if (_exhausted()) { + // If the iterator is at the end of the map or now points to an entry that does not + // correspond to the correct database. + _mapIter = _catalog->_orderedCollections.end(); + _uuid = boost::none; + return *this; } - return _mapIter->first.second == other._mapIter->first.second; + _uuid = _mapIter->first.second; + return *this; } -bool CollectionCatalog::iterator::operator!=(const iterator& other) const { - return !(*this == other); +CollectionCatalog::iterator CollectionCatalog::iterator::operator++(int) { + auto oldPosition = *this; + ++(*this); + return oldPosition; } -void CollectionCatalog::iterator::_skipUncommitted() { - // Advance to the next collection that is visible outside of its transaction. - auto end = _map.end(); - while (_mapIter != end && !_mapIter->second->isCommitted()) { - ++_mapIter; +bool CollectionCatalog::iterator::operator==(const iterator& other) const { + invariant(_catalog == other._catalog); + if (other._mapIter == _catalog->_orderedCollections.end()) { + return _uuid == boost::none; } -} - -CollectionCatalog::Range::Range(const OrderedCollectionMap& map, const DatabaseName& dbName) - : _map{map}, _dbName{dbName} {} -CollectionCatalog::iterator CollectionCatalog::Range::begin() const { - return {_dbName, _map.lower_bound(std::make_pair(_dbName, minUuid)), _map}; + return _uuid == other._uuid; } -CollectionCatalog::iterator CollectionCatalog::Range::end() const { - return {_dbName, _map.upper_bound(std::make_pair(_dbName, maxUuid)), _map}; +bool CollectionCatalog::iterator::operator!=(const iterator& other) const { + return !(*this == other); } -bool CollectionCatalog::Range::empty() const { - return begin() == end(); +bool CollectionCatalog::iterator::_exhausted() { + return _mapIter == _catalog->_orderedCollections.end() || _mapIter->first.first != _dbName; } std::shared_ptr<const CollectionCatalog> CollectionCatalog::latest(ServiceContext* svcCtx) { @@ -1321,10 +1340,6 @@ uint64_t CollectionCatalog::getEpoch() const { return _epoch; } -CollectionCatalog::Range CollectionCatalog::range(const DatabaseName& dbName) const { - return {_orderedCollections, dbName}; -} - std::shared_ptr<const Collection> CollectionCatalog::_getCollectionByUUID(OperationContext* opCtx, const UUID& uuid) const { // It's important to look in UncommittedCatalogUpdates before OpenedCollections because in a @@ -1387,7 +1402,6 @@ Collection* CollectionCatalog::lookupCollectionByUUIDForMetadataWrite(OperationC // on the thread doing the batch write and it would trigger the regular path where we do a // copy-on-write on the catalog when committing. if (_isCatalogBatchWriter()) { - batchedCatalogClonedCollections.emplace(cloned.get()); // Do not update min valid timestamp in batched write as the write is not corresponding to // an oplog entry. If the write require an update to this timestamp it is the responsibility // of the user. @@ -1516,7 +1530,6 @@ Collection* CollectionCatalog::lookupCollectionByNamespaceForMetadataWrite( // on the thread doing the batch write and it would trigger the regular path where we do a // copy-on-write on the catalog when committing. if (_isCatalogBatchWriter()) { - batchedCatalogClonedCollections.emplace(cloned.get()); // Do not update min valid timestamp in batched write as the write is not corresponding to // an oplog entry. If the write require an update to this timestamp it is the responsibility // of the user. @@ -1885,24 +1898,26 @@ CollectionCatalog::ViewCatalogSet CollectionCatalog::getViewCatalogDbNames( } void CollectionCatalog::registerCollection(OperationContext* opCtx, + const UUID& uuid, std::shared_ptr<Collection> coll, boost::optional<Timestamp> commitTime) { invariant(opCtx->lockState()->isW()); - _registerCollection(opCtx, std::move(coll), /*twoPhase=*/false, commitTime); + _registerCollection(opCtx, uuid, std::move(coll), /*twoPhase=*/false, commitTime); } void CollectionCatalog::registerCollectionTwoPhase(OperationContext* opCtx, + const UUID& uuid, std::shared_ptr<Collection> coll, boost::optional<Timestamp> commitTime) { - _registerCollection(opCtx, std::move(coll), /*twoPhase=*/true, commitTime); + _registerCollection(opCtx, uuid, std::move(coll), /*twoPhase=*/true, commitTime); } void CollectionCatalog::_registerCollection(OperationContext* opCtx, + const UUID& uuid, std::shared_ptr<Collection> coll, bool twoPhase, boost::optional<Timestamp> commitTime) { auto nss = coll->ns(); - auto uuid = coll->uuid(); _ensureNamespaceDoesNotExist(opCtx, nss, NamespaceType::kAll); LOGV2_DEBUG(20280, @@ -1920,7 +1935,7 @@ void CollectionCatalog::_registerCollection(OperationContext* opCtx, _catalog = _catalog.set(uuid, coll); _collections = _collections.set(nss, coll); - _orderedCollections = _orderedCollections.set(dbIdPair, coll); + _orderedCollections[dbIdPair] = coll; if (twoPhase) { _pendingCommitNamespaces = _pendingCommitNamespaces.set(nss, coll); _pendingCommitUUIDs = _pendingCommitUUIDs.set(uuid, coll); @@ -2000,7 +2015,7 @@ std::shared_ptr<Collection> CollectionCatalog::deregisterCollection( } } - _orderedCollections = _orderedCollections.erase(dbIdPair); + _orderedCollections.erase(dbIdPair); _collections = _collections.erase(ns); _catalog = _catalog.erase(uuid); _pendingCommitNamespaces = _pendingCommitNamespaces.erase(ns); @@ -2099,7 +2114,7 @@ void CollectionCatalog::deregisterAllCollectionsAndViews(ServiceContext* svcCtx) } _collections = {}; - _orderedCollections = {}; + _orderedCollections.clear(); _catalog = {}; _viewsForDatabase = {}; _dropPendingCollection = {}; @@ -2151,6 +2166,15 @@ void CollectionCatalog::notifyIdentDropped(const std::string& ident) { _dropPendingIndex = _dropPendingIndex.erase(ident); } +CollectionCatalog::iterator CollectionCatalog::begin(OperationContext* opCtx, + const DatabaseName& dbName) const { + return iterator(opCtx, dbName, *this); +} + +CollectionCatalog::iterator CollectionCatalog::end(OperationContext* opCtx) const { + return iterator(opCtx, _orderedCollections.end(), *this); +} + void CollectionCatalog::invariantHasExclusiveAccessToCollection(OperationContext* opCtx, const NamespaceString& nss) { invariant(hasExclusiveAccessToCollection(opCtx, nss), nss.toStringForErrorMsg()); @@ -2202,16 +2226,22 @@ bool CollectionCatalog::_isCatalogBatchWriter() const { bool CollectionCatalog::_alreadyClonedForBatchedWriter( const std::shared_ptr<Collection>& collection) const { - // We may skip cloning the Collection instance if and only if have already cloned it for write - // use in this batch writer. - return _isCatalogBatchWriter() && batchedCatalogClonedCollections.contains(collection.get()); + // We may skip cloning the Collection instance if and only if we are currently in a batched + // catalog write and all references to this Collection is owned by the cloned CollectionCatalog + // instance owned by the batch writer. i.e. the Collection is uniquely owned by the batch + // writer. When the batch writer initially clones the catalog, all collections will have a + // 'use_count' of at least kNumCollectionReferencesStored*2 (because there are at least 2 + // catalog instances). To check for uniquely owned we need to check that the reference count is + // exactly kNumCollectionReferencesStored (owned by a single catalog) while also account for the + // instance that is extracted from the catalog and provided as a parameter to this function, we + // therefore need to add 1. + return _isCatalogBatchWriter() && collection.use_count() == kNumCollectionReferencesStored + 1; } BatchedCollectionCatalogWriter::BatchedCollectionCatalogWriter(OperationContext* opCtx) : _opCtx(opCtx) { invariant(_opCtx->lockState()->isW()); invariant(!batchedCatalogWriteInstance); - invariant(batchedCatalogClonedCollections.empty()); auto& storage = getCatalog(_opCtx->getServiceContext()); // hold onto base so if we need to delete it we can do it outside of the lock @@ -2236,7 +2266,6 @@ BatchedCollectionCatalogWriter::~BatchedCollectionCatalogWriter() { ongoingBatchedWrite.store(false); _batchedInstance = nullptr; batchedCatalogWriteInstance = nullptr; - batchedCatalogClonedCollections.clear(); } } // namespace mongo diff --git a/src/mongo/db/catalog/collection_catalog.h b/src/mongo/db/catalog/collection_catalog.h index dfd4bb311e2..a5086bba80d 100644 --- a/src/mongo/db/catalog/collection_catalog.h +++ b/src/mongo/db/catalog/collection_catalog.h @@ -43,7 +43,6 @@ #include "mongo/db/views/view.h" #include "mongo/stdx/unordered_map.h" #include "mongo/util/functional.h" -#include "mongo/util/immutable/map.h" #include "mongo/util/immutable/unordered_map.h" #include "mongo/util/immutable/unordered_set.h" #include "mongo/util/uuid.h" @@ -52,43 +51,47 @@ namespace mongo { class CollectionCatalog { friend class iterator; - using OrderedCollectionMap = - immutable::map<std::pair<DatabaseName, UUID>, std::shared_ptr<Collection>>; public: using CollectionInfoFn = std::function<bool(const Collection* collection)>; + // Number of how many Collection references for a single Collection that is stored in the + // catalog. Used to determine whether there are external references (uniquely owned). Needs to + // be kept in sync with the data structures below. + static constexpr size_t kNumCollectionReferencesStored = 3; class iterator { public: using value_type = const Collection*; - iterator(const DatabaseName& dbName, - OrderedCollectionMap::iterator it, - const OrderedCollectionMap& catalog); + iterator(OperationContext* opCtx, + const DatabaseName& dbName, + const CollectionCatalog& catalog); + iterator(OperationContext* opCtx, + std::map<std::pair<DatabaseName, UUID>, + std::shared_ptr<Collection>>::const_iterator mapIter, + const CollectionCatalog& catalog); value_type operator*(); iterator operator++(); + iterator operator++(int); + UUID uuid() const; + + /* + * Equality operators == and != do not attempt to reposition the iterators being compared. + * The behavior for comparing invalid iterators is undefined. + */ bool operator==(const iterator& other) const; bool operator!=(const iterator& other) const; private: - void _skipUncommitted(); - - const OrderedCollectionMap& _map; - immutable::map<std::pair<DatabaseName, UUID>, std::shared_ptr<Collection>>::iterator - _mapIter; - }; - - class Range { - public: - Range(const OrderedCollectionMap&, const DatabaseName& dbName); - iterator begin() const; - iterator end() const; - bool empty() const; + bool _exhausted(); - private: - OrderedCollectionMap _map; + OperationContext* _opCtx; DatabaseName _dbName; + boost::optional<UUID> _uuid; + std::map<std::pair<DatabaseName, UUID>, std::shared_ptr<Collection>>::const_iterator + _mapIter; + const CollectionCatalog* _catalog; }; struct ProfileSettings { @@ -305,6 +308,7 @@ public: * The global lock must be held in exclusive mode. */ void registerCollection(OperationContext* opCtx, + const UUID& uuid, std::shared_ptr<Collection> collection, boost::optional<Timestamp> commitTime); @@ -315,6 +319,7 @@ public: * 'onCreateCollection' which sets up the necessary state for finishing the two-phase commit. */ void registerCollectionTwoPhase(OperationContext* opCtx, + const UUID& uuid, std::shared_ptr<Collection> collection, boost::optional<Timestamp> commitTime); @@ -627,13 +632,8 @@ public: */ uint64_t getEpoch() const; - /** - * Provides an iterable range for the collections belonging to the specified database. - * - * Will not observe any updates made to the catalog after the creation of the 'Range'. The - * 'Range' object just remain in scope for the duration of the iteration. - */ - Range range(const DatabaseName& dbName) const; + iterator begin(OperationContext* opCtx, const DatabaseName& dbName) const; + iterator end(OperationContext* opCtx) const; /** * Ensures we have a MODE_X lock on a collection or MODE_IX lock for newly created collections. @@ -663,12 +663,13 @@ private: const UUID& uuid) const; /** - * Register the collection. + * Register the collection with `uuid`. * * If 'twoPhase' is true, this call must be followed by 'onCreateCollection' which continues the * two-phase commit process. */ void _registerCollection(OperationContext* opCtx, + const UUID& uuid, std::shared_ptr<Collection> collection, bool twoPhase, boost::optional<Timestamp> commitTime); @@ -797,6 +798,8 @@ private: using CollectionCatalogMap = immutable::unordered_map<UUID, std::shared_ptr<Collection>, UUID::Hash>; + using OrderedCollectionMap = + std::map<std::pair<DatabaseName, UUID>, std::shared_ptr<Collection>>; using NamespaceCollectionMap = immutable::unordered_map<NamespaceString, std::shared_ptr<Collection>>; using UncommittedViewsSet = immutable::unordered_set<NamespaceString>; diff --git a/src/mongo/db/catalog/collection_catalog_bm.cpp b/src/mongo/db/catalog/collection_catalog_bm.cpp index 57898636095..a78b9d1e3d6 100644 --- a/src/mongo/db/catalog/collection_catalog_bm.cpp +++ b/src/mongo/db/catalog/collection_catalog_bm.cpp @@ -80,6 +80,7 @@ void createCollections(OperationContext* opCtx, int numCollections) { const NamespaceString nss("collection_catalog_bm", std::to_string(i)); CollectionCatalog::write(opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection(opCtx, + UUID::gen(), std::make_shared<CollectionMock>(nss), /*ts=*/boost::none); }); @@ -123,127 +124,7 @@ void BM_CollectionCatalogWriteBatchedWithGlobalExclusiveLock(benchmark::State& s } } -void BM_CollectionCatalogCreateDropCollection(benchmark::State& state) { - auto serviceContext = setupServiceContext(); - ThreadClient threadClient(serviceContext); - ServiceContext::UniqueOperationContext opCtx = threadClient->makeOperationContext(); - Lock::GlobalLock globalLk(opCtx.get(), MODE_X); - - createCollections(opCtx.get(), state.range(0)); - - for (auto _ : state) { - benchmark::ClobberMemory(); - CollectionCatalog::write(opCtx.get(), [&](CollectionCatalog& catalog) { - const NamespaceString nss("collection_catalog_bm", std::to_string(state.range(0))); - const UUID uuid = UUID::gen(); - catalog.registerCollection( - opCtx.get(), std::make_shared<CollectionMock>(uuid, nss), boost::none); - catalog.deregisterCollection(opCtx.get(), uuid, false, boost::none); - }); - } -} - -void BM_CollectionCatalogCreateNCollectionsBatched(benchmark::State& state) { - for (auto _ : state) { - benchmark::ClobberMemory(); - - auto serviceContext = setupServiceContext(); - ThreadClient threadClient(serviceContext); - ServiceContext::UniqueOperationContext opCtx = threadClient->makeOperationContext(); - - Lock::GlobalLock globalLk(opCtx.get(), MODE_X); - BatchedCollectionCatalogWriter batched(opCtx.get()); - - auto numCollections = state.range(0); - for (auto i = 0; i < numCollections; i++) { - const NamespaceString nss("collection_catalog_bm", std::to_string(i)); - CollectionCatalog::write(opCtx.get(), [&](CollectionCatalog& catalog) { - catalog.registerCollection( - opCtx.get(), std::make_shared<CollectionMock>(nss), boost::none); - }); - } - } -} - -void BM_CollectionCatalogCreateNCollections(benchmark::State& state) { - for (auto _ : state) { - benchmark::ClobberMemory(); - - auto serviceContext = setupServiceContext(); - ThreadClient threadClient(serviceContext); - ServiceContext::UniqueOperationContext opCtx = threadClient->makeOperationContext(); - Lock::GlobalLock globalLk(opCtx.get(), MODE_X); - - auto numCollections = state.range(0); - for (auto i = 0; i < numCollections; i++) { - const NamespaceString nss("collection_catalog_bm", std::to_string(i)); - CollectionCatalog::write(opCtx.get(), [&](CollectionCatalog& catalog) { - catalog.registerCollection( - opCtx.get(), std::make_shared<CollectionMock>(nss), boost::none); - }); - } - } -} - -void BM_CollectionCatalogLookupCollectionByNamespace(benchmark::State& state) { - auto serviceContext = setupServiceContext(); - ThreadClient threadClient(serviceContext); - ServiceContext::UniqueOperationContext opCtx = threadClient->makeOperationContext(); - - createCollections(opCtx.get(), state.range(0)); - const NamespaceString nss("collection_catalog_bm", std::to_string(state.range(0) / 2)); - - for (auto _ : state) { - benchmark::ClobberMemory(); - auto coll = - CollectionCatalog::get(opCtx.get())->lookupCollectionByNamespace(opCtx.get(), nss); - invariant(coll); - } -} - -void BM_CollectionCatalogLookupCollectionByUUID(benchmark::State& state) { - auto serviceContext = setupServiceContext(); - ThreadClient threadClient(serviceContext); - ServiceContext::UniqueOperationContext opCtx = threadClient->makeOperationContext(); - - createCollections(opCtx.get(), state.range(0)); - const NamespaceString nss("collection_catalog_bm", std::to_string(state.range(0) / 2)); - auto coll = CollectionCatalog::get(opCtx.get())->lookupCollectionByNamespace(opCtx.get(), nss); - invariant(coll->ns() == nss); - const UUID uuid = coll->uuid(); - - for (auto _ : state) { - benchmark::ClobberMemory(); - auto res = CollectionCatalog::get(opCtx.get())->lookupCollectionByUUID(opCtx.get(), uuid); - invariant(res == coll); - } -} - -void BM_CollectionCatalogIterateCollections(benchmark::State& state) { - auto serviceContext = setupServiceContext(); - ThreadClient threadClient(serviceContext); - ServiceContext::UniqueOperationContext opCtx = threadClient->makeOperationContext(); - - createCollections(opCtx.get(), state.range(0)); - - for (auto _ : state) { - benchmark::ClobberMemory(); - auto catalog = CollectionCatalog::get(opCtx.get()); - auto count = 0; - for ([[maybe_unused]] auto&& coll : catalog->range( - DatabaseName::createDatabaseName_forTest(boost::none, "collection_catalog_bm"))) { - benchmark::DoNotOptimize(count++); - } - } -} - BENCHMARK(BM_CollectionCatalogWrite)->Ranges({{{1}, {100'000}}}); BENCHMARK(BM_CollectionCatalogWriteBatchedWithGlobalExclusiveLock)->Ranges({{{1}, {100'000}}}); -BENCHMARK(BM_CollectionCatalogCreateDropCollection)->Ranges({{{1}, {100'000}}}); -BENCHMARK(BM_CollectionCatalogCreateNCollectionsBatched)->Ranges({{{1}, {100'000}}}); -BENCHMARK(BM_CollectionCatalogCreateNCollections)->Ranges({{{1}, {32'768}}}); -BENCHMARK(BM_CollectionCatalogLookupCollectionByNamespace)->Ranges({{{1}, {100'000}}}); -BENCHMARK(BM_CollectionCatalogLookupCollectionByUUID)->Ranges({{{1}, {100'000}}}); -BENCHMARK(BM_CollectionCatalogIterateCollections)->Ranges({{{1}, {100'000}}}); } // namespace mongo diff --git a/src/mongo/db/catalog/collection_catalog_helper.cpp b/src/mongo/db/catalog/collection_catalog_helper.cpp index c9dadb96b57..e74c8cd05b9 100644 --- a/src/mongo/db/catalog/collection_catalog_helper.cpp +++ b/src/mongo/db/catalog/collection_catalog_helper.cpp @@ -69,9 +69,11 @@ void forEachCollectionFromDb(OperationContext* opCtx, CollectionCatalog::CollectionInfoFn predicate) { auto catalogForIteration = CollectionCatalog::get(opCtx); - for (auto&& coll : catalogForIteration->range(dbName)) { - auto uuid = coll->uuid(); + for (auto collectionIt = catalogForIteration->begin(opCtx, dbName); + collectionIt != catalogForIteration->end(opCtx);) { + auto uuid = collectionIt.uuid(); if (predicate && !catalogForIteration->checkIfCollectionSatisfiable(uuid, predicate)) { + ++collectionIt; continue; } @@ -95,6 +97,10 @@ void forEachCollectionFromDb(OperationContext* opCtx, clk.reset(); } + // Increment iterator before calling callback. This allows for collection deletion inside + // this callback even if we are in batched inplace mode. + ++collectionIt; + // The NamespaceString couldn't be resolved from the uuid, so the collection was dropped. if (!collection) continue; diff --git a/src/mongo/db/catalog/collection_catalog_test.cpp b/src/mongo/db/catalog/collection_catalog_test.cpp index fc0ce7245d5..9eb767d1dec 100644 --- a/src/mongo/db/catalog/collection_catalog_test.cpp +++ b/src/mongo/db/catalog/collection_catalog_test.cpp @@ -78,7 +78,12 @@ public: std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(colUUID, nss); col = CollectionPtr(collection.get()); // Register dummy collection in catalog. - catalog.registerCollection(opCtx.get(), collection, boost::none); + catalog.registerCollection(opCtx.get(), colUUID, collection, boost::none); + + // Validate that kNumCollectionReferencesStored is correct, add one reference for the one we + // hold in this function. + ASSERT_EQUALS(collection.use_count(), + CollectionCatalog::kNumCollectionReferencesStored + 1); } void tearDown() { @@ -110,14 +115,17 @@ public: NamespaceString barNss = NamespaceString::createNamespaceString_forTest( "bar", "coll" + std::to_string(counter)); + auto fooUuid = UUID::gen(); std::shared_ptr<Collection> fooColl = std::make_shared<CollectionMock>(fooNss); + + auto barUuid = UUID::gen(); std::shared_ptr<Collection> barColl = std::make_shared<CollectionMock>(barNss); - dbMap["foo"].insert(std::make_pair(fooColl->uuid(), fooColl.get())); - dbMap["bar"].insert(std::make_pair(barColl->uuid(), barColl.get())); + dbMap["foo"].insert(std::make_pair(fooUuid, fooColl.get())); + dbMap["bar"].insert(std::make_pair(barUuid, barColl.get())); - catalog.registerCollection(opCtx.get(), fooColl, boost::none); - catalog.registerCollection(opCtx.get(), barColl, boost::none); + catalog.registerCollection(opCtx.get(), fooUuid, fooColl, boost::none); + catalog.registerCollection(opCtx.get(), barUuid, barColl, boost::none); } } @@ -146,10 +154,10 @@ public: void checkCollections(const DatabaseName& dbName) { unsigned long counter = 0; - auto orderedIt = collsIterator(dbName.toString()); - auto catalogRange = catalog.range(dbName); - auto catalogIt = catalogRange.begin(); - for (; catalogIt != catalogRange.end() && orderedIt != collsIteratorEnd(dbName.toString()); + for (auto [orderedIt, catalogIt] = + std::tuple{collsIterator(dbName.toString()), catalog.begin(opCtx.get(), dbName)}; + catalogIt != catalog.end(opCtx.get()) && + orderedIt != collsIteratorEnd(dbName.toString()); ++catalogIt, ++orderedIt) { auto catalogColl = *catalogIt; @@ -184,13 +192,17 @@ public: NamespaceString nss = NamespaceString::createNamespaceString_forTest( "resourceDb", "coll" + std::to_string(i)); std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); + auto uuid = collection->uuid(); - catalog.registerCollection(opCtx.get(), std::move(collection), boost::none); + catalog.registerCollection(opCtx.get(), uuid, std::move(collection), boost::none); } int numEntries = 0; - for (auto&& coll : - catalog.range(DatabaseName::createDatabaseName_forTest(boost::none, "resourceDb"))) { + for (auto it = catalog.begin( + opCtx.get(), DatabaseName::createDatabaseName_forTest(boost::none, "resourceDb")); + it != catalog.end(opCtx.get()); + it++) { + auto coll = *it; auto collName = coll->ns(); ResourceId rid(RESOURCE_COLLECTION, collName); @@ -202,8 +214,11 @@ public: void tearDown() { std::vector<UUID> collectionsToDeregister; - for (auto&& coll : - catalog.range(DatabaseName::createDatabaseName_forTest(boost::none, "resourceDb"))) { + for (auto it = catalog.begin( + opCtx.get(), DatabaseName::createDatabaseName_forTest(boost::none, "resourceDb")); + it != catalog.end(opCtx.get()); + ++it) { + auto coll = *it; auto uuid = coll->uuid(); if (!coll) { break; @@ -217,8 +232,10 @@ public: } int numEntries = 0; - for ([[maybe_unused]] auto&& coll : - catalog.range(DatabaseName::createDatabaseName_forTest(boost::none, "resourceDb"))) { + for (auto it = catalog.begin( + opCtx.get(), DatabaseName::createDatabaseName_forTest(boost::none, "resourceDb")); + it != catalog.end(opCtx.get()); + it++) { numEntries++; } ASSERT_EQ(0, numEntries); @@ -301,14 +318,14 @@ TEST_F(CollectionCatalogIterationTest, EndAtEndOfSection) { } TEST_F(CollectionCatalogIterationTest, GetUUIDWontRepositionEvenIfEntryIsDropped) { - auto range = catalog.range(DatabaseName::createDatabaseName_forTest(boost::none, "bar")); - auto it = range.begin(); + auto it = + catalog.begin(opCtx.get(), DatabaseName::createDatabaseName_forTest(boost::none, "bar")); auto collsIt = collsIterator("bar"); auto uuid = collsIt->first; catalog.deregisterCollection(opCtx.get(), uuid, /*isDropPending=*/false, boost::none); dropColl("bar", uuid); - ASSERT_EQUALS(uuid, (*it)->uuid()); + ASSERT_EQUALS(uuid, it.uuid()); } TEST_F(CollectionCatalogTest, OnCreateCollection) { @@ -333,13 +350,13 @@ TEST_F(CollectionCatalogTest, LookupNSSByUUID) { TEST_F(CollectionCatalogTest, InsertAfterLookup) { auto newUUID = UUID::gen(); NamespaceString newNss = NamespaceString::createNamespaceString_forTest(nss.dbName(), "newcol"); - std::shared_ptr<Collection> newCollShared = std::make_shared<CollectionMock>(newUUID, newNss); + std::shared_ptr<Collection> newCollShared = std::make_shared<CollectionMock>(newNss); auto newCol = newCollShared.get(); // Ensure that looking up non-existing UUIDs doesn't affect later registration of those UUIDs. ASSERT(catalog.lookupCollectionByUUID(opCtx.get(), newUUID) == nullptr); ASSERT_EQUALS(catalog.lookupNSSByUUID(opCtx.get(), newUUID), boost::none); - catalog.registerCollection(opCtx.get(), std::move(newCollShared), boost::none); + catalog.registerCollection(opCtx.get(), newUUID, std::move(newCollShared), boost::none); ASSERT_EQUALS(catalog.lookupCollectionByUUID(opCtx.get(), newUUID), newCol); ASSERT_EQUALS(*catalog.lookupNSSByUUID(opCtx.get(), colUUID), nss); } @@ -386,7 +403,7 @@ TEST_F(CollectionCatalogTest, RenameCollection) { NamespaceString oldNss = NamespaceString::createNamespaceString_forTest(nss.dbName(), "oldcol"); std::shared_ptr<Collection> collShared = std::make_shared<CollectionMock>(uuid, oldNss); auto collection = collShared.get(); - catalog.registerCollection(opCtx.get(), std::move(collShared), boost::none); + catalog.registerCollection(opCtx.get(), uuid, std::move(collShared), boost::none); CollectionPtr yieldableColl(catalog.lookupCollectionByUUID(opCtx.get(), uuid)); ASSERT(yieldableColl); ASSERT_EQUALS(yieldableColl, CollectionPtr(collection)); @@ -445,7 +462,7 @@ TEST_F(CollectionCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsOldNSSIfDrop TEST_F(CollectionCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsNewlyCreatedNSS) { auto newUUID = UUID::gen(); NamespaceString newNss = NamespaceString::createNamespaceString_forTest(nss.dbName(), "newcol"); - std::shared_ptr<Collection> newCollShared = std::make_shared<CollectionMock>(newUUID, newNss); + std::shared_ptr<Collection> newCollShared = std::make_shared<CollectionMock>(newNss); auto newCol = newCollShared.get(); // Ensure that looking up non-existing UUIDs doesn't affect later registration of those UUIDs. @@ -456,7 +473,7 @@ TEST_F(CollectionCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsNewlyCreated ASSERT(catalog.lookupCollectionByUUID(opCtx.get(), newUUID) == nullptr); ASSERT_EQUALS(catalog.lookupNSSByUUID(opCtx.get(), newUUID), boost::none); - catalog.registerCollection(opCtx.get(), std::move(newCollShared), boost::none); + catalog.registerCollection(opCtx.get(), newUUID, std::move(newCollShared), boost::none); ASSERT_EQUALS(catalog.lookupCollectionByUUID(opCtx.get(), newUUID), newCol); ASSERT_EQUALS(*catalog.lookupNSSByUUID(opCtx.get(), colUUID), nss); @@ -472,7 +489,7 @@ TEST_F(CollectionCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsNewlyCreated TEST_F(CollectionCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsFreshestNSS) { NamespaceString newNss = NamespaceString::createNamespaceString_forTest(nss.dbName(), "newcol"); - std::shared_ptr<Collection> newCollShared = std::make_shared<CollectionMock>(colUUID, newNss); + std::shared_ptr<Collection> newCollShared = std::make_shared<CollectionMock>(newNss); auto newCol = newCollShared.get(); { @@ -485,7 +502,7 @@ TEST_F(CollectionCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsFreshestNSS) ASSERT_EQUALS(*catalog.lookupNSSByUUID(opCtx.get(), colUUID), nss); { Lock::GlobalWrite lk(opCtx.get()); - catalog.registerCollection(opCtx.get(), std::move(newCollShared), boost::none); + catalog.registerCollection(opCtx.get(), colUUID, std::move(newCollShared), boost::none); } ASSERT_EQUALS(catalog.lookupCollectionByUUID(opCtx.get(), colUUID), newCol); @@ -527,7 +544,8 @@ TEST_F(CollectionCatalogTest, GetAllCollectionNamesAndGetAllDbNames) { std::vector<NamespaceString> nsss = {aColl, b1Coll, b2Coll, cColl, d1Coll, d2Coll, d3Coll}; for (auto& nss : nsss) { std::shared_ptr<Collection> newColl = std::make_shared<CollectionMock>(nss); - catalog.registerCollection(opCtx.get(), std::move(newColl), boost::none); + auto uuid = UUID::gen(); + catalog.registerCollection(opCtx.get(), uuid, std::move(newColl), boost::none); } std::vector<NamespaceString> dCollList = {d1Coll, d2Coll, d3Coll}; @@ -559,7 +577,8 @@ TEST_F(CollectionCatalogTest, GetAllDbNamesForTenant) { std::vector<NamespaceString> nsss = {dbA, dbB, dbC, dbD}; for (auto& nss : nsss) { std::shared_ptr<Collection> newColl = std::make_shared<CollectionMock>(nss); - catalog.registerCollection(opCtx.get(), std::move(newColl), boost::none); + auto uuid = UUID::gen(); + catalog.registerCollection(opCtx.get(), uuid, std::move(newColl), boost::none); } std::vector<DatabaseName> dbNamesForTid1 = { @@ -588,7 +607,8 @@ TEST_F(CollectionCatalogTest, GetAllTenants) { for (auto& nss : nsss) { std::shared_ptr<Collection> newColl = std::make_shared<CollectionMock>(nss); - catalog.registerCollection(opCtx.get(), std::move(newColl), boost::none); + auto uuid = UUID::gen(); + catalog.registerCollection(opCtx.get(), uuid, std::move(newColl), boost::none); } std::set<TenantId> expectedTenants = {tid1, tid2}; @@ -633,7 +653,8 @@ TEST_F(CollectionCatalogTest, GetAllCollectionNamesAndGetAllDbNamesWithUncommitt std::vector<NamespaceString> nsss = {aColl, b1Coll, b2Coll, cColl, d1Coll, d2Coll, d3Coll}; for (auto& nss : nsss) { std::shared_ptr<Collection> newColl = std::make_shared<CollectionMock>(nss); - catalog.registerCollection(opCtx.get(), std::move(newColl), boost::none); + auto uuid = UUID::gen(); + catalog.registerCollection(opCtx.get(), uuid, std::move(newColl), boost::none); } // One dbName with only an invisible collection does not appear in dbNames. Use const_cast to diff --git a/src/mongo/db/catalog/collection_writer_test.cpp b/src/mongo/db/catalog/collection_writer_test.cpp index f509ffa8bb9..15368437ff4 100644 --- a/src/mongo/db/catalog/collection_writer_test.cpp +++ b/src/mongo/db/catalog/collection_writer_test.cpp @@ -59,8 +59,9 @@ protected: std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(kNss); CollectionCatalog::write(getServiceContext(), [&](CollectionCatalog& catalog) { + auto uuid = collection->uuid(); catalog.registerCollection( - operationContext(), std::move(collection), /*ts=*/boost::none); + operationContext(), uuid, std::move(collection), /*ts=*/boost::none); }); } @@ -254,6 +255,7 @@ public: for (size_t i = 0; i < NumCollections; ++i) { catalog.registerCollection( operationContext(), + UUID::gen(), std::make_shared<CollectionMock>(NamespaceString::createNamespaceString_forTest( "many", fmt::format("coll{}", i))), /*ts=*/boost::none); diff --git a/src/mongo/db/catalog/database_holder_impl.cpp b/src/mongo/db/catalog/database_holder_impl.cpp index fdd16d9f9ee..73c0d39750a 100644 --- a/src/mongo/db/catalog/database_holder_impl.cpp +++ b/src/mongo/db/catalog/database_holder_impl.cpp @@ -179,7 +179,8 @@ void DatabaseHolderImpl::dropDb(OperationContext* opCtx, Database* db) { invariant(opCtx->lockState()->isDbLockedForMode(name, MODE_X)); auto catalog = CollectionCatalog::get(opCtx); - for (auto&& coll : catalog->range(name)) { + for (auto collIt = catalog->begin(opCtx, name); collIt != catalog->end(opCtx); ++collIt) { + auto coll = *collIt; if (!coll) { break; } @@ -195,7 +196,8 @@ void DatabaseHolderImpl::dropDb(OperationContext* opCtx, Database* db) { auto const serviceContext = opCtx->getServiceContext(); - for (auto&& coll : catalog->range(name)) { + for (auto collIt = catalog->begin(opCtx, name); collIt != catalog->end(opCtx); ++collIt) { + auto coll = *collIt; if (!coll) { break; } diff --git a/src/mongo/db/catalog/drop_database.cpp b/src/mongo/db/catalog/drop_database.cpp index ec307920a21..ed431fb0370 100644 --- a/src/mongo/db/catalog/drop_database.cpp +++ b/src/mongo/db/catalog/drop_database.cpp @@ -273,7 +273,9 @@ Status _dropDatabase(OperationContext* opCtx, const DatabaseName& dbName, bool a catalog = CollectionCatalog::get(opCtx); std::vector<NamespaceString> collectionsToDrop; - for (auto&& collection : catalog->range(db->name())) { + for (auto collIt = catalog->begin(opCtx, db->name()); collIt != catalog->end(opCtx); + ++collIt) { + auto collection = *collIt; if (!collection) { break; } diff --git a/src/mongo/db/catalog/uncommitted_catalog_updates.cpp b/src/mongo/db/catalog/uncommitted_catalog_updates.cpp index 87109f28314..751c0b25f6f 100644 --- a/src/mongo/db/catalog/uncommitted_catalog_updates.cpp +++ b/src/mongo/db/catalog/uncommitted_catalog_updates.cpp @@ -122,7 +122,7 @@ void UncommittedCatalogUpdates::_createCollection(OperationContext* opCtx, // This will throw when registering a namespace which is already in use. CollectionCatalog::write(opCtx, [&, coll = createdColl](CollectionCatalog& catalog) { - catalog.registerCollectionTwoPhase(opCtx, coll, /*ts=*/boost::none); + catalog.registerCollectionTwoPhase(opCtx, uuid, coll, /*ts=*/boost::none); }); opCtx->recoveryUnit()->onRollback([uuid](OperationContext* opCtx) { diff --git a/src/mongo/db/commands/dbhash.cpp b/src/mongo/db/commands/dbhash.cpp index d412eb48f3f..9f413648913 100644 --- a/src/mongo/db/commands/dbhash.cpp +++ b/src/mongo/db/commands/dbhash.cpp @@ -311,22 +311,22 @@ public: return true; }; - for (auto&& coll : catalog->range(dbName)) { - UUID uuid = coll->uuid(); + for (auto it = catalog->begin(opCtx, dbName); it != catalog->end(opCtx); ++it) { + UUID uuid = it.uuid(); // The namespace must be found as the UUID is fetched from the same // CollectionCatalog instance. boost::optional<NamespaceString> nss = catalog->lookupNSSByUUID(opCtx, uuid); invariant(nss); - const Collection* collection = nullptr; + const Collection* coll = nullptr; if (nss->isGlobalIndex()) { // TODO SERVER-74209: Reading earlier than the minimum valid snapshot is not // supported for global indexes. It appears that the primary and secondaries apply // operations differently resulting in hash mismatches. This requires further // investigation. In the meantime, global indexes use the behaviour prior to // point-in-time lookups. - collection = coll; + coll = *it; if (auto readTimestamp = opCtx->recoveryUnit()->getPointInTimeReadTimestamp(opCtx)) { @@ -343,18 +343,18 @@ public: } else { // TODO:SERVER-75848 Make this lock-free Lock::CollectionLock clk(opCtx, *nss, MODE_IS); - collection = catalog->establishConsistentCollection( + coll = catalog->establishConsistentCollection( opCtx, {dbName, uuid}, opCtx->recoveryUnit()->getPointInTimeReadTimestamp(opCtx)); - if (!collection) { + if (!coll) { // The collection did not exist at the read timestamp with the given UUID. continue; } } - (void)checkAndHashCollection(collection); + (void)checkAndHashCollection(coll); } BSONObjBuilder bb(result.subobjStart("collections")); diff --git a/src/mongo/db/commands/list_collections.cpp b/src/mongo/db/commands/list_collections.cpp index 571c3156786..4d192815e27 100644 --- a/src/mongo/db/commands/list_collections.cpp +++ b/src/mongo/db/commands/list_collections.cpp @@ -440,8 +440,10 @@ public: // needing to yield as we don't take any locks. if (opCtx->isLockFreeReadsOp()) { auto collectionCatalog = CollectionCatalog::get(opCtx); - for (auto&& coll : collectionCatalog->range(dbName)) { - perCollectionWork(coll); + for (auto it = collectionCatalog->begin(opCtx, dbName); + it != collectionCatalog->end(opCtx); + ++it) { + perCollectionWork(*it); } } else { mongo::catalog::forEachCollectionFromDb( diff --git a/src/mongo/db/commands/validate_db_metadata_cmd.cpp b/src/mongo/db/commands/validate_db_metadata_cmd.cpp index 43ef8d739f6..d4347d72884 100644 --- a/src/mongo/db/commands/validate_db_metadata_cmd.cpp +++ b/src/mongo/db/commands/validate_db_metadata_cmd.cpp @@ -149,10 +149,12 @@ public: return _validateView(opCtx, view); }); - for (auto&& coll : collectionCatalog->range(dbName)) { + for (auto collIt = collectionCatalog->begin(opCtx, dbName); + collIt != collectionCatalog->end(opCtx); + ++collIt) { if (!_validateNamespace( opCtx, - collectionCatalog->lookupNSSByUUID(opCtx, coll->uuid()).value())) { + collectionCatalog->lookupNSSByUUID(opCtx, collIt.uuid()).value())) { return; } } diff --git a/src/mongo/db/pipeline/document_source_change_stream_test.cpp b/src/mongo/db/pipeline/document_source_change_stream_test.cpp index ebf1557d215..d84412e5284 100644 --- a/src/mongo/db/pipeline/document_source_change_stream_test.cpp +++ b/src/mongo/db/pipeline/document_source_change_stream_test.cpp @@ -496,7 +496,8 @@ TEST_F(ChangeStreamStageTest, ShouldRejectBothStartAtOperationTimeAndResumeAfter Lock::GlobalWrite lk(expCtx->opCtx); std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(expCtx->opCtx, [&](CollectionCatalog& catalog) { - catalog.registerCollection(expCtx->opCtx, std::move(collection), /*ts=*/boost::none); + catalog.registerCollection( + expCtx->opCtx, testUuid(), std::move(collection), /*ts=*/boost::none); }); } @@ -522,7 +523,8 @@ TEST_F(ChangeStreamStageTest, ShouldRejectBothStartAfterAndResumeAfterOptions) { Lock::GlobalWrite lk(opCtx); std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(opCtx, [&](CollectionCatalog& catalog) { - catalog.registerCollection(opCtx, std::move(collection), /*ts=*/boost::none); + catalog.registerCollection( + opCtx, testUuid(), std::move(collection), /*ts=*/boost::none); }); } @@ -553,7 +555,8 @@ TEST_F(ChangeStreamStageTest, ShouldRejectBothStartAtOperationTimeAndStartAfterO Lock::GlobalWrite lk(opCtx); std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(opCtx, [&](CollectionCatalog& catalog) { - catalog.registerCollection(opCtx, std::move(collection), /*ts=*/boost::none); + catalog.registerCollection( + opCtx, testUuid(), std::move(collection), /*ts=*/boost::none); }); } @@ -579,7 +582,8 @@ TEST_F(ChangeStreamStageTest, ShouldRejectResumeAfterWithResumeTokenMissingUUID) Lock::GlobalWrite lk(opCtx); std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(opCtx, [&](CollectionCatalog& catalog) { - catalog.registerCollection(opCtx, std::move(collection), /*ts=*/boost::none); + catalog.registerCollection( + opCtx, testUuid(), std::move(collection), /*ts=*/boost::none); }); } @@ -2763,10 +2767,10 @@ TEST_F(ChangeStreamStageTest, DocumentKeyShouldNotIncludeShardKeyWhenNoO2FieldIn { Lock::GlobalWrite lk(getExpCtx()->opCtx); - std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(uuid, nss); + std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(getExpCtx()->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, uuid, std::move(collection), /*ts=*/boost::none); }); } @@ -2810,10 +2814,10 @@ TEST_F(ChangeStreamStageTest, DocumentKeyShouldUseO2FieldInOplog) { { Lock::GlobalWrite lk(getExpCtx()->opCtx); - std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(uuid, nss); + std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(getExpCtx()->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, uuid, std::move(collection), /*ts=*/boost::none); }); } @@ -2853,13 +2857,14 @@ TEST_F(ChangeStreamStageTest, DocumentKeyShouldUseO2FieldInOplog) { TEST_F(ChangeStreamStageTest, ResumeAfterFailsIfResumeTokenDoesNotContainUUID) { const Timestamp ts(3, 45); + const auto uuid = testUuid(); { Lock::GlobalWrite lk(getExpCtx()->opCtx); std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(getExpCtx()->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, uuid, std::move(collection), /*ts=*/boost::none); }); } @@ -2926,7 +2931,7 @@ TEST_F(ChangeStreamStageTest, ResumeAfterWithTokenFromInvalidateShouldFail) { std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(expCtx->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, testUuid(), std::move(collection), /*ts=*/boost::none); }); } @@ -3418,10 +3423,10 @@ TEST_F(ChangeStreamStageDBTest, DocumentKeyShouldNotIncludeShardKeyWhenNoO2Field { Lock::GlobalWrite lk(getExpCtx()->opCtx); - std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(uuid, nss); + std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(getExpCtx()->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, uuid, std::move(collection), /*ts=*/boost::none); }); } @@ -3460,10 +3465,10 @@ TEST_F(ChangeStreamStageDBTest, DocumentKeyShouldUseO2FieldInOplog) { { Lock::GlobalWrite lk(getExpCtx()->opCtx); - std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(uuid, nss); + std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(getExpCtx()->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, uuid, std::move(collection), /*ts=*/boost::none); }); } @@ -3505,7 +3510,7 @@ TEST_F(ChangeStreamStageDBTest, ResumeAfterWithTokenFromInvalidateShouldFail) { std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(expCtx->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, testUuid(), std::move(collection), /*ts=*/boost::none); }); } @@ -3530,10 +3535,10 @@ TEST_F(ChangeStreamStageDBTest, ResumeAfterWithTokenFromDropDatabase) { { Lock::GlobalWrite lk(getExpCtx()->opCtx); - std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(uuid, nss); + std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(getExpCtx()->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, uuid, std::move(collection), /*ts=*/boost::none); }); } @@ -3568,10 +3573,10 @@ TEST_F(ChangeStreamStageDBTest, StartAfterSucceedsEvenIfResumeTokenDoesNotContai { Lock::GlobalWrite lk(getExpCtx()->opCtx); - std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(uuid, nss); + std::shared_ptr<Collection> collection = std::make_shared<CollectionMock>(nss); CollectionCatalog::write(getExpCtx()->opCtx, [&](CollectionCatalog& catalog) { catalog.registerCollection( - getExpCtx()->opCtx, std::move(collection), /*ts=*/boost::none); + getExpCtx()->opCtx, uuid, std::move(collection), /*ts=*/boost::none); }); } diff --git a/src/mongo/db/repl/shard_merge_recipient_op_observer.cpp b/src/mongo/db/repl/shard_merge_recipient_op_observer.cpp index b5e402ee14a..2f6ba5e7afd 100644 --- a/src/mongo/db/repl/shard_merge_recipient_op_observer.cpp +++ b/src/mongo/db/repl/shard_merge_recipient_op_observer.cpp @@ -97,7 +97,9 @@ void deleteTenantDataWhenMergeAborts(OperationContext* opCtx, IndexBuildsCoordinator::get(opCtx)->assertNoBgOpInProgForDb(db->name()); auto catalog = CollectionCatalog::get(opCtx); - for (auto&& collection : catalog->range(db->name())) { + for (auto collIt = catalog->begin(opCtx, db->name()); collIt != catalog->end(opCtx); + ++collIt) { + auto collection = *collIt; if (!collection) { break; } diff --git a/src/mongo/db/s/config/sharding_catalog_manager_shard_collection_test.cpp b/src/mongo/db/s/config/sharding_catalog_manager_shard_collection_test.cpp index e3ec02257ed..5424ecebd46 100644 --- a/src/mongo/db/s/config/sharding_catalog_manager_shard_collection_test.cpp +++ b/src/mongo/db/s/config/sharding_catalog_manager_shard_collection_test.cpp @@ -91,7 +91,8 @@ TEST_F(CreateFirstChunksTest, NonEmptyCollection_NoZones_OneChunkToPrimary) { Lock::GlobalWrite lk(operationContext()); CollectionCatalog::write(getServiceContext(), [&](CollectionCatalog& catalog) { catalog.registerCollection(operationContext(), - std::make_shared<CollectionMock>(uuid, kNamespace), + uuid, + std::make_shared<CollectionMock>(kNamespace), /*ts=*/boost::none); }); } diff --git a/src/mongo/db/s/move_primary/move_primary_database_cloner.cpp b/src/mongo/db/s/move_primary/move_primary_database_cloner.cpp index 3e901338f4b..9b198e212f8 100644 --- a/src/mongo/db/s/move_primary/move_primary_database_cloner.cpp +++ b/src/mongo/db/s/move_primary/move_primary_database_cloner.cpp @@ -151,8 +151,10 @@ void MovePrimaryDatabaseCloner::calculateListCatalogEntriesForRecipient() { if (opCtx->isLockFreeReadsOp()) { auto collectionCatalog = CollectionCatalog::get(opCtx); - for (auto&& coll : collectionCatalog->range(_dbName)) { - parseCollection(coll); + for (auto it = collectionCatalog->begin(opCtx, _dbName); + it != collectionCatalog->end(opCtx); + ++it) { + parseCollection(*it); } } else { mongo::catalog::forEachCollectionFromDb(opCtx, _dbName, MODE_IS, parseCollection); diff --git a/src/mongo/db/s/shardsvr_check_metadata_consistency_participant_command.cpp b/src/mongo/db/s/shardsvr_check_metadata_consistency_participant_command.cpp index 72a61486b5a..0d57f8494ed 100644 --- a/src/mongo/db/s/shardsvr_check_metadata_consistency_participant_command.cpp +++ b/src/mongo/db/s/shardsvr_check_metadata_consistency_participant_command.cpp @@ -266,7 +266,10 @@ public: std::vector<CollectionPtr> localCollections; switch (metadata_consistency_util::getCommandLevel(nss)) { case MetadataConsistencyCommandLevelEnum::kDatabaseLevel: { - for (auto&& coll : collCatalogSnapshot->range(nss.dbName())) { + for (auto it = collCatalogSnapshot->begin(opCtx, nss.dbName()); + it != collCatalogSnapshot->end(opCtx); + ++it) { + const auto coll = *it; if (!coll) { continue; } diff --git a/src/mongo/db/startup_recovery.cpp b/src/mongo/db/startup_recovery.cpp index 77c57e7e1a6..ea6d937e2c0 100644 --- a/src/mongo/db/startup_recovery.cpp +++ b/src/mongo/db/startup_recovery.cpp @@ -212,7 +212,8 @@ Status ensureCollectionProperties(OperationContext* opCtx, const DatabaseName& dbName, EnsureIndexPolicy ensureIndexPolicy) { auto catalog = CollectionCatalog::get(opCtx); - for (auto&& coll : catalog->range(dbName)) { + for (auto collIt = catalog->begin(opCtx, dbName); collIt != catalog->end(opCtx); ++collIt) { + auto coll = *collIt; if (!coll) { break; } @@ -232,7 +233,7 @@ Status ensureCollectionProperties(OperationContext* opCtx, logAttrs(*coll)); if (EnsureIndexPolicy::kBuildMissing == ensureIndexPolicy) { auto writableCollection = - catalog->lookupCollectionByUUIDForMetadataWrite(opCtx, coll->uuid()); + catalog->lookupCollectionByUUIDForMetadataWrite(opCtx, collIt.uuid()); auto status = buildMissingIdIndex(opCtx, writableCollection); if (!status.isOK()) { LOGV2_ERROR(21021, diff --git a/src/mongo/db/storage/kv/durable_catalog_test.cpp b/src/mongo/db/storage/kv/durable_catalog_test.cpp index bd49f858a33..5a29311d40b 100644 --- a/src/mongo/db/storage/kv/durable_catalog_test.cpp +++ b/src/mongo/db/storage/kv/durable_catalog_test.cpp @@ -119,6 +119,7 @@ public: CollectionCatalog::write(operationContext(), [&](CollectionCatalog& catalog) { catalog.registerCollection(operationContext(), + options.uuid.value(), std::move(collection), /*ts=*/boost::none); }); diff --git a/src/mongo/db/storage/storage_engine_impl.cpp b/src/mongo/db/storage/storage_engine_impl.cpp index fc567484812..4cfa38852c0 100644 --- a/src/mongo/db/storage/storage_engine_impl.cpp +++ b/src/mongo/db/storage/storage_engine_impl.cpp @@ -428,7 +428,8 @@ void StorageEngineImpl::_initCollection(OperationContext* opCtx, auto collection = collectionFactory->make(opCtx, nss, catalogId, md, std::move(rs)); CollectionCatalog::write(opCtx, [&](CollectionCatalog& catalog) { - catalog.registerCollection(opCtx, std::move(collection), /*commitTime*/ minValidTs); + catalog.registerCollection( + opCtx, md->options.uuid.value(), std::move(collection), /*commitTime*/ minValidTs); }); } @@ -1398,8 +1399,9 @@ int64_t StorageEngineImpl::sizeOnDiskForDb(OperationContext* opCtx, const Databa if (opCtx->isLockFreeReadsOp()) { auto collectionCatalog = CollectionCatalog::get(opCtx); - for (auto&& coll : collectionCatalog->range(dbName)) { - perCollectionWork(coll); + for (auto it = collectionCatalog->begin(opCtx, dbName); it != collectionCatalog->end(opCtx); + ++it) { + perCollectionWork(*it); } } else { catalog::forEachCollectionFromDb(opCtx, dbName, MODE_IS, perCollectionWork); diff --git a/src/mongo/db/storage/storage_engine_test_fixture.h b/src/mongo/db/storage/storage_engine_test_fixture.h index 6e9c3ef8cce..d0a79882e9f 100644 --- a/src/mongo/db/storage/storage_engine_test_fixture.h +++ b/src/mongo/db/storage/storage_engine_test_fixture.h @@ -82,7 +82,8 @@ public: std::move(rs)); CollectionCatalog::write(opCtx, [&](CollectionCatalog& catalog) { - catalog.registerCollection(opCtx, std::move(coll), /*ts=*/boost::none); + catalog.registerCollection( + opCtx, options.uuid.get(), std::move(coll), /*ts=*/boost::none); }); return {{_storageEngine->getCatalog()->getEntry(catalogId)}}; diff --git a/src/mongo/db/storage/storage_util.cpp b/src/mongo/db/storage/storage_util.cpp index e7634b858c9..7339dde91f2 100644 --- a/src/mongo/db/storage/storage_util.cpp +++ b/src/mongo/db/storage/storage_util.cpp @@ -59,7 +59,8 @@ auto removeEmptyDirectory = auto collectionCatalog = CollectionCatalog::latest(svcCtx); const DatabaseName& dbName = ns.dbName(); if (!storageEngine->isUsingDirectoryPerDb() || - (storageEngine->supportsPendingDrops() && !collectionCatalog->range(dbName).empty())) { + (storageEngine->supportsPendingDrops() && + collectionCatalog->begin(nullptr, dbName) != collectionCatalog->end(nullptr))) { return; } @@ -68,7 +69,7 @@ auto removeEmptyDirectory = if (!ec) { LOGV2(4888200, "Removed empty database directory", logAttrs(dbName)); - } else if (collectionCatalog->range(dbName).empty()) { + } else if (collectionCatalog->begin(nullptr, dbName) == collectionCatalog->end(nullptr)) { // It is possible for a new collection to be created in the database between when we // check whether the database is empty and actually attempting to remove the directory. // In this case, don't log that the removal failed because it is expected. However, |