diff options
author | Dianna Hohensee <dianna.hohensee@10gen.com> | 2018-07-03 17:44:44 -0400 |
---|---|---|
committer | Dianna Hohensee <dianna.hohensee@10gen.com> | 2018-07-13 19:50:40 -0400 |
commit | 9184a03574c398b087b929fda8ed428f0c64d28c (patch) | |
tree | ce18b03959d5945c9ee10ae056fbb640c48c6649 | |
parent | 93accf1620db95d4aa7edbd5bbc017264b28e0f2 (diff) | |
download | mongo-9184a03574c398b087b929fda8ed428f0c64d28c.tar.gz |
SERVER-35085 Hide the visibility of UUIDCatalog changes during repairDatabase cmdr4.1.1
Added RAII object around UUIDCatalog::onCloseCatalog and UUIDCatalog::onOpenCatalog
-rw-r--r-- | src/mongo/db/catalog/catalog_control.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/catalog/uuid_catalog.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/catalog/uuid_catalog.h | 13 | ||||
-rw-r--r-- | src/mongo/db/catalog/uuid_catalog_test.cpp | 12 | ||||
-rw-r--r-- | src/mongo/db/catalog_raii.cpp | 10 | ||||
-rw-r--r-- | src/mongo/db/catalog_raii.h | 26 | ||||
-rw-r--r-- | src/mongo/db/commands/dbcommands.cpp | 20 | ||||
-rw-r--r-- | src/mongo/db/concurrency/locker_noop.h | 2 |
8 files changed, 73 insertions, 22 deletions
diff --git a/src/mongo/db/catalog/catalog_control.cpp b/src/mongo/db/catalog/catalog_control.cpp index de4baef54ce..3abc562103b 100644 --- a/src/mongo/db/catalog/catalog_control.cpp +++ b/src/mongo/db/catalog/catalog_control.cpp @@ -68,12 +68,12 @@ MinVisibleTimestampMap closeCatalog(OperationContext* opCtx) { } // Need to mark the UUIDCatalog as open if we our closeAll fails, dismissed if successful. - auto reopenOnFailure = MakeGuard([opCtx] { UUIDCatalog::get(opCtx).onOpenCatalog(); }); + auto reopenOnFailure = MakeGuard([opCtx] { UUIDCatalog::get(opCtx).onOpenCatalog(opCtx); }); // Closing UUID Catalog: only lookupNSSByUUID will fall back to using pre-closing state to // allow authorization for currently unknown UUIDs. This is needed because authorization needs // to work before acquiring locks, and might otherwise spuriously regard a UUID as unknown // while reloading the catalog. - UUIDCatalog::get(opCtx).onCloseCatalog(); + UUIDCatalog::get(opCtx).onCloseCatalog(opCtx); LOG(1) << "closeCatalog: closing UUID catalog"; // Close all databases. @@ -205,7 +205,7 @@ void openCatalog(OperationContext* opCtx, const MinVisibleTimestampMap& minVisib } // Opening UUID Catalog: The UUID catalog is now in sync with the storage engine catalog. Clear // the pre-closing state. - UUIDCatalog::get(opCtx).onOpenCatalog(); + UUIDCatalog::get(opCtx).onOpenCatalog(opCtx); LOG(1) << "openCatalog: finished reloading UUID catalog"; } } // namespace catalog diff --git a/src/mongo/db/catalog/uuid_catalog.cpp b/src/mongo/db/catalog/uuid_catalog.cpp index 781c0748fb2..cdc01f63d32 100644 --- a/src/mongo/db/catalog/uuid_catalog.cpp +++ b/src/mongo/db/catalog/uuid_catalog.cpp @@ -161,7 +161,8 @@ void UUIDCatalog::onCloseDatabase(Database* db) { } } -void UUIDCatalog::onCloseCatalog() { +void UUIDCatalog::onCloseCatalog(OperationContext* opCtx) { + invariant(opCtx->lockState()->isW()); stdx::lock_guard<stdx::mutex> lock(_catalogLock); invariant(!_shadowCatalog); _shadowCatalog.emplace(); @@ -169,7 +170,8 @@ void UUIDCatalog::onCloseCatalog() { _shadowCatalog->insert({entry.first, entry.second->ns()}); } -void UUIDCatalog::onOpenCatalog() { +void UUIDCatalog::onOpenCatalog(OperationContext* opCtx) { + invariant(opCtx->lockState()->isW()); stdx::lock_guard<stdx::mutex> lock(_catalogLock); invariant(_shadowCatalog); _shadowCatalog.reset(); diff --git a/src/mongo/db/catalog/uuid_catalog.h b/src/mongo/db/catalog/uuid_catalog.h index 7b1ace95701..a3fc453e07d 100644 --- a/src/mongo/db/catalog/uuid_catalog.h +++ b/src/mongo/db/catalog/uuid_catalog.h @@ -181,15 +181,20 @@ public: /** * Puts the catalog in closed state. In this state, the lookupNSSByUUID method will fall back - * to the pre-close state to resolve queries for currently unknown UUIDs. This allows - * authorization, which needs to do lookups outside of database locks, to proceed. + * to the pre-close state to resolve queries for currently unknown UUIDs. This allows processes, + * like authorization and replication, which need to do lookups outside of database locks, to + * proceed. + * + * Must be called with the global lock acquired in exclusive mode. */ - void onCloseCatalog(); + void onCloseCatalog(OperationContext* opCtx); /** * Puts the catatlog back in open state, removing the pre-close state. See onCloseCatalog. + * + * Must be called with the global lock acquired in exclusive mode. */ - void onOpenCatalog(); + void onOpenCatalog(OperationContext* opCtx); /** * Return the UUID lexicographically preceding `uuid` in the database named by `db`. diff --git a/src/mongo/db/catalog/uuid_catalog_test.cpp b/src/mongo/db/catalog/uuid_catalog_test.cpp index 097aea9c029..3db12976675 100644 --- a/src/mongo/db/catalog/uuid_catalog_test.cpp +++ b/src/mongo/db/catalog/uuid_catalog_test.cpp @@ -200,11 +200,11 @@ TEST_F(UUIDCatalogTest, InvalidateOrdering) { } TEST_F(UUIDCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsOldNSSIfDropped) { - catalog.onCloseCatalog(); + catalog.onCloseCatalog(&opCtx); catalog.onDropCollection(&opCtx, colUUID); ASSERT(catalog.lookupCollectionByUUID(colUUID) == nullptr); ASSERT_EQUALS(catalog.lookupNSSByUUID(colUUID), nss); - catalog.onOpenCatalog(); + catalog.onOpenCatalog(&opCtx); ASSERT_EQUALS(catalog.lookupNSSByUUID(colUUID), NamespaceString()); } @@ -214,7 +214,7 @@ TEST_F(UUIDCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsNewlyCreatedNSS) { Collection newCol(stdx::make_unique<CollectionMock>(newNss)); // Ensure that looking up non-existing UUIDs doesn't affect later registration of those UUIDs. - catalog.onCloseCatalog(); + catalog.onCloseCatalog(&opCtx); ASSERT(catalog.lookupCollectionByUUID(newUUID) == nullptr); ASSERT(catalog.lookupNSSByUUID(newUUID) == NamespaceString()); catalog.onCreateCollection(&opCtx, &newCol, newUUID); @@ -222,7 +222,7 @@ TEST_F(UUIDCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsNewlyCreatedNSS) { ASSERT_EQUALS(catalog.lookupNSSByUUID(colUUID), nss); // Ensure that collection still exists after opening the catalog again. - catalog.onOpenCatalog(); + catalog.onOpenCatalog(&opCtx); ASSERT_EQUALS(catalog.lookupCollectionByUUID(newUUID), &newCol); ASSERT_EQUALS(catalog.lookupNSSByUUID(colUUID), nss); } @@ -230,7 +230,7 @@ TEST_F(UUIDCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsNewlyCreatedNSS) { TEST_F(UUIDCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsFreshestNSS) { NamespaceString newNss(nss.db(), "newcol"); Collection newCol(stdx::make_unique<CollectionMock>(newNss)); - catalog.onCloseCatalog(); + catalog.onCloseCatalog(&opCtx); catalog.onDropCollection(&opCtx, colUUID); ASSERT(catalog.lookupCollectionByUUID(colUUID) == nullptr); ASSERT_EQUALS(catalog.lookupNSSByUUID(colUUID), nss); @@ -239,7 +239,7 @@ TEST_F(UUIDCatalogTest, LookupNSSByUUIDForClosedCatalogReturnsFreshestNSS) { ASSERT_EQUALS(catalog.lookupNSSByUUID(colUUID), newNss); // Ensure that collection still exists after opening the catalog again. - catalog.onOpenCatalog(); + catalog.onOpenCatalog(&opCtx); ASSERT_EQUALS(catalog.lookupCollectionByUUID(colUUID), &newCol); ASSERT_EQUALS(catalog.lookupNSSByUUID(colUUID), newNss); } diff --git a/src/mongo/db/catalog_raii.cpp b/src/mongo/db/catalog_raii.cpp index 162e96020a2..00c2344bf2b 100644 --- a/src/mongo/db/catalog_raii.cpp +++ b/src/mongo/db/catalog_raii.cpp @@ -184,4 +184,14 @@ AutoGetOrCreateDb::AutoGetOrCreateDb(OperationContext* opCtx, DatabaseShardingState::get(_db).checkDbVersion(opCtx); } +ConcealUUIDCatalogChangesBlock::ConcealUUIDCatalogChangesBlock(OperationContext* opCtx) + : _opCtx(opCtx) { + UUIDCatalog::get(_opCtx).onCloseCatalog(_opCtx); +} + +ConcealUUIDCatalogChangesBlock::~ConcealUUIDCatalogChangesBlock() { + invariant(_opCtx); + UUIDCatalog::get(_opCtx).onOpenCatalog(_opCtx); +} + } // namespace mongo diff --git a/src/mongo/db/catalog_raii.h b/src/mongo/db/catalog_raii.h index 1d0b1e9238f..f8998e405cc 100644 --- a/src/mongo/db/catalog_raii.h +++ b/src/mongo/db/catalog_raii.h @@ -187,4 +187,30 @@ private: bool _justCreated{false}; }; +/** + * RAII-style class. Hides changes to the UUIDCatalog For the life of the object so that calls to + * UUIDCatalog::lookupNSSByUUID will return results as before the RAII object was instantiated. + * + * The caller must hold the global exclusive lock for the life of the instance. + */ +class ConcealUUIDCatalogChangesBlock { + MONGO_DISALLOW_COPYING(ConcealUUIDCatalogChangesBlock); + +public: + /** + * Conceals future UUIDCatalog changes and stashes a pointer to the opCtx for the destructor to + * use. + */ + ConcealUUIDCatalogChangesBlock(OperationContext* opCtx); + + /** + * Reveals UUIDCatalog changes. + */ + ~ConcealUUIDCatalogChangesBlock(); + +private: + // Needed for the destructor to access the UUIDCatalog in order to call onOpenCatalog. + OperationContext* _opCtx; +}; + } // namespace mongo diff --git a/src/mongo/db/commands/dbcommands.cpp b/src/mongo/db/commands/dbcommands.cpp index 90d2cd2d5f0..56d28ebb53a 100644 --- a/src/mongo/db/commands/dbcommands.cpp +++ b/src/mongo/db/commands/dbcommands.cpp @@ -247,13 +247,21 @@ public: "backupOriginalFiles not supported", !cmdObj.getField("backupOriginalFiles").trueValue()); - StorageEngine* engine = getGlobalServiceContext()->getStorageEngine(); - repl::UnreplicatedWritesBlock uwb(opCtx); - Status status = repairDatabase(opCtx, engine, dbname); + { + // Conceal UUIDCatalog changes for the duration of repairDatabase so that calls to + // UUIDCatalog::lookupNSSByUUID do not cause spurious NamespaceNotFound errors while + // repairDatabase makes updates. + ConcealUUIDCatalogChangesBlock cucc(opCtx); + + StorageEngine* engine = getGlobalServiceContext()->getStorageEngine(); + repl::UnreplicatedWritesBlock uwb(opCtx); + Status status = repairDatabase(opCtx, engine, dbname); + + // Open database before returning + DatabaseHolder::getDatabaseHolder().openDb(opCtx, dbname); + uassertStatusOK(status); + } - // Open database before returning - DatabaseHolder::getDatabaseHolder().openDb(opCtx, dbname); - uassertStatusOK(status); return true; } } cmdRepairDatabase; diff --git a/src/mongo/db/concurrency/locker_noop.h b/src/mongo/db/concurrency/locker_noop.h index 51d8e3dd2f3..6b9ac5227f5 100644 --- a/src/mongo/db/concurrency/locker_noop.h +++ b/src/mongo/db/concurrency/locker_noop.h @@ -189,7 +189,7 @@ public: } virtual bool isW() const { - MONGO_UNREACHABLE; + return true; } virtual bool isR() const { |