diff options
author | Devin Hilly <devin.hilly@mongodb.com> | 2018-11-13 17:55:50 -0500 |
---|---|---|
committer | Charlie Swanson <charlie.swanson@mongodb.com> | 2019-02-06 18:19:07 -0500 |
commit | 93f8e8b2df10c2ec28090d9bbfd533b45f3b58e2 (patch) | |
tree | e98c6abd0edf8a9e39916e3f0b3519be0dbf9764 | |
parent | 5598d54afdfdd5efc418016aeb927cef0a62e3f3 (diff) | |
download | mongo-93f8e8b2df10c2ec28090d9bbfd533b45f3b58e2.tar.gz |
SERVER-31098 Wrong ns in system.profile for aggregation query
(cherry picked from commit 1862b00862a6ea9c3fdd08bba52ea6a016eccbc5)
-rw-r--r-- | buildscripts/resmokeconfig/suites/aggregation_mongos_passthrough.yml | 1 | ||||
-rw-r--r-- | buildscripts/resmokeconfig/suites/aggregation_one_shard_sharded_collections.yml | 1 | ||||
-rw-r--r-- | buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml | 1 | ||||
-rw-r--r-- | jstests/aggregation/sources/lookup/profile_lookup.js | 40 | ||||
-rw-r--r-- | src/mongo/db/commands/getmore_cmd.cpp | 8 | ||||
-rw-r--r-- | src/mongo/db/commands/killcursors_cmd.cpp | 7 | ||||
-rw-r--r-- | src/mongo/db/commands/run_aggregate.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/db_raii.cpp | 17 | ||||
-rw-r--r-- | src/mongo/db/db_raii.h | 29 | ||||
-rw-r--r-- | src/mongo/db/pipeline/pipeline_d.cpp | 11 | ||||
-rw-r--r-- | src/mongo/db/query/find.cpp | 7 |
11 files changed, 107 insertions, 21 deletions
diff --git a/buildscripts/resmokeconfig/suites/aggregation_mongos_passthrough.yml b/buildscripts/resmokeconfig/suites/aggregation_mongos_passthrough.yml index 700a7586c45..a2309b45b67 100644 --- a/buildscripts/resmokeconfig/suites/aggregation_mongos_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/aggregation_mongos_passthrough.yml @@ -13,6 +13,7 @@ selector: # TODO: Remove when SERVER-23229 is fixed. - jstests/aggregation/bugs/groupMissing.js exclude_with_any_tags: + - requires_profiling # The following tests start their own ShardingTest or ReplSetTest, respectively. - requires_sharding - requires_replication diff --git a/buildscripts/resmokeconfig/suites/aggregation_one_shard_sharded_collections.yml b/buildscripts/resmokeconfig/suites/aggregation_one_shard_sharded_collections.yml index 5a3418ff8b4..a177da99cb1 100644 --- a/buildscripts/resmokeconfig/suites/aggregation_one_shard_sharded_collections.yml +++ b/buildscripts/resmokeconfig/suites/aggregation_one_shard_sharded_collections.yml @@ -18,6 +18,7 @@ selector: - assumes_no_implicit_index_creation - assumes_unsharded_collection - cannot_create_unique_index_when_using_hashed_shard_key + - requires_profiling # The following tests start their own ShardingTest or ReplSetTest, respectively. - requires_sharding - requires_replication diff --git a/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml b/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml index 782c5c57d78..89958479da2 100644 --- a/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml @@ -30,6 +30,7 @@ selector: - assumes_unsharded_collection - cannot_create_unique_index_when_using_hashed_shard_key - requires_sharding + - requires_profiling executor: archive: diff --git a/jstests/aggregation/sources/lookup/profile_lookup.js b/jstests/aggregation/sources/lookup/profile_lookup.js new file mode 100644 index 00000000000..a861adbb93f --- /dev/null +++ b/jstests/aggregation/sources/lookup/profile_lookup.js @@ -0,0 +1,40 @@ +// @tags: [does_not_support_stepdowns, requires_profiling] +// +// Tests that profiled $lookups contain the correct namespace and that Top is updated accordingly. + +(function() { + "use strict"; + + const localColl = db.local; + const foreignColl = db.foreign; + localColl.drop(); + foreignColl.drop(); + + assert.commandWorked(localColl.insert([{a: 1}, {b: 1}, {a: 2}])); + assert.commandWorked(foreignColl.insert({a: 1})); + + db.system.profile.drop(); + db.setProfilingLevel(2); + + const oldTop = db.adminCommand("top"); + + localColl.aggregate( + [{$lookup: {from: foreignColl.getName(), as: "res", localField: "a", foreignField: "a"}}]); + + db.setProfilingLevel(0); + + // Confirm that namespace is the local rather than foreign collection. + const profileDoc = db.system.profile.findOne(); + assert.eq("test.local", profileDoc.ns); + + // Confirm that the local collection had one command added to Top. + const newTop = db.adminCommand("top"); + assert.eq(1, + newTop.totals[localColl.getFullName()].commands.count - + oldTop.totals[localColl.getFullName()].commands.count); + + // Confirm that for each document in local, the foreign collection had one entry added to Top. + assert.eq(3, + newTop.totals[foreignColl.getFullName()].commands.count - + oldTop.totals[foreignColl.getFullName()].commands.count); +}()); diff --git a/src/mongo/db/commands/getmore_cmd.cpp b/src/mongo/db/commands/getmore_cmd.cpp index 47be5591504..8366f354c06 100644 --- a/src/mongo/db/commands/getmore_cmd.cpp +++ b/src/mongo/db/commands/getmore_cmd.cpp @@ -259,8 +259,11 @@ public: ? request.nss.getTargetNSForGloballyManagedNamespace() : request.nss) { const boost::optional<int> dbProfilingLevel = boost::none; - statsTracker.emplace( - opCtx, *nssForCurOp, Top::LockType::NotLocked, dbProfilingLevel); + statsTracker.emplace(opCtx, + *nssForCurOp, + Top::LockType::NotLocked, + AutoStatsTracker::LogMode::kUpdateTopAndCurop, + dbProfilingLevel); } } else { readLock.emplace(opCtx, request.nss); @@ -268,6 +271,7 @@ public: statsTracker.emplace(opCtx, request.nss, Top::LockType::ReadLocked, + AutoStatsTracker::LogMode::kUpdateTopAndCurop, readLock->getDb() ? readLock->getDb()->getProfilingLevel() : doNotChangeProfilingLevel); diff --git a/src/mongo/db/commands/killcursors_cmd.cpp b/src/mongo/db/commands/killcursors_cmd.cpp index 2d0bc8b985d..2819e3f6dc2 100644 --- a/src/mongo/db/commands/killcursors_cmd.cpp +++ b/src/mongo/db/commands/killcursors_cmd.cpp @@ -76,8 +76,11 @@ private: ? nss.getTargetNSForGloballyManagedNamespace() : nss) { const boost::optional<int> dbProfilingLevel = boost::none; - statsTracker.emplace( - opCtx, *nssForCurOp, Top::LockType::NotLocked, dbProfilingLevel); + statsTracker.emplace(opCtx, + *nssForCurOp, + Top::LockType::NotLocked, + AutoStatsTracker::LogMode::kUpdateTopAndCurop, + dbProfilingLevel); } } diff --git a/src/mongo/db/commands/run_aggregate.cpp b/src/mongo/db/commands/run_aggregate.cpp index 4e38fb03e96..c9eb61bb7a5 100644 --- a/src/mongo/db/commands/run_aggregate.cpp +++ b/src/mongo/db/commands/run_aggregate.cpp @@ -387,7 +387,11 @@ Status runAggregate(OperationContext* opCtx, // If this is a collectionless aggregation with no foreign namespaces, we don't want to // acquire any locks. Otherwise, lock the collection or view. if (nss.isCollectionlessAggregateNS() && pipelineInvolvedNamespaces.empty()) { - statsTracker.emplace(opCtx, nss, Top::LockType::NotLocked, 0); + statsTracker.emplace(opCtx, + nss, + Top::LockType::NotLocked, + AutoStatsTracker::LogMode::kUpdateTopAndCurop, + 0); } else { ctx.emplace(opCtx, nss, AutoGetCollection::ViewMode::kViewsPermitted); } diff --git a/src/mongo/db/db_raii.cpp b/src/mongo/db/db_raii.cpp index bfeaee724b2..52535e32181 100644 --- a/src/mongo/db/db_raii.cpp +++ b/src/mongo/db/db_raii.cpp @@ -57,27 +57,30 @@ MONGO_EXPORT_SERVER_PARAMETER(allowSecondaryReadsDuringBatchApplication, bool, t AutoStatsTracker::AutoStatsTracker(OperationContext* opCtx, const NamespaceString& nss, Top::LockType lockType, + LogMode logMode, boost::optional<int> dbProfilingLevel, Date_t deadline) - : _opCtx(opCtx), _lockType(lockType) { - if (!dbProfilingLevel) { + : _opCtx(opCtx), _lockType(lockType), _nss(nss) { + if (!dbProfilingLevel && logMode == LogMode::kUpdateTopAndCurop) { // No profiling level was determined, attempt to read the profiling level from the Database // object. - AutoGetDb autoDb(_opCtx, nss.db(), MODE_IS, deadline); + AutoGetDb autoDb(_opCtx, _nss.db(), MODE_IS, deadline); if (autoDb.getDb()) { dbProfilingLevel = autoDb.getDb()->getProfilingLevel(); } } stdx::lock_guard<Client> clientLock(*_opCtx->getClient()); - CurOp::get(_opCtx)->enter_inlock(nss.ns().c_str(), dbProfilingLevel); + if (logMode == LogMode::kUpdateTopAndCurop) { + CurOp::get(_opCtx)->enter_inlock(_nss.ns().c_str(), dbProfilingLevel); + } } AutoStatsTracker::~AutoStatsTracker() { auto curOp = CurOp::get(_opCtx); Top::get(_opCtx->getServiceContext()) .record(_opCtx, - curOp->getNS(), + _nss.ns(), curOp->getLogicalOp(), _lockType, durationCount<Microseconds>(curOp->elapsedTimeExcludingPauses()), @@ -253,11 +256,13 @@ AutoGetCollectionForReadCommand::AutoGetCollectionForReadCommand( OperationContext* opCtx, const NamespaceStringOrUUID& nsOrUUID, AutoGetCollection::ViewMode viewMode, - Date_t deadline) + Date_t deadline, + AutoStatsTracker::LogMode logMode) : _autoCollForRead(opCtx, nsOrUUID, viewMode, deadline), _statsTracker(opCtx, _autoCollForRead.getNss(), Top::LockType::ReadLocked, + logMode, _autoCollForRead.getDb() ? _autoCollForRead.getDb()->getProfilingLevel() : kDoNotChangeProfilingLevel, deadline) { diff --git a/src/mongo/db/db_raii.h b/src/mongo/db/db_raii.h index 1f9cd4bf768..0348fcbf4c0 100644 --- a/src/mongo/db/db_raii.h +++ b/src/mongo/db/db_raii.h @@ -39,22 +39,35 @@ namespace mongo { /** - * RAII-style class which automatically tracks the operation namespace in CurrentOp and records the - * operation via Top upon destruction. + * RAII-style class which can update the diagnostic state on the operation's CurOp object and record + * the operation via Top upon destruction. Can be configured to only update the Top counters if + * desired. */ class AutoStatsTracker { MONGO_DISALLOW_COPYING(AutoStatsTracker); public: /** - * Sets the namespace of the CurOp object associated with 'opCtx' to be 'nss' and starts the - * CurOp timer. 'lockType' describes which type of lock is held by this operation, and will be - * used for reporting via Top. If 'dbProfilingLevel' is not given, this constructor will acquire - * and then drop a database lock in order to determine the database's profiling level. + * Describes which diagnostics to update during the lifetime of this object. + */ + enum class LogMode { + kUpdateTop, // Increments the Top counter for this operation type and this namespace upon + // destruction. + kUpdateTopAndCurop, // In addition to incrementing the Top counter, adjusts state on the + // CurOp object associated with the OperationContext. Updates the + // namespace to be 'nss', starts a timer for the operation (if it + // hasn't started already), and figures out and records the profiling + // level of the operation. + }; + + /** + * If 'logMode' is 'kUpdateTopAndCurop', sets up and records state on the CurOp object attached + * to 'opCtx', as described above. */ AutoStatsTracker(OperationContext* opCtx, const NamespaceString& nss, Top::LockType lockType, + LogMode logMode, boost::optional<int> dbProfilingLevel, Date_t deadline = Date_t::max()); @@ -66,6 +79,7 @@ public: private: OperationContext* _opCtx; Top::LockType _lockType; + const NamespaceString _nss; }; /** @@ -143,7 +157,8 @@ public: OperationContext* opCtx, const NamespaceStringOrUUID& nsOrUUID, AutoGetCollection::ViewMode viewMode = AutoGetCollection::ViewMode::kViewsForbidden, - Date_t deadline = Date_t::max()); + Date_t deadline = Date_t::max(), + AutoStatsTracker::LogMode logMode = AutoStatsTracker::LogMode::kUpdateTopAndCurop); Database* getDb() const { return _autoCollForRead.getDb(); diff --git a/src/mongo/db/pipeline/pipeline_d.cpp b/src/mongo/db/pipeline/pipeline_d.cpp index f56359d761f..f7f1dcf4a7f 100644 --- a/src/mongo/db/pipeline/pipeline_d.cpp +++ b/src/mongo/db/pipeline/pipeline_d.cpp @@ -708,13 +708,20 @@ Status PipelineD::MongoDInterface::attachCursorSourceToPipeline( if (expCtx->uuid) { try { autoColl.emplace(expCtx->opCtx, - NamespaceStringOrUUID{expCtx->ns.db().toString(), *expCtx->uuid}); + NamespaceStringOrUUID{expCtx->ns.db().toString(), *expCtx->uuid}, + AutoGetCollection::ViewMode::kViewsForbidden, + Date_t::max(), + AutoStatsTracker::LogMode::kUpdateTop); } catch (const ExceptionFor<ErrorCodes::NamespaceNotFound>& ex) { // The UUID doesn't exist anymore return ex.toStatus(); } } else { - autoColl.emplace(expCtx->opCtx, expCtx->ns); + autoColl.emplace(expCtx->opCtx, + expCtx->ns, + AutoGetCollection::ViewMode::kViewsForbidden, + Date_t::max(), + AutoStatsTracker::LogMode::kUpdateTop); } // makePipeline() is only called to perform secondary aggregation requests and expects the diff --git a/src/mongo/db/query/find.cpp b/src/mongo/db/query/find.cpp index b7805881be2..ae14be74be9 100644 --- a/src/mongo/db/query/find.cpp +++ b/src/mongo/db/query/find.cpp @@ -269,7 +269,11 @@ Message getMore(OperationContext* opCtx, const auto profilingLevel = autoDb.getDb() ? boost::optional<int>{autoDb.getDb()->getProfilingLevel()} : boost::none; - statsTracker.emplace(opCtx, *nssForCurOp, Top::LockType::NotLocked, profilingLevel); + statsTracker.emplace(opCtx, + *nssForCurOp, + Top::LockType::NotLocked, + AutoStatsTracker::LogMode::kUpdateTopAndCurop, + profilingLevel); auto view = autoDb.getDb() ? autoDb.getDb()->getViewCatalog()->lookup(opCtx, nssForCurOp->ns()) : nullptr; @@ -287,6 +291,7 @@ Message getMore(OperationContext* opCtx, statsTracker.emplace(opCtx, nss, Top::LockType::ReadLocked, + AutoStatsTracker::LogMode::kUpdateTopAndCurop, readLock->getDb() ? readLock->getDb()->getProfilingLevel() : doNotChangeProfilingLevel); Collection* collection = readLock->getCollection(); |