SERVER-46287: Switched background validation to use PIT read instead of checkpoint cursor

delete mode 100644 jstests/noPassthrough/background_validation_checkpoint_existence.js
 create mode 100644 jstests/noPassthrough/background_validation_repl.js

jstests/noPassthrough/background_validation_checkpoint_existence.js

@@ -1,62 +0,0 @@
-/**
- * Tests that validating a collection in the background that has not been checkpointed yet does no
- * validation. In addition, ensures that background validation skips indexes that are not yet
- * checkpointed.
- *
- * @tags: [requires_fsync, requires_wiredtiger, requires_persistence]
- */
-(function() {
-'use strict';
-
-// To prevent the checkpoint thread from running during this test, change its frequency to the
-// largest possible value using the 'syncdelay' parameter.
-const kMaxSyncDelaySecs = 9 * 1000 * 1000;
-let conn = MongoRunner.runMongod({syncdelay: kMaxSyncDelaySecs});
-assert.neq(null, conn, "mongod was unable to start up");
-
-const dbName = "test";
-const collName = "background_validation_checkpoint_existence";
-
-const db = conn.getDB(dbName);
-
-const forceCheckpoint = () => {
-    assert.commandWorked(db.fsyncLock());
-    assert.commandWorked(db.fsyncUnlock());
-};
-
-assert.commandWorked(db.createCollection(collName));
-const coll = db.getCollection(collName);
-
-for (let i = 0; i < 5; i++) {
-    assert.commandWorked(coll.insert({x: i}));
-}
-
-// The collection has not been checkpointed yet, so there is nothing to validate.
-let res = assert.commandWorked(db.runCommand({validate: collName, background: true}));
-assert.eq(true, res.valid);
-assert.eq(false, res.hasOwnProperty("nrecords"));
-assert.eq(false, res.hasOwnProperty("nIndexes"));
-
-forceCheckpoint();
-
-res = assert.commandWorked(db.runCommand({validate: collName, background: true}));
-assert.eq(true, res.valid);
-assert.eq(true, res.hasOwnProperty("nrecords"));
-assert.eq(true, res.hasOwnProperty("nIndexes"));
-
-assert.commandWorked(coll.createIndex({x: 1}));
-
-// Shouldn't validate the newly created index here as it wasn't checkpointed yet.
-res = assert.commandWorked(db.runCommand({validate: collName, background: true}));
-assert.eq(true, res.valid);
-assert.eq(1, res.nIndexes);
-
-forceCheckpoint();
-
-// Validating after the checkpoint should validate the newly created index.
-res = assert.commandWorked(db.runCommand({validate: collName, background: true}));
-assert.eq(true, res.valid);
-assert.eq(2, res.nIndexes);
-
-MongoRunner.stopMongod(conn);
-}());

jstests/noPassthrough/background_validation_repl.js

@@ -0,0 +1,138 @@
+/**
+ * Tests the validate command with {background:true} in a replica set.
+ *
+ * Checks that {full:true} cannot be run with {background:true}.
+ * Checks that {background:true} runs.
+ * Checks that {background:true} can run concurrently with CRUD ops on the same collection.
+ *
+ * @tags: [
+ *   # Background validation is only supported by WT.
+ *   requires_wiredtiger,
+ *   # inMemory does not have checkpoints; background validation only runs on a checkpoint.
+ *   requires_persistence,
+ *   # A failpoint is set that only exists on the mongod.
+ *   assumes_against_mongod_not_mongos,
+ *   # A failpoint is set against the primary only.
+ *   does_not_support_stepdowns,
+ *   # Checkpoint cursors cannot be open in lsm.
+ *   does_not_support_wiredtiger_lsm,
+ *   # Background validation will be first available in v4.4.
+ *   requires_fcv_44,
+ *   requires_replication,
+ * ]
+ */
+
+(function() {
+'use strict';
+
+load("jstests/libs/fail_point_util.js");
+load("jstests/core/txns/libs/prepare_helpers.js");
+
+const dbName = "db_background_validation_repl";
+const collName = "coll_background_validation_repl";
+
+// Starts and returns a replica set.
+const initTest = () => {
+    const replSet = new ReplSetTest({nodes: 1, name: "rs"});
+    replSet.startSet();
+    replSet.initiate();
+    const primary = replSet.getPrimary();
+
+    let testColl = primary.getDB(dbName)[collName];
+    testColl.drop();
+    return replSet;
+};
+
+const doTest = replSet => {
+    /*
+     * Create some indexes and insert some data, so we can validate them more meaningfully.
+     */
+    const testDB = replSet.getPrimary().getDB(dbName);
+    const testColl = testDB[collName];
+    assert.commandWorked(testColl.createIndex({a: 1}));
+    assert.commandWorked(testColl.createIndex({b: 1}));
+    assert.commandWorked(testColl.createIndex({c: 1}));
+
+    const numDocs = 100;
+    for (let i = 0; i < numDocs; ++i) {
+        assert.commandWorked(testColl.insert({a: i, b: i, c: i}));
+    }
+
+    /**
+     * Ensure {full:true} and {background:true} cannot be run together.
+     */
+    assert.commandFailedWithCode(testColl.validate({background: true, full: true}),
+                                 ErrorCodes.CommandNotSupported);
+
+    assert.commandWorked(testDB.adminCommand({fsync: 1}));
+
+    // Check that {backround:true} is successful.
+    let res = testColl.validate({background: true});
+    assert.commandWorked(res);
+    assert(res.valid, "Validate cmd with {background:true} failed: " + tojson(res));
+
+    /*
+     * Test background validation with concurrent CRUD operations.
+     */
+
+    // Set a failpoint in the background validation code to pause validation while holding a
+    // collection lock.
+    let failPoint = configureFailPoint(testDB, "pauseCollectionValidationWithLock");
+
+    jsTest.log(`Starting parallel shell on port ${replSet.getPrimary().port}`);
+    // Start an asynchronous thread to run collection validation with {background:true}.
+    // Ensure we can perform multiple collection validations on the same collection
+    // concurrently.
+    let awaitValidateCommand = startParallelShell(function() {
+        const asyncTestDB = db.getSiblingDB("db_background_validation_repl");
+        const asyncTestColl = asyncTestDB.coll_background_validation_repl;
+        const validateRes = asyncTestColl.validate({background: true});
+        assert.commandWorked(
+            validateRes, "asynchronous background validate command failed: " + tojson(validateRes));
+        assert(validateRes.valid,
+               "asynchronous background validate command was not valid: " + tojson(validateRes));
+    }, replSet.getPrimary().port);
+
+    // Wait for background validation command to start.
+    jsTest.log("Waiting for failpoint to hit...");
+    failPoint.wait();
+
+    // Check that CRUD ops are succesful while validation is in progress.
+    assert.commandWorked(testColl.remove({a: 1, b: 1, c: 1}));
+    assert.commandWorked(testColl.insert({a: 1, b: 1, c: 1, d: 100}));
+    assert.commandWorked(testColl.update({d: 100}, {"e": "updated"}));
+    let docRes = testColl.find({"e": "updated"});
+    assert.eq(1,
+              docRes.toArray().length,
+              "expected to find a single document, found: " + tojson(docRes.toArray()));
+
+    // Clear the failpoint and make sure the validate command was successful.
+    failPoint.off();
+    awaitValidateCommand();
+
+    /**
+     * Verify everything is still OK by running foreground validation.
+     */
+    res = testColl.validate({background: false});
+    assert.commandWorked(res);
+    assert(res.valid, "Validate cmd with {background:true} failed: " + tojson(res));
+    assert.eq(res.nIndexes, 4, "Expected 4 indexes: " + tojson(res));
+    assert.eq(res.nrecords, numDocs, "Expected " + numDocs + " collection records:" + tojson(res));
+    assert.eq(res.keysPerIndex._id_,
+              numDocs,
+              "Expected " + numDocs + " _id index records: " + tojson(res));
+    assert.eq(res.keysPerIndex.a_1,
+              numDocs,
+              "Expected " + numDocs + " a_1 index records: " + tojson(res));
+    assert.eq(res.keysPerIndex.b_1,
+              numDocs,
+              "Expected " + numDocs + " b_1 index records: " + tojson(res));
+    assert.eq(res.keysPerIndex.c_1,
+              numDocs,
+              "Expected " + numDocs + " c_1 index records: " + tojson(res));
+};
+
+const replSet = initTest();
+doTest(replSet);
+replSet.stopSet();
+})();

src/mongo/db/catalog/validate_state.cpp

@@ -59,6 +59,10 @@ ValidateState::ValidateState(OperationContext* opCtx,
 
     // Subsequent re-locks will use the UUID when 'background' is true.
     if (_background) {
+        // We need to hold the global lock throughout the entire validation to avoid having to save
+        // and restore our cursors used throughout. This is done in order to avoid abandoning the
+        // snapshot and invalidating our cursors.
+        _globalLock.emplace(opCtx, MODE_IS);
         _databaseLock.emplace(opCtx, _nss.db(), MODE_IS);
         _collectionLock.emplace(opCtx, _nss, MODE_IS);
     } else {
@@ -138,18 +142,22 @@ void ValidateState::initializeCursors(OperationContext* opCtx) {
     invariant(!_traverseRecordStoreCursor && !_seekRecordStoreCursor && _indexCursors.size() == 0 &&
               _indexes.size() == 0);
 
-    // Background validation will read from the last stable checkpoint instead of the latest data.
-    // This allows concurrent writes to go ahead without interfering with validation's view of the
-    // data. The checkpoint lock must be taken around cursor creation to ensure all cursors point at
-    // the same checkpoint, i.e. a consistent view of the collection data.
-    std::unique_ptr<StorageEngine::CheckpointLock> checkpointCursorsLock;
+    // Background validation will read from a snapshot opened on the all durable timestamp instead
+    // of the latest data. This allows concurrent writes to go ahead without interfering with
+    // validation's view of the data.
     if (_background) {
         invariant(!opCtx->lockState()->isCollectionLockedForMode(_nss, MODE_X));
-        auto storageEngine = opCtx->getServiceContext()->getStorageEngine();
-        invariant(storageEngine->supportsCheckpoints());
         opCtx->recoveryUnit()->abandonSnapshot();
-        opCtx->recoveryUnit()->setTimestampReadSource(RecoveryUnit::ReadSource::kCheckpoint);
-        checkpointCursorsLock = storageEngine->getCheckpointLock(opCtx);
+        // Background validation is expecting to read from the all durable timestamp, but
+        // standalones do not support timestamps. Therefore, if this process is currently running as
+        // a standalone, don't use a timestamp.
+        RecoveryUnit::ReadSource rs;
+        if (repl::ReplicationCoordinator::get(opCtx)->isReplEnabled()) {
+            rs = RecoveryUnit::ReadSource::kAllDurableSnapshot;
+        } else {
+            rs = RecoveryUnit::ReadSource::kNoTimestamp;
+        }
+        opCtx->recoveryUnit()->setTimestampReadSource(rs);
     }
 
     // We want to share the same data throttle instance across all the cursors used during this
@@ -159,105 +167,24 @@ void ValidateState::initializeCursors(OperationContext* opCtx) {
         _dataThrottle.turnThrottlingOff();
     }
 
-    try {
-        _traverseRecordStoreCursor = std::make_unique<SeekableRecordThrottleCursor>(
-            opCtx, _collection->getRecordStore(), &_dataThrottle);
-        _seekRecordStoreCursor = std::make_unique<SeekableRecordThrottleCursor>(
-            opCtx, _collection->getRecordStore(), &_dataThrottle);
-    } catch (const ExceptionFor<ErrorCodes::CursorNotFound>& ex) {
-        invariant(_background);
-        // End the validation if we can't open a checkpoint cursor on the collection.
-        LOGV2(20405,
-              "Skipping background validation on collection '{nss}' because the collection is not "
-              "yet in a checkpoint: {ex}",
-              "nss"_attr = _nss,
-              "ex"_attr = ex);
-        throw;
-    }
-
-    std::vector<std::string> readyDurableIndexes;
-    try {
-        DurableCatalog::get(opCtx)->getReadyIndexes(
-            opCtx, _collection->getCatalogId(), &readyDurableIndexes);
-    } catch (const ExceptionFor<ErrorCodes::CursorNotFound>& ex) {
-        invariant(_background);
-        LOGV2(20406,
-              "Skipping background validation on collection '{nss}' because the data is not yet in "
-              "a checkpoint: {ex}",
-              "nss"_attr = _nss,
-              "ex"_attr = ex);
-        throw;
-    }
+    _traverseRecordStoreCursor = std::make_unique<SeekableRecordThrottleCursor>(
+        opCtx, _collection->getRecordStore(), &_dataThrottle);
+    _seekRecordStoreCursor = std::make_unique<SeekableRecordThrottleCursor>(
+        opCtx, _collection->getRecordStore(), &_dataThrottle);
 
     const IndexCatalog* indexCatalog = _collection->getIndexCatalog();
+    // The index iterator for ready indexes is timestamp-aware and will only return indexes that
+    // are visible at our read time.
     const std::unique_ptr<IndexCatalog::IndexIterator> it =
         indexCatalog->getIndexIterator(opCtx, /*includeUnfinished*/ false);
     while (it->more()) {
         const IndexCatalogEntry* entry = it->next();
         const IndexDescriptor* desc = entry->descriptor();
 
-        // Filter out any in-memory index in the collection that is not in our PIT view of the MDB
-        // catalog. This is only important when background:true because we are then reading from the
-        // checkpoint's view of the MDB catalog and data.
-        bool isIndexDurable =
-            std::find(readyDurableIndexes.begin(), readyDurableIndexes.end(), desc->indexName()) !=
-            readyDurableIndexes.end();
-        if (_background && !isIndexDurable) {
-            LOGV2(20407,
-                  "Skipping validation on index '{desc_indexName}' in collection '{nss}' because "
-                  "the index is not yet in a checkpoint.",
-                  "desc_indexName"_attr = desc->indexName(),
-                  "nss"_attr = _nss);
-            continue;
-        }
+        _indexCursors.emplace(desc->indexName(),
+                              std::make_unique<SortedDataInterfaceThrottleCursor>(
+                                  opCtx, entry->accessMethod(), &_dataThrottle));
 
-        // Read the index's ident from disk (the checkpoint if background:true). If it does not
-        // match the in-memory ident saved in the IndexCatalogEntry, then our PIT view of the index
-        // is old and the index has been dropped and recreated. In this case we will skip it since
-        // there is no utility in checking a dropped index (we also cannot currently access it
-        // because its in-memory representation is gone).
-        auto diskIndexIdent =
-            opCtx->getServiceContext()->getStorageEngine()->getCatalog()->getIndexIdent(
-                opCtx, _collection->getCatalogId(), desc->indexName());
-        if (entry->getIdent() != diskIndexIdent) {
-            LOGV2(20408,
-                  "Skipping validation on index '{desc_indexName}' in collection '{nss}' because "
-                  "the index was recreated and is not yet in a checkpoint.",
-                  "desc_indexName"_attr = desc->indexName(),
-                  "nss"_attr = _nss);
-            continue;
-        }
-
-        try {
-            _indexCursors.emplace(desc->indexName(),
-                                  std::make_unique<SortedDataInterfaceThrottleCursor>(
-                                      opCtx, entry->accessMethod(), &_dataThrottle));
-        } catch (const ExceptionFor<ErrorCodes::CursorNotFound>& ex) {
-            invariant(_background);
-            // This can only happen if the checkpoint has the MDB catalog entry for the index, but
-            // not the corresponding index table.
-            LOGV2(20409,
-                  "Skipping validation on index '{desc_indexName}' in collection '{nss}' because "
-                  "the index data is not in a checkpoint: {ex}",
-                  "desc_indexName"_attr = desc->indexName(),
-                  "nss"_attr = _nss,
-                  "ex"_attr = ex);
-            continue;
-        }
-
-        // Skip any newly created indexes that, because they were built with a WT bulk loader, are
-        // checkpoint'ed but not yet consistent with the rest of checkpoint's PIT view of the data.
-        if (_background &&
-            opCtx->getServiceContext()->getStorageEngine()->isInIndividuallyCheckpointedIndexesList(
-                diskIndexIdent)) {
-            _indexCursors.erase(desc->indexName());
-            LOGV2(20410,
-                  "Skipping validation on index '{desc_indexName}' in collection '{nss}' because "
-                  "the index data is not yet consistent in the checkpoint.",
-                  "desc_indexName"_attr = desc->indexName(),
-                  "nss"_attr = _nss);
-            continue;
-        }
 
         _indexes.push_back(indexCatalog->getEntryShared(desc));
     }

src/mongo/db/catalog/validate_state.h

@@ -172,6 +172,7 @@ private:
     ValidateOptions _options;
     OptionalCollectionUUID _uuid;
 
+    boost::optional<Lock::GlobalLock> _globalLock;
     boost::optional<AutoGetDb> _databaseLock;
     boost::optional<Lock::CollectionLock> _collectionLock;
 

src/mongo/db/catalog/validate_state_test.cpp

@@ -123,8 +123,7 @@ TEST_F(ValidateStateTest, NonExistentCollectionShouldThrowNamespaceNotFoundError
                        ErrorCodes::NamespaceNotFound);
 }
 
-// Validate with {background:true} should fail to find an uncheckpoint'ed collection.
-TEST_F(ValidateStateTest, UncheckpointedCollectionShouldThrowCursorNotFoundError) {
+TEST_F(ValidateStateTest, UncheckpointedCollectionShouldBeAbleToInitializeCursors) {
     auto opCtx = operationContext();
 
     // Disable periodic checkpoint'ing thread so we can control when checkpoints occur.
@@ -133,13 +132,13 @@ TEST_F(ValidateStateTest, UncheckpointedCollectionShouldThrowCursorNotFoundError
     // Checkpoint of all of the data.
     opCtx->recoveryUnit()->waitUntilUnjournaledWritesDurable(opCtx, /*stableCheckpoint*/ false);
 
-    // Create the collection, which will not be in the checkpoint, and check that a CursorNotFound
-    // error is thrown when attempting to open cursors.
     createCollectionAndPopulateIt(opCtx, kNss);
     CollectionValidation::ValidateState validateState(
         opCtx, kNss, /*background*/ true, CollectionValidation::ValidateOptions::kNoFullValidation);
-    ASSERT_THROWS_CODE(
-        validateState.initializeCursors(opCtx), AssertionException, ErrorCodes::CursorNotFound);
+    // Assert that cursors are able to created on the new collection.
+    validateState.initializeCursors(opCtx);
+    // There should only be a first record id if cursors were initialized successfully.
+    ASSERT(!validateState.getFirstRecordId().isNull());
 }
 
 // Basic test with {background:false} to open cursors against all collection indexes.
@@ -191,8 +190,8 @@ TEST_F(ValidateStateTest, OpenCursorsOnAllIndexes) {
     ASSERT_EQ(validateState.getIndexes().size(), 5);
 }
 
-// Open cursors against checkpoint'ed indexes with {background:true}.
-TEST_F(ValidateStateTest, OpenCursorsOnCheckpointedIndexes) {
+// Open cursors against all indexes with {background:true}.
+TEST_F(ValidateStateTest, OpenCursorsOnAllIndexesWithBackground) {
     // Disable index build commit quorum as we don't have support of replication subsystem for
     // voting.
     ASSERT_OK(ServerParameterSet::getGlobal()
@@ -220,58 +219,9 @@ TEST_F(ValidateStateTest, OpenCursorsOnCheckpointedIndexes) {
         opCtx, kNss, /*background*/ true, CollectionValidation::ValidateOptions::kNoFullValidation);
     validateState.initializeCursors(opCtx);
 
-    // Make sure the uncheckpoint'ed indexes are not found.
-    // (Note the _id index was create with collection creation, so we have 3 indexes.)
-    ASSERT_EQ(validateState.getIndexes().size(), 3);
-}
-
-// Only open cursors against indexes that are consistent with the rest of the checkpoint'ed data.
-TEST_F(ValidateStateTest, OpenCursorsOnConsistentlyCheckpointedIndexes) {
-    // Disable index build commit quorum as we don't have support of replication subsystem for
-    // voting.
-    ASSERT_OK(ServerParameterSet::getGlobal()
-                  ->getMap()
-                  .find("enableIndexBuildCommitQuorum")
-                  ->second->setFromString("false"));
-
-    auto opCtx = operationContext();
-    Collection* coll = createCollectionAndPopulateIt(opCtx, kNss);
-
-    // Disable periodic checkpoint'ing thread so we can control when checkpoints occur.
-    FailPointEnableBlock failPoint("pauseCheckpointThread");
-
-    // Create several indexes.
-    createIndex(opCtx, kNss, BSON("a" << 1));
-    createIndex(opCtx, kNss, BSON("b" << 1));
-    createIndex(opCtx, kNss, BSON("c" << 1));
-    createIndex(opCtx, kNss, BSON("d" << 1));
-
-    // Checkpoint the indexes.
-    opCtx->recoveryUnit()->waitUntilUnjournaledWritesDurable(opCtx, /*stableCheckpoint*/ false);
-
-    {
-        // Artificially set two indexes as inconsistent with the checkpoint.
-        AutoGetCollection autoColl(opCtx, kNss, MODE_IS);
-        auto checkpointLock =
-            opCtx->getServiceContext()->getStorageEngine()->getCheckpointLock(opCtx);
-        auto indexIdentA =
-            opCtx->getServiceContext()->getStorageEngine()->getCatalog()->getIndexIdent(
-                opCtx, coll->getCatalogId(), "a_1");
-        auto indexIdentB =
-            opCtx->getServiceContext()->getStorageEngine()->getCatalog()->getIndexIdent(
-                opCtx, coll->getCatalogId(), "b_1");
-        opCtx->getServiceContext()->getStorageEngine()->addIndividuallyCheckpointedIndexToList(
-            indexIdentA);
-        opCtx->getServiceContext()->getStorageEngine()->addIndividuallyCheckpointedIndexToList(
-            indexIdentB);
-    }
-
-    // The two inconsistent indexes should not be found.
-    // (Note the _id index was create with collection creation, so we have 3 indexes.)
-    CollectionValidation::ValidateState validateState(
-        opCtx, kNss, /*background*/ true, CollectionValidation::ValidateOptions::kNoFullValidation);
-    validateState.initializeCursors(opCtx);
-    ASSERT_EQ(validateState.getIndexes().size(), 3);
+    // We should be able to open a cursor on each index.
+    // (Note the _id index was create with collection creation, so we have 5 indexes.)
+    ASSERT_EQ(validateState.getIndexes().size(), 5);
 }
 
 // Indexes in the checkpoint that were dropped in the present should not have cursors opened against