[SERVER-45515] [4.2] renameCollectionForApplyOps can perform untimestamped writes to the catalog Created: 13/Jan/20  Updated: 18/Feb/20  Resolved: 18/Feb/20

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Louis Williams Assignee: Daniel Gottlieb (Inactive)
Resolution: Duplicate Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
Duplicate
Related
Operating System: ALL
Steps To Reproduce:

Run resmoke.py --suite=replica_sets jstests/replsets/apply_ops_idempotency.js with the following patch:

diff --git a/src/mongo/db/catalog/index_timestamp_helper.cpp b/src/mongo/db/catalog/index_timestamp_helper.cpp
index ceecf0b838..8686e7af8f 100644
--- a/src/mongo/db/catalog/index_timestamp_helper.cpp
+++ b/src/mongo/db/catalog/index_timestamp_helper.cpp
@@ -102,13 +102,12 @@ void IndexTimestampHelper::setGhostCommitTimestampForWrite(OperationContext* opC
     fassert(51053, status);
 }
 
-/**
- * Returns true if writes to the catalog entry for the input namespace require being
- * timestamped. A ghost write is when the operation is not committed with an oplog entry and
- * implies the caller will look at the logical clock to choose a time to use.
- */
-namespace {
-bool requiresGhostCommitTimestampForCatalogWrite(OperationContext* opCtx, NamespaceString nss) {
+bool IndexTimestampHelper::requiresGhostCommitTimestampForCatalogWrite(OperationContext* opCtx,
+                                                                       NamespaceString nss) {
+    if (opCtx->writesAreReplicated()) {
+        return false;
+    }
+
     if (!nss.isReplicated() || nss.coll().startsWith("tmp.mr.")) {
         return false;
     }
@@ -144,7 +143,6 @@ bool requiresGhostCommitTimestampForCatalogWrite(OperationContext* opCtx, Namesp
 
     return true;
 }
-}  // namespace
 
 bool IndexTimestampHelper::setGhostCommitTimestampForCatalogWrite(OperationContext* opCtx,
                                                                   const NamespaceString& nss) {
diff --git a/src/mongo/db/catalog/index_timestamp_helper.h b/src/mongo/db/catalog/index_timestamp_helper.h
index 9ae4457e40..7953c8ef67 100644
--- a/src/mongo/db/catalog/index_timestamp_helper.h
+++ b/src/mongo/db/catalog/index_timestamp_helper.h
@@ -36,6 +36,13 @@ namespace mongo {
 
 namespace IndexTimestampHelper {
 
+/**
+ * Returns true if writes to the catalog entry for the input namespace require being
+ * timestamped. A ghost write is when the operation is not committed with an oplog entry and
+ * implies the caller will look at the logical clock to choose a time to use.
+ */
+bool requiresGhostCommitTimestampForCatalogWrite(OperationContext* opCtx, NamespaceString nss);
+
 /**
  * If required, sets a timestamp on an active WriteUnitOfWork. A ghost write is when the
  * operation is not committed with an oplog entry, which may be necessary for certain index
diff --git a/src/mongo/db/storage/SConscript b/src/mongo/db/storage/SConscript
index bff1205b15..af58c0c262 100644
--- a/src/mongo/db/storage/SConscript
+++ b/src/mongo/db/storage/SConscript
@@ -435,7 +435,8 @@ env.Library(
     ],
     LIBDEPS_PRIVATE=[
         '$BUILD_DIR/mongo/db/catalog/disable_index_spec_namespace_generation',
-    ]
+        '$BUILD_DIR/mongo/db/catalog/index_timestamp_helper',
+    ],
 )
 
 env.Library(
diff --git a/src/mongo/db/storage/durable_catalog_impl.cpp b/src/mongo/db/storage/durable_catalog_impl.cpp
index 8e3d75c664..9997da0ab9 100644
--- a/src/mongo/db/storage/durable_catalog_impl.cpp
+++ b/src/mongo/db/storage/durable_catalog_impl.cpp
@@ -36,6 +36,7 @@
 #include "mongo/bson/util/bson_extract.h"
 #include "mongo/bson/util/builder.h"
 #include "mongo/db/catalog/disable_index_spec_namespace_generation_gen.h"
+#include "mongo/db/catalog/index_timestamp_helper.h"
 #include "mongo/db/concurrency/d_concurrency.h"
 #include "mongo/db/index/index_descriptor.h"
 #include "mongo/db/namespace_string.h"
@@ -602,6 +603,11 @@ void DurableCatalogImpl::putMetaData(OperationContext* opCtx,
         obj = b.obj();
     }
 
+    if (IndexTimestampHelper::requiresGhostCommitTimestampForCatalogWrite(opCtx, nss) &&
+        !nss.isDropPendingNamespace()) {
+        opCtx->recoveryUnit()->setMustBeTimestamped();
+    }
+
     LOG(3) << "recording new metadata: " << obj;
     Status status = _rs->updateRecord(opCtx, loc, obj.objdata(), obj.objsize());
     fassert(28521, status.isOK());
@@ -643,6 +649,11 @@ Status DurableCatalogImpl::_replaceEntry(OperationContext* opCtx,
     _idents.erase(fromIt);
     _idents[toNss.toString()] = Entry(old["ident"].String(), loc);
 
+    if (IndexTimestampHelper::requiresGhostCommitTimestampForCatalogWrite(opCtx, fromNss) &&
+        !fromNss.isDropPendingNamespace()) {
+        opCtx->recoveryUnit()->setMustBeTimestamped();
+    }
+
     return Status::OK();
 }
 
diff --git a/src/mongo/db/storage/recovery_unit.h b/src/mongo/db/storage/recovery_unit.h
index cd640c82e2..91c6306769 100644
--- a/src/mongo/db/storage/recovery_unit.h
+++ b/src/mongo/db/storage/recovery_unit.h
@@ -509,8 +509,14 @@ public:
 
     virtual void setOrderedCommit(bool orderedCommit) = 0;
 
+    void setMustBeTimestamped() {
+        _mustBeTimestamped = true;
+    }
+
 protected:
     RecoveryUnit() {}
+
+    bool _mustBeTimestamped = false;
 };
 
 }  // namespace mongo
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_recovery_unit.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_recovery_unit.cpp
index ea78f02b27..26be848b02 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_recovery_unit.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_recovery_unit.cpp
@@ -397,6 +397,10 @@ void WiredTigerRecoveryUnit::_txnClose(bool commit) {
             conf << "durable_timestamp=" << integerToHex(_durableTimestamp.asULL());
         }
 
+        if (_mustBeTimestamped) {
+            invariant(_isTimestamped);
+        }
+
         wtRet = s->commit_transaction(s, conf.str().c_str());
         LOG(3) << "WT commit_transaction for snapshot id " << _mySnapshotId;
     } else {
@@ -435,6 +439,7 @@ void WiredTigerRecoveryUnit::_txnClose(bool commit) {
     _isOplogReader = false;
     _oplogVisibleTs = boost::none;
     _orderedCommit = true;  // Default value is true; we assume all writes are ordered.
+    _mustBeTimestamped = false;
 }
 
 SnapshotId WiredTigerRecoveryUnit::getSnapshotId() const {
diff --git a/src/mongo/dbtests/dbtests.cpp b/src/mongo/dbtests/dbtests.cpp
index 3a16b947f9..446906a7ed 100644
--- a/src/mongo/dbtests/dbtests.cpp
+++ b/src/mongo/dbtests/dbtests.cpp
@@ -106,7 +106,17 @@ Status createIndexFromSpec(OperationContext* opCtx, StringData ns, const BSONObj
     }
     MultiIndexBlock indexer;
     ON_BLOCK_EXIT([&] { indexer.cleanUpAfterBuild(opCtx, coll); });
-    Status status = indexer.init(opCtx, coll, spec, MultiIndexBlock::kNoopOnInitFn).getStatus();
+    Status status = indexer
+                        .init(opCtx,
+                              coll,
+                              spec,
+                              [opCtx](const std::vector<BSONObj>& specs) -> Status {
+                                  if (opCtx->recoveryUnit()->getCommitTimestamp().isNull()) {
+                                      return opCtx->recoveryUnit()->setTimestamp(Timestamp(1, 1));
+                                  }
+                                  return Status::OK();
+                              })
+                        .getStatus();
     if (status == ErrorCodes::IndexAlreadyExists) {
         return Status::OK();
     }
@@ -124,6 +134,7 @@ Status createIndexFromSpec(OperationContext* opCtx, StringData ns, const BSONObj
     WriteUnitOfWork wunit(opCtx);
     ASSERT_OK(indexer.commit(
         opCtx, coll, MultiIndexBlock::kNoopOnCreateEachFn, MultiIndexBlock::kNoopOnCommitFn));
+    ASSERT_OK(opCtx->recoveryUnit()->setTimestamp(Timestamp(1, 1)));
     wunit.commit();
     return Status::OK();
 }
diff --git a/src/mongo/dbtests/repltests.cpp b/src/mongo/dbtests/repltests.cpp
index ccb1e796b2..de0a6c1490 100644
--- a/src/mongo/dbtests/repltests.cpp
+++ b/src/mongo/dbtests/repltests.cpp
@@ -145,6 +145,8 @@ public:
 
         ASSERT(c->getIndexCatalog()->haveIdIndex(&_opCtx));
         wuow.commit();
+
+        _opCtx.getServiceContext()->getStorageEngine()->setOldestTimestamp(_lastSetOldestTimestamp);
     }
     ~Base() {
         // Replication is not supported by mobile SE.
@@ -239,6 +241,8 @@ protected:
                     if (auto tsElem = ops.front()["ts"]) {
                         _opCtx.getServiceContext()->getStorageEngine()->setOldestTimestamp(
                             tsElem.timestamp());
+                        _lastSetOldestTimestamp =
+                            std::max(_lastSetOldestTimestamp, tsElem.timestamp());
                     }
                 }
             }
@@ -305,6 +309,7 @@ protected:
             repl::UnreplicatedWritesBlock uwb(&_opCtx);
             coll->insertDocument(&_opCtx, InsertStatement(o), nullOpDebug, true)
                 .transitional_ignore();
+            ASSERT_OK(_opCtx.recoveryUnit()->setTimestamp(_lastSetOldestTimestamp));
             wunit.commit();
             return;
         }
@@ -317,6 +322,7 @@ protected:
         repl::UnreplicatedWritesBlock uwb(&_opCtx);
         coll->insertDocument(&_opCtx, InsertStatement(b.obj()), nullOpDebug, true)
             .transitional_ignore();
+        ASSERT_OK(_opCtx.recoveryUnit()->setTimestamp(_lastSetOldestTimestamp));
         wunit.commit();
     }
     static BSONObj wid(const char* json) {
@@ -327,6 +333,8 @@ protected:
         b.appendElements(fromjson(json));
         return b.obj();
     }
+
+    Timestamp _lastSetOldestTimestamp = Timestamp(1, 1);
 };
 
 
diff --git a/src/mongo/dbtests/storage_timestamp_tests.cpp b/src/mongo/dbtests/storage_timestamp_tests.cpp
index 8bfacc5a18..8f7e85c1ee 100644
--- a/src/mongo/dbtests/storage_timestamp_tests.cpp
+++ b/src/mongo/dbtests/storage_timestamp_tests.cpp
@@ -241,6 +241,9 @@ public:
 
             AutoGetOrCreateDb dbRaii(_opCtx, nss.db(), LockMode::MODE_X);
             WriteUnitOfWork wunit(_opCtx);
+            if (_opCtx->recoveryUnit()->getCommitTimestamp().isNull()) {
+                ASSERT_OK(_opCtx->recoveryUnit()->setTimestamp(Timestamp(1, 1)));
+            }
             invariant(dbRaii.getDb()->createCollection(_opCtx, nss));
             wunit.commit();
         });

 

Sprint: Execution Team 2020-02-24
Participants:
Linked BF Score: 39

 Description   

I encountered this issue attempting to backport SERVER-42497 to 4.2. I am not certain of about the cause. The renameCollection logic is very similar between 4.2 and master.



 Comments   
Comment by Louis Williams [ 13/Jan/20 ]

See this patch build.

Generated at Thu Feb 08 05:09:00 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.