SERVER-66145 Prevent operations from writing with read tickets

Since we do not support ticket upgrades, this effectively bans global lock upgrades

2 files changed, 27 insertions, 28 deletions

diff --git a/src/mongo/db/storage/kv/durable_catalog_test.cpp b/src/mongo/db/storage/kv/durable_catalog_test.cpp
index 9d419d50eb6..9bf2a31e610 100644
--- a/src/mongo/db/storage/kv/durable_catalog_test.cpp
+++ b/src/mongo/db/storage/kv/durable_catalog_test.cpp
@@ -94,6 +94,7 @@ public:
 
     CollectionCatalogIdAndUUID createCollection(const NamespaceString& nss,
                                                 CollectionOptions options) {
+        Lock::GlobalWrite lk(operationContext());
         Lock::DBLock dbLk(operationContext(), nss.dbName(), MODE_IX);
         Lock::CollectionLock collLk(operationContext(), nss, MODE_IX);
 
@@ -116,7 +117,6 @@ public:
             getCatalog()->getMetaData(operationContext(), catalogId),
             std::move(coll.second));
 
-        Lock::GlobalWrite lk(operationContext());
         CollectionCatalog::write(operationContext(), [&](CollectionCatalog& catalog) {
             catalog.registerCollection(operationContext(),
                                        options.uuid.value(),
diff --git a/src/mongo/db/storage/kv/storage_engine_test.cpp b/src/mongo/db/storage/kv/storage_engine_test.cpp
index c3f5ae88e31..0e6d85e82dd 100644
--- a/src/mongo/db/storage/kv/storage_engine_test.cpp
+++ b/src/mongo/db/storage/kv/storage_engine_test.cpp
@@ -122,10 +122,10 @@ TEST_F(StorageEngineTest, TemporaryRecordStoreClustered) {
     auto opCtx = cc().makeOperationContext();
 
     Lock::GlobalLock lk(&*opCtx, MODE_IS);
-
-    const auto trs = makeTemporaryClustered(opCtx.get());
+    auto trs = makeTemporaryClustered(opCtx.get());
     ASSERT(trs.get());
-    const auto rs = trs->rs();
+
+    auto rs = trs->rs();
     ASSERT(identExists(opCtx.get(), rs->getIdent()));
 
     // Insert record with RecordId of KeyFormat::String.
@@ -146,13 +146,14 @@ TEST_F(StorageEngineTest, TemporaryRecordStoreClustered) {
 TEST_F(StorageEngineTest, ReconcileDropsTemporary) {
     auto opCtx = cc().makeOperationContext();
 
-    Lock::GlobalLock lk(&*opCtx, MODE_IS);
-
-    auto rs = makeTemporary(opCtx.get());
-    ASSERT(rs.get());
-    const std::string ident = rs->rs()->getIdent();
+    std::unique_ptr<TemporaryRecordStore> rs;
+    {
+        Lock::GlobalLock lk(&*opCtx, MODE_IS);
+        rs = makeTemporary(opCtx.get());
+        ASSERT(rs.get());
+    }
 
-    ASSERT(identExists(opCtx.get(), ident));
+    ASSERT(identExists(opCtx.get(), rs->rs()->getIdent()));
 
     // Reconcile will only drop temporary idents when starting up after an unclean shutdown.
     auto reconcileResult = unittest::assertGet(reconcileAfterUncleanShutdown(opCtx.get()));
@@ -161,25 +162,26 @@ TEST_F(StorageEngineTest, ReconcileDropsTemporary) {
     ASSERT_EQUALS(0UL, reconcileResult.indexBuildsToResume.size());
 
     // The storage engine is responsible for dropping its temporary idents.
-    ASSERT(!identExists(opCtx.get(), ident));
+    ASSERT(!identExists(opCtx.get(), rs->rs()->getIdent()));
 }
 
 TEST_F(StorageEngineTest, ReconcileKeepsTemporary) {
     auto opCtx = cc().makeOperationContext();
 
-    Lock::GlobalLock lk(&*opCtx, MODE_IS);
-
-    auto rs = makeTemporary(opCtx.get());
-    ASSERT(rs.get());
-    const std::string ident = rs->rs()->getIdent();
+    std::unique_ptr<TemporaryRecordStore> rs;
+    {
+        Lock::GlobalLock lk(&*opCtx, MODE_IS);
+        rs = makeTemporary(opCtx.get());
+        ASSERT(rs.get());
+    }
 
-    ASSERT(identExists(opCtx.get(), ident));
+    ASSERT(identExists(opCtx.get(), rs->rs()->getIdent()));
 
     auto reconcileResult = unittest::assertGet(reconcile(opCtx.get()));
     ASSERT_EQUALS(0UL, reconcileResult.indexesToRebuild.size());
     ASSERT_EQUALS(0UL, reconcileResult.indexBuildsToRestart.size());
 
-    ASSERT_FALSE(identExists(opCtx.get(), ident));
+    ASSERT_FALSE(identExists(opCtx.get(), rs->rs()->getIdent()));
 }
 
 class StorageEngineTimestampMonitorTest : public StorageEngineTest {
@@ -283,14 +285,14 @@ TEST_F(StorageEngineTest, ReconcileUnfinishedIndex) {
 TEST_F(StorageEngineTest, ReconcileUnfinishedBackgroundSecondaryIndex) {
     auto opCtx = cc().makeOperationContext();
 
-    Lock::GlobalLock lk(&*opCtx, MODE_IX);
-
     const NamespaceString ns("db.coll1");
     const std::string indexName("a_1");
 
     auto swCollInfo = createCollection(opCtx.get(), ns);
     ASSERT_OK(swCollInfo.getStatus());
 
+    Lock::GlobalLock lk(&*opCtx, MODE_IX);
+
     // Start a backgroundSecondary single-phase (i.e. no build UUID) index.
     const bool isBackgroundSecondaryBuild = true;
     const boost::optional<UUID> buildUUID = boost::none;
@@ -328,8 +330,6 @@ TEST_F(StorageEngineTest, ReconcileUnfinishedBackgroundSecondaryIndex) {
 TEST_F(StorageEngineTest, ReconcileTwoPhaseIndexBuilds) {
     auto opCtx = cc().makeOperationContext();
 
-    Lock::GlobalLock lk(&*opCtx, MODE_IX);
-
     const NamespaceString ns("db.coll1");
     const std::string indexA("a_1");
     const std::string indexB("b_1");
@@ -337,6 +337,8 @@ TEST_F(StorageEngineTest, ReconcileTwoPhaseIndexBuilds) {
     auto swCollInfo = createCollection(opCtx.get(), ns);
     ASSERT_OK(swCollInfo.getStatus());
 
+    Lock::GlobalLock lk(&*opCtx, MODE_IX);
+
     // Using a build UUID implies that this index build is two-phase, so the isBackgroundSecondary
     // field will be ignored. There is no special behavior on primaries or secondaries.
     auto buildUUID = UUID::gen();
@@ -451,6 +453,7 @@ TEST_F(StorageEngineRepairTest, LoadCatalogRecoversOrphansInCatalog) {
     ASSERT_OK(swCollInfo.getStatus());
     ASSERT(collectionExists(opCtx.get(), collNs));
 
+    Lock::GlobalWrite writeLock(opCtx.get(), Date_t::max(), Lock::InterruptBehavior::kThrow);
     AutoGetDb db(opCtx.get(), collNs.dbName(), LockMode::MODE_X);
     // Only drop the catalog entry; storage engine still knows about this ident.
     // This simulates an unclean shutdown happening between dropping the catalog entry and
@@ -464,11 +467,7 @@ TEST_F(StorageEngineRepairTest, LoadCatalogRecoversOrphansInCatalog) {
     ASSERT(!collectionExists(opCtx.get(), collNs));
 
     // When in a repair context, loadCatalog() recreates catalog entries for orphaned idents.
-    {
-        Lock::GlobalWrite writeLock(opCtx.get(), Date_t::max(), Lock::InterruptBehavior::kThrow);
-        _storageEngine->loadCatalog(
-            opCtx.get(), boost::none, StorageEngine::LastShutdownState::kClean);
-    }
+    _storageEngine->loadCatalog(opCtx.get(), boost::none, StorageEngine::LastShutdownState::kClean);
     auto identNs = swCollInfo.getValue().ident;
     std::replace(identNs.begin(), identNs.end(), '-', '_');
     NamespaceString orphanNs = NamespaceString("local.orphan." + identNs);
@@ -488,11 +487,11 @@ TEST_F(StorageEngineTest, LoadCatalogDropsOrphans) {
     ASSERT_OK(swCollInfo.getStatus());
     ASSERT(collectionExists(opCtx.get(), collNs));
 
-    AutoGetDb db(opCtx.get(), collNs.dbName(), LockMode::MODE_X);
     // Only drop the catalog entry; storage engine still knows about this ident.
     // This simulates an unclean shutdown happening between dropping the catalog entry and
     // the actual drop in storage engine.
     {
+        AutoGetDb db(opCtx.get(), collNs.dbName(), LockMode::MODE_X);
         WriteUnitOfWork wuow(opCtx.get());
         ASSERT_OK(removeEntry(opCtx.get(), collNs.ns(), _storageEngine->getCatalog()));
         wuow.commit();