From a1c67941bf08c69cab04eba20bc9ce9a763e1c7f Mon Sep 17 00:00:00 2001 From: Geert Bosch Date: Sat, 15 Jul 2017 13:53:13 -0400 Subject: Revert "SERVER-30106 Allow syncTail replication to write to drop-pending collections" This reverts commit d8afe01c37aa24bef255e8aea4ec6882df71b1dd. --- src/mongo/db/cloner.cpp | 2 +- src/mongo/db/concurrency/lock_state.cpp | 2 - src/mongo/db/namespace_string.cpp | 54 +++++++++++------------ src/mongo/db/namespace_string.h | 11 +++-- src/mongo/db/query/get_executor.cpp | 50 ++++++++++++++------- src/mongo/db/repl/databases_cloner.cpp | 2 +- src/mongo/db/repl/storage_interface_impl.cpp | 2 +- src/mongo/db/repl/storage_interface_impl_test.cpp | 4 +- 8 files changed, 71 insertions(+), 56 deletions(-) (limited to 'src') diff --git a/src/mongo/db/cloner.cpp b/src/mongo/db/cloner.cpp index 7c26f24ed88..42081c94b79 100644 --- a/src/mongo/db/cloner.cpp +++ b/src/mongo/db/cloner.cpp @@ -539,7 +539,7 @@ StatusWith> Cloner::filterCollectionsForClone( const NamespaceString ns(opts.fromDB, collectionName.c_str()); if (ns.isSystem()) { - if (!ns.isLegalClientSystemNS()) { + if (legalClientSystemNS(ns.ns()) == 0) { LOG(2) << "\t\t not cloning because system collection"; continue; } diff --git a/src/mongo/db/concurrency/lock_state.cpp b/src/mongo/db/concurrency/lock_state.cpp index 721067e3aad..710de4ac234 100644 --- a/src/mongo/db/concurrency/lock_state.cpp +++ b/src/mongo/db/concurrency/lock_state.cpp @@ -505,8 +505,6 @@ bool LockerImpl::isCollectionLockedForMode(StringData ns, LockMode const ResourceId resIdDb(RESOURCE_DATABASE, nss.db()); LockMode dbMode = getLockMode(resIdDb); - if (!shouldConflictWithSecondaryBatchApplication()) - return true; switch (dbMode) { case MODE_NONE: diff --git a/src/mongo/db/namespace_string.cpp b/src/mongo/db/namespace_string.cpp index eaf1fab39ff..b9cedd47a95 100644 --- a/src/mongo/db/namespace_string.cpp +++ b/src/mongo/db/namespace_string.cpp @@ -79,6 +79,33 @@ constexpr auto dropPendingNSPrefix = "system.drop."_sd; } // namespace +bool legalClientSystemNS(StringData ns) { + if (ns == "local.system.replset") + return true; + + if (ns.find(".system.users") != string::npos) + return true; + + if (ns == "admin.system.roles") + return true; + if (ns == kServerConfiguration) + return true; + if (ns == kLogicalTimeKeysCollection) + return true; + if (ns == "admin.system.new_users") + return true; + if (ns == "admin.system.backup_users") + return true; + + if (ns.find(".system.js") != string::npos) + return true; + + if (nsToCollectionSubstring(ns) == NamespaceString::kSystemDotViewsCollectionName) + return true; + + return false; +} + constexpr StringData NamespaceString::kAdminDb; constexpr StringData NamespaceString::kLocalDb; constexpr StringData NamespaceString::kConfigDb; @@ -102,33 +129,6 @@ bool NamespaceString::isCollectionlessAggregateNS() const { return coll() == collectionlessAggregateCursorCol; } -bool NamespaceString::isLegalClientSystemNS() const { - if (db() == "admin") { - if (ns() == "admin.system.roles") - return true; - if (ns() == kServerConfiguration) - return true; - if (ns() == kLogicalTimeKeysCollection) - return true; - if (ns() == "admin.system.new_users") - return true; - if (ns() == "admin.system.backup_users") - return true; - } - if (ns() == "local.system.replset") - return true; - - if (coll() == "system.users") - return true; - if (coll() == "system.js") - return true; - - if (coll() == kSystemDotViewsCollectionName) - return true; - - return false; -} - NamespaceString NamespaceString::makeListCollectionsNSS(StringData dbName) { NamespaceString nss(dbName, listCollectionsCursorCol); dassert(nss.isValid()); diff --git a/src/mongo/db/namespace_string.h b/src/mongo/db/namespace_string.h index 5572eaab886..24abea86771 100644 --- a/src/mongo/db/namespace_string.h +++ b/src/mongo/db/namespace_string.h @@ -43,6 +43,11 @@ namespace mongo { const size_t MaxDatabaseNameLen = 128; // max str len for the db name, including null char +/** @return true if a client can modify this namespace even though it is under ".system." + For example .system.users is ok for regular clients to update. +*/ +bool legalClientSystemNS(StringData ns); + /* e.g. NamespaceString ns("acme.orders"); cout << ns.coll; // "orders" @@ -221,12 +226,6 @@ public: bool isListCollectionsCursorNS() const; bool isListIndexesCursorNS() const; - /** - * Returns true if a client can modify this namespace even though it is under ".system." - * For example .system.users is ok for regular clients to update. - */ - bool isLegalClientSystemNS() const; - /** * Given a NamespaceString for which isGloballyManagedNamespace() returns true, returns the * namespace the command targets, or boost::none for commands like 'listCollections' which diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp index bb2d6a6b68e..5367ce9c82b 100644 --- a/src/mongo/db/query/get_executor.cpp +++ b/src/mongo/db/query/get_executor.cpp @@ -696,8 +696,8 @@ StatusWith> getExecutorDelete( const NamespaceString& nss(request->getNamespaceString()); if (!request->isGod()) { - if (nss.isSystem() && opCtx->lockState()->shouldConflictWithSecondaryBatchApplication()) { - uassert(12050, "cannot delete from system namespace", nss.isLegalClientSystemNS()); + if (nss.isSystem()) { + uassert(12050, "cannot delete from system namespace", legalClientSystemNS(nss.ns())); } if (nss.isVirtualized()) { log() << "cannot delete from a virtual collection: " << nss; @@ -824,21 +824,33 @@ StatusWith> getExecutorDelete( // Update // +namespace { + +// TODO: Make this a function on NamespaceString, or make it cleaner. +inline void validateUpdate(const char* ns, const BSONObj& updateobj, const BSONObj& patternOrig) { + uassert(10155, "cannot update reserved $ collection", strchr(ns, '$') == 0); + if (strstr(ns, ".system.")) { + /* dm: it's very important that system.indexes is never updated as IndexDetails + has pointers into it */ + uassert(10156, + str::stream() << "cannot update system collection: " << ns << " q: " << patternOrig + << " u: " + << updateobj, + legalClientSystemNS(ns)); + } +} + +} // namespace + StatusWith> getExecutorUpdate( OperationContext* opCtx, OpDebug* opDebug, Collection* collection, ParsedUpdate* parsedUpdate) { const UpdateRequest* request = parsedUpdate->getRequest(); UpdateDriver* driver = parsedUpdate->getDriver(); - const NamespaceString& nss = request->getNamespaceString(); + const NamespaceString& nsString = request->getNamespaceString(); UpdateLifecycle* lifecycle = request->getLifecycle(); - if (nss.isSystem() && opCtx->lockState()->shouldConflictWithSecondaryBatchApplication()) { - uassert(10156, "cannot update a system namespace", nss.isLegalClientSystemNS()); - } - if (nss.isVirtualized()) { - log() << "cannot update a virtual collection: " << nss; - uasserted(10155, "cannot update a virtual collection"); - } + validateUpdate(nsString.ns().c_str(), request->getUpdates(), request->getQuery()); // If there is no collection and this is an upsert, callers are supposed to create // the collection prior to calling this method. Explain, however, will never do @@ -858,11 +870,11 @@ StatusWith> getExecutorUpdate( // writes on a secondary. If this is an update to a secondary from the replication system, // however, then we make an exception and let the write proceed. bool userInitiatedWritesAndNotPrimary = opCtx->writesAreReplicated() && - !repl::getGlobalReplicationCoordinator()->canAcceptWritesFor(opCtx, nss); + !repl::getGlobalReplicationCoordinator()->canAcceptWritesFor(opCtx, nsString); if (userInitiatedWritesAndNotPrimary) { return Status(ErrorCodes::PrimarySteppedDown, - str::stream() << "Not primary while performing update on " << nss.ns()); + str::stream() << "Not primary while performing update on " << nsString.ns()); } if (lifecycle) { @@ -884,11 +896,12 @@ StatusWith> getExecutorUpdate( // Treat collections that do not exist as empty collections. Note that the explain // reporting machinery always assumes that the root stage for an update operation is // an UpdateStage, so in this case we put an UpdateStage on top of an EOFStage. - LOG(2) << "Collection " << nss.ns() << " does not exist." + LOG(2) << "Collection " << nsString.ns() << " does not exist." << " Using EOF stage: " << redact(unparsedQuery); auto updateStage = make_unique( opCtx, updateStageParams, ws.get(), collection, new EOFStage(opCtx)); - return PlanExecutor::make(opCtx, std::move(ws), std::move(updateStage), nss, policy); + return PlanExecutor::make( + opCtx, std::move(ws), std::move(updateStage), nsString, policy); } const IndexDescriptor* descriptor = collection->getIndexCatalog()->findIdIndex(opCtx); @@ -944,8 +957,13 @@ StatusWith> getExecutorUpdate( // is invalid to use a positional projection because the query expression need not // match the array element after the update has been applied. const bool allowPositional = request->shouldReturnOldDocs(); - StatusWith> projStatus = applyProjection( - opCtx, nss, cq.get(), request->getProj(), allowPositional, ws.get(), std::move(root)); + StatusWith> projStatus = applyProjection(opCtx, + nsString, + cq.get(), + request->getProj(), + allowPositional, + ws.get(), + std::move(root)); if (!projStatus.isOK()) { return projStatus.getStatus(); } diff --git a/src/mongo/db/repl/databases_cloner.cpp b/src/mongo/db/repl/databases_cloner.cpp index d03f7252c8b..74c4543da21 100644 --- a/src/mongo/db/repl/databases_cloner.cpp +++ b/src/mongo/db/repl/databases_cloner.cpp @@ -321,7 +321,7 @@ void DatabasesCloner::_onListDatabaseFinish(const CommandCallbackArgs& cbd) { const auto collectionFilterPred = [dbName](const BSONObj& collInfo) { const auto collName = collInfo["name"].str(); const NamespaceString ns(dbName, collName); - if (ns.isSystem() && !ns.isLegalClientSystemNS()) { + if (ns.isSystem() && !legalClientSystemNS(ns.ns())) { LOG(1) << "Skipping 'system' collection: " << ns.ns(); return false; } diff --git a/src/mongo/db/repl/storage_interface_impl.cpp b/src/mongo/db/repl/storage_interface_impl.cpp index 672a2eb16ea..0dcb15a0110 100644 --- a/src/mongo/db/repl/storage_interface_impl.cpp +++ b/src/mongo/db/repl/storage_interface_impl.cpp @@ -830,7 +830,7 @@ Status StorageInterfaceImpl::deleteByFilter(OperationContext* opCtx, request.setMulti(true); request.setYieldPolicy(PlanExecutor::NO_YIELD); - // This disables the isLegalClientSystemNS() check in getExecutorDelete() which is used to + // This disables the legalClientSystemNS() check in getExecutorDelete() which is used to // disallow client deletes from unrecognized system collections. request.setGod(); diff --git a/src/mongo/db/repl/storage_interface_impl_test.cpp b/src/mongo/db/repl/storage_interface_impl_test.cpp index 825402ef879..70e801e2fe3 100644 --- a/src/mongo/db/repl/storage_interface_impl_test.cpp +++ b/src/mongo/db/repl/storage_interface_impl_test.cpp @@ -1844,7 +1844,7 @@ TEST_F(StorageInterfaceImplTest, // Checks that we can update collections with namespaces not considered "legal client system" // namespaces. NamespaceString nss("local.system.rollback.docs"); - ASSERT_FALSE(nss.isLegalClientSystemNS()); + ASSERT_FALSE(legalClientSystemNS(nss.ns())); auto opCtx = getOperationContext(); StorageInterfaceImpl storage; @@ -2088,7 +2088,7 @@ TEST_F(StorageInterfaceImplTest, DeleteByFilterRemovesDocumentsInIllegalClientSy // Checks that we can remove documents from collections with namespaces not considered "legal // client system" namespaces. NamespaceString nss("local.system.rollback.docs"); - ASSERT_FALSE(nss.isLegalClientSystemNS()); + ASSERT_FALSE(legalClientSystemNS(nss.ns())); auto opCtx = getOperationContext(); StorageInterfaceImpl storage; -- cgit v1.2.1