Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-45515

[4.2] renameCollectionForApplyOps can perform untimestamped writes to the catalog

    • Type: Icon: Bug Bug
    • Resolution: Duplicate
    • Priority: Icon: Major - P3 Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • Labels:
      None
    • ALL
    • Hide

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

      Unable to find source-code formatter for language: diff. Available languages are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml
      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();
               });
      

       

      Show
      Run resmoke.py --suite=replica_sets jstests/replsets/apply_ops_idempotency.js  with the following patch: Unable to find source-code formatter for language: diff. Available languages are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml 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(); });  
    • Execution Team 2020-02-24
    • 39

      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.

            Assignee:
            daniel.gottlieb@mongodb.com Daniel Gottlieb (Inactive)
            Reporter:
            louis.williams@mongodb.com Louis Williams
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: