SERVER-97174 Validate command on secondaries reads unfinished oplog batch (#32610)

GitOrigin-RevId: 6f2b790441d0ad5256a8c81aee297a3f53514aa5

Author: clarissecheah

jstests/noPassthrough/validate/validate_oplog_batch_applier_sync.js

@@ -0,0 +1,70 @@
+/**
+ * Tests that foreground validation on the secondary occurs in between batch boundaries of oplog
+ * application and that trying to proceed with validation while we have yet to finish applying a
+ * batch will result in validate being blocked due to waiting for a lock.
+ */
+
+import {configureFailPoint} from "jstests/libs/fail_point_util.js";
+import {ReplSetTest} from "jstests/libs/replsettest.js";
+
+const rst = new ReplSetTest({nodes: 2});
+rst.startSet();
+rst.initiate();
+
+const primary = rst.getPrimary();
+const secondary = rst.getSecondary();
+
+const dbName = "test_validation";
+const collName = "test_coll_validation";
+const primaryDB = primary.getDB(dbName);
+const secDB = secondary.getDB(dbName);
+const primaryColl = primaryDB.getCollection(collName);
+
+/*
+ * Create some indexes and insert some data, so we can validate them more meaningfully.
+ */
+assert.commandWorked(primaryColl.createIndex({a: 1}));
+assert.commandWorked(primaryColl.createIndex({b: 1}));
+assert.commandWorked(primaryColl.createIndex({c: 1}));
+
+const numDocs = 10;
+for (let i = 0; i < numDocs; ++i) {
+    assert.commandWorked(primaryColl.insert({a: i, b: i, c: i}));
+}
+
+rst.awaitReplication();
+/* Enable the failpoint to stop oplog batch application before completion. */
+let failPoint = "pauseBatchApplicationBeforeCompletion";
+const fp = configureFailPoint(secDB, failPoint);
+
+// Start oplog batch application.
+const awaitOplog = startParallelShell(function() {
+    const primaryDB = db.getSiblingDB('test_validation');
+    const coll = primaryDB.getCollection('test_coll_validation');
+    assert.commandWorked(coll.insert({a: 1, b: 1, c: 1}, {writeConcern: {w: 2}}));
+}, primary.port);
+
+fp.wait();
+
+// Start validation on the secondary on a parallel shell.
+const awaitValidate = startParallelShell(function() {
+    const secDB = db.getSiblingDB('test_validation');
+    const coll = secDB.getCollection('test_coll_validation');
+    secDB.setSecondaryOk();
+    const res = coll.validate();
+    assert.commandWorked(res);
+    assert(res.valid, "Validate cmd failed: " + tojson(res));
+}, secondary.port);
+
+// Ensure that the validation call is blocked on waiting for the lock.
+assert.soon(() => {
+    return secDB.currentOp().inprog.some(op => {
+        return op.active && (op.command.validate === "test_coll_validation") && (op.waitingForLock);
+    });
+});
+
+fp.off();
+awaitOplog();
+awaitValidate();
+
+rst.stopSet();

src/mongo/db/catalog/validate/validate_state.cpp

@@ -69,6 +69,9 @@ ValidateState::ValidateState(OperationContext* opCtx,
                              const NamespaceString& nss,
                              ValidationOptions options)
     : ValidationOptions(std::move(options)),
+      _validateLock(isBackground()
+                        ? boost::none
+                        : boost::optional<Lock::SharedLock>{obtainSharedValidationLock(opCtx)}),
       _globalLock(opCtx, isBackground() ? MODE_IS : MODE_IX),
       _nss(nss),
       _dataThrottle(opCtx, [&]() { return gMaxValidateMBperSec.load(); }) {
@@ -224,6 +227,9 @@ Status ValidateState::initializeCollection(OperationContext* opCtx) {
         _dataThrottle.turnThrottlingOff();
     }
 
+    // We can release the validate lock here as we now have exclusive access to the collection and
+    // no CRUD operations or fast count changes will occur.
+    _validateLock.reset();
     return Status::OK();
 }
 
@@ -258,5 +264,26 @@ void ValidateState::initializeCursors(OperationContext* opCtx) {
     _firstRecordId = record ? std::move(record->id) : RecordId();
 }
 
+namespace {
+/*
+ * Oplog Batch Applier takes this lock in exclusive mode when applying the
+ * batch. Foreground validation waits on this lock to begin validation.
+ * We must synchronise these operations as foreground validation involves opening a snapshot of the
+ * most recent data and during oplog application, CRUD operations on the document are performed in a
+ * transaction separate from the fast count updates. This could potentially lead to validation
+ * opening a snapshot between these two transactions and result in an incorrectly reported fast
+ * count discrepancy.
+ */
+Lock::ResourceMutex validateLock("validateLock");
+}  // namespace
+
+Lock::ExclusiveLock ValidateState::obtainExclusiveValidationLock(OperationContext* opCtx) {
+    return Lock::ExclusiveLock(opCtx, validateLock);
+}
+
+Lock::SharedLock ValidateState::obtainSharedValidationLock(OperationContext* opCtx) {
+    return Lock::SharedLock(opCtx, validateLock);
+}
+
 }  // namespace CollectionValidation
 }  // namespace mongo

src/mongo/db/catalog/validate/validate_state.h

@@ -141,9 +141,28 @@ class ValidateState : public ValidationOptions {
         return _validateTs;
     }
 
+    /**
+     * Returns a scoped object, which holds the 'validateLock' in exclusive mode for
+     * the given scope. It must only be used to coordinate validation with concurrent
+     * oplog batch applications.
+     */
+    static Lock::ExclusiveLock obtainExclusiveValidationLock(OperationContext* opCtx);
+
+    /**
+     * Returns a scoped object, which holds the 'validateLock' in shared mode for
+     * the given scope. It must only be used to coordinate validation with concurrent
+     * oplog batch applications.
+     */
+    static Lock::SharedLock obtainSharedValidationLock(OperationContext* opCtx);
+
 private:
     ValidateState() = delete;
 
+    // This lock needs to be obtained before the global lock. Initialise in the validation
+    // constructor. Oplog Batch Applier takes this lock in exclusive mode when applying the batch.
+    // Foreground validation waits on this lock to begin.
+    boost::optional<Lock::SharedLock> _validateLock;
+
     // To avoid racing with shutdown/rollback, this lock must be initialized early.
     Lock::GlobalLock _globalLock;
 

src/mongo/db/repl/oplog_applier_impl.cpp

@@ -51,6 +51,7 @@
 #include "mongo/bson/bsonobj.h"
 #include "mongo/bson/timestamp.h"
 #include "mongo/db/admission/execution_admission_context.h"
+#include "mongo/db/catalog/validate/validate_state.h"
 #include "mongo/db/client.h"
 #include "mongo/db/collection_crud/collection_write_path.h"
 #include "mongo/db/commands/fsync.h"
@@ -565,6 +566,9 @@ void OplogApplierImpl::_run(OplogBuffer* oplogBuffer) {
         // Don't allow the fsync+lock thread to see intermediate states of batch application.
         stdx::lock_guard<stdx::mutex> fsynclk(oplogApplierLockedFsync);
 
+        // Obtain the validation lock to synchronise batch application with validation.
+        auto lk = CollectionValidation::ValidateState::obtainExclusiveValidationLock(&opCtx);
+
         // Apply the operations in this batch. '_applyOplogBatch' returns the optime of the
         // last op that was applied, which should be the last optime in the batch.
         auto swLastOpTimeAppliedInBatch = _applyOplogBatch(&opCtx, ops.releaseBatch());