diff options
author | Devin Hilly <devin.hilly@mongodb.com> | 2018-11-13 17:55:50 -0500 |
---|---|---|
committer | Devin Hilly <devin.hilly@mongodb.com> | 2018-11-20 11:24:41 -0500 |
commit | 1862b00862a6ea9c3fdd08bba52ea6a016eccbc5 (patch) | |
tree | e0004fc325c86b0e4465fc602dd0d453df83bbec | |
parent | b92c7480df5406cb9487228af36cb10bb973e77c (diff) | |
download | mongo-1862b00862a6ea9c3fdd08bba52ea6a016eccbc5.tar.gz |
SERVER-31098 Wrong ns in system.profile for aggregation query
-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/create_indexes.cpp | 7 | ||||
-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/process_interface_standalone.cpp | 11 | ||||
-rw-r--r-- | src/mongo/db/query/find.cpp | 7 |
12 files changed, 113 insertions, 22 deletions
diff --git a/buildscripts/resmokeconfig/suites/aggregation_mongos_passthrough.yml b/buildscripts/resmokeconfig/suites/aggregation_mongos_passthrough.yml index 14151b4ea4f..bc58810c38a 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 3f31669bd9f..97177e9e11c 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 83817f9039e..e73d0ddeb4b 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..a6c07b910a0 --- /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); + + let 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. + let profileDoc = db.system.profile.findOne(); + assert.eq("test.local", profileDoc.ns); + + // Confirm that the local collection had one command added to Top. + let 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/create_indexes.cpp b/src/mongo/db/commands/create_indexes.cpp index ead1b508e4e..9b0cfae466d 100644 --- a/src/mongo/db/commands/create_indexes.cpp +++ b/src/mongo/db/commands/create_indexes.cpp @@ -345,7 +345,12 @@ public: // Use AutoStatsTracker to update Top. boost::optional<AutoStatsTracker> statsTracker; const boost::optional<int> dbProfilingLevel = boost::none; - statsTracker.emplace(opCtx, ns, Top::LockType::WriteLocked, dbProfilingLevel); + statsTracker.emplace(opCtx, + ns, + Top::LockType::WriteLocked, + AutoStatsTracker::LogMode::kUpdateTopAndCurop, + dbProfilingLevel); + MultiIndexBlockImpl indexer(opCtx, collection); indexer.allowBackgroundBuilding(); diff --git a/src/mongo/db/commands/getmore_cmd.cpp b/src/mongo/db/commands/getmore_cmd.cpp index ff1748d6c83..768c22831df 100644 --- a/src/mongo/db/commands/getmore_cmd.cpp +++ b/src/mongo/db/commands/getmore_cmd.cpp @@ -283,8 +283,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); @@ -292,6 +295,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 fb846d38b6a..dc381aae471 100644 --- a/src/mongo/db/commands/killcursors_cmd.cpp +++ b/src/mongo/db/commands/killcursors_cmd.cpp @@ -83,8 +83,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 98fc17718fc..affce5b9600 100644 --- a/src/mongo/db/commands/run_aggregate.cpp +++ b/src/mongo/db/commands/run_aggregate.cpp @@ -425,7 +425,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 b195e4ce5d0..2d3279dc41e 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()), @@ -257,11 +260,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 abc8b2280fe..39a30282630 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/process_interface_standalone.cpp b/src/mongo/db/pipeline/process_interface_standalone.cpp index 949f6a7c195..19b9b6c0588 100644 --- a/src/mongo/db/pipeline/process_interface_standalone.cpp +++ b/src/mongo/db/pipeline/process_interface_standalone.cpp @@ -305,13 +305,20 @@ Status MongoInterfaceStandalone::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 bdeeb907c45..a4ad2ad5d91 100644 --- a/src/mongo/db/query/find.cpp +++ b/src/mongo/db/query/find.cpp @@ -272,7 +272,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; @@ -290,6 +294,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(); |