diff --git a/src/mongo/db/query/collection_query_info.cpp b/src/mongo/db/query/collection_query_info.cpp index ac951ff9778..840ad2d6719 100644 --- a/src/mongo/db/query/collection_query_info.cpp +++ b/src/mongo/db/query/collection_query_info.cpp @@ -116,9 +116,7 @@ void CollectionQueryInfo::PlanCacheState::clearPlanCache() { planCacheInvalidator.clearPlanCache(); } -CollectionQueryInfo::CollectionQueryInfo() - : _planCacheState{std::make_shared()}, - _pathArraynessState{std::make_shared()} {} +CollectionQueryInfo::CollectionQueryInfo() : _planCacheState{std::make_shared()} {} void CollectionQueryInfo::clearQueryCache(OperationContext* opCtx, const CollectionPtr& coll) { // We are operating on a cloned collection, the use_count can only be 1 if we've created a new @@ -171,12 +169,7 @@ CollectionQueryInfo::PathArraynessState::PathArraynessState() void CollectionQueryInfo::updatePathArraynessForSetMultikey(OperationContext* opCtx, const Collection* coll) const { - // Acquire write lock to prevent concurrent re-assignment of PathArrayness. - auto writeLock = _pathArraynessState->rwMutex.writeLock(); - // Create an empty PathArrayness in response to an index multikeyness change from a document - // write/update operation. // TODO: SERVER-114809: Create a PathArrayness that reflects a more precise arrayness state. - _pathArraynessState->pathArrayness = std::make_shared(); } void CollectionQueryInfo::rebuildPathArrayness(OperationContext* opCtx, const Collection* coll) { @@ -210,12 +203,13 @@ void CollectionQueryInfo::rebuildPathArrayness(OperationContext* opCtx, const Co } } } - _pathArraynessState->pathArrayness = std::make_shared(tmpPathArrayness); + // We assume that this method will be called in a thread-safe manner and thus, can safely do + // this re-assignment without a mutex (see method header comment for more details). + _pathArraynessState.pathArrayness = std::make_shared(tmpPathArrayness); } std::shared_ptr CollectionQueryInfo::getPathArrayness() const { - auto readLock = _pathArraynessState->rwMutex.readLock(); - return _pathArraynessState->pathArrayness; + return _pathArraynessState.pathArrayness; } void CollectionQueryInfo::init(OperationContext* opCtx, Collection* coll) { diff --git a/src/mongo/db/query/collection_query_info.h b/src/mongo/db/query/collection_query_info.h index 5f49a3bfa75..8f55b2b1d88 100644 --- a/src/mongo/db/query/collection_query_info.h +++ b/src/mongo/db/query/collection_query_info.h @@ -133,11 +133,7 @@ public: * document insert or update flips the multikeyness of indexes. This flip in multikeyness can * only go in one direction, non-multikey to multikey. * - * Const-ness: While this method modifies the 'PathArrayness' data associated with the - * 'CollectionQueryInfo', it can be marked as 'const' because the changes are made indirectly - * through the 'PathArraynessState' wrapper. This design ensures compatibility with existing - * methods like 'IndexCatalogEntry::_catalogSetMultikey()' which require the method to be - * 'const'. + * TODO: SERVER-115001: Explain const-ness of function when we implement. */ void updatePathArraynessForSetMultikey(OperationContext* opCtx, const Collection* coll) const; @@ -179,10 +175,8 @@ private: struct PathArraynessState { PathArraynessState(); - // Mutex to protect concurrent writers from re-assigning the pathArrayness pointer. - // This is going to face contention in the rare case where the multikeyness of an index is - // flipped in a document write transaction concurrently with a query. - mutable WriteRarelyRWMutex rwMutex; + // All clones of CollectionQueryInfo will initially point to the same PathArrayness + // instance. std::shared_ptr pathArrayness; }; @@ -190,10 +184,7 @@ private: std::shared_ptr _planCacheState; - // Use std::shared_ptr to ensure that multiple cloned instances of 'Collection' instance can - // share the same 'PathArraynessState'. We clone the 'CollectionQueryInfo' when 'Collection' - // gets cloned. - std::shared_ptr _pathArraynessState; + PathArraynessState _pathArraynessState; }; } // namespace mongo diff --git a/src/mongo/db/query/collection_query_info_test.cpp b/src/mongo/db/query/collection_query_info_test.cpp index 148e9995958..a59676f564d 100644 --- a/src/mongo/db/query/collection_query_info_test.cpp +++ b/src/mongo/db/query/collection_query_info_test.cpp @@ -162,36 +162,37 @@ TEST_F(CollectionQueryInfoTest, PathArraynessUpdatesForCreateIndex) { } } -TEST_F(CollectionQueryInfoTest, PathArraynessUpdatesForMultikeyChange) { - RAIIServerParameterControllerForTest featureFlag{"featureFlagPathArrayness", true}; - std::vector docs; - for (int i = 0; i < 100; ++i) { - docs.push_back(BSON("_id" << i << "a" << i)); - } - ce::createCollAndInsertDocuments(operationContext(), _kTestNss, docs); - - auto indexA = BSON("a" << 1); - ASSERT_OK(mongo::createIndex(operationContext(), _kTestNss.ns_forTest(), indexA)); - - { - const auto coll = acquireCollectionForRead(operationContext(), _kTestNss); - const auto pathArrayness = - CollectionQueryInfo::get(coll.getCollection().getCollectionPtr()).getPathArrayness(); - // "a" is not multi-key at this point. - ASSERT_FALSE(pathArrayness.get()->isPathArray("a")); - } - - // Make "a" multikey but inserting a doc where "a" is multikey. - const auto multikeyADoc = BSON("_id" << 100 << "a" << BSON_ARRAY(1)); - insertDocuments(_kTestNss, {multikeyADoc}); - { - const auto coll = acquireCollectionForRead(operationContext(), _kTestNss); - const auto pathArrayness = - CollectionQueryInfo::get(coll.getCollection().getCollectionPtr()).getPathArrayness(); - // "a" is now mulitkey. - ASSERT_TRUE(pathArrayness.get()->isPathArray("a")); - } -} +// TODO: SERVER-114809: Re-enable test once we finalize this PR. +// TEST_F(CollectionQueryInfoTest, PathArraynessUpdatesForMultikeyChange) { +// RAIIServerParameterControllerForTest featureFlag{"featureFlagPathArrayness", true}; +// std::vector docs; +// for (int i = 0; i < 100; ++i) { +// docs.push_back(BSON("_id" << i << "a" << i)); +// } +// ce::createCollAndInsertDocuments(operationContext(), _kTestNss, docs); +// +// auto indexA = BSON("a" << 1); +// ASSERT_OK(mongo::createIndex(operationContext(), _kTestNss.ns_forTest(), indexA)); +// +// { +// const auto coll = acquireCollectionForRead(operationContext(), _kTestNss); +// const auto pathArrayness = +// CollectionQueryInfo::get(coll.getCollection().getCollectionPtr()).getPathArrayness(); +// // "a" is not multi-key at this point. +// ASSERT_FALSE(pathArrayness.get()->isPathArray("a")); +// } +// +// // Make "a" multikey but inserting a doc where "a" is multikey. +// const auto multikeyADoc = BSON("_id" << 100 << "a" << BSON_ARRAY(1)); +// insertDocuments(_kTestNss, {multikeyADoc}); +// { +// const auto coll = acquireCollectionForRead(operationContext(), _kTestNss); +// const auto pathArrayness = +// CollectionQueryInfo::get(coll.getCollection().getCollectionPtr()).getPathArrayness(); +// // "a" is now mulitkey. +// ASSERT_TRUE(pathArrayness.get()->isPathArray("a")); +// } +// } TEST_F(CollectionQueryInfoTest, PathArraynessUpdatesForDropIndex) { RAIIServerParameterControllerForTest featureFlag{"featureFlagPathArrayness", true};