SERVER-47866 Secondary readers do not need to reacquire PBWM lock if there are catalog conflicts

9 files changed, 142 insertions, 35 deletions

diff --git a/jstests/concurrency/fsm_workloads/snapshot_read_at_cluster_time_ddl_operations.js b/jstests/concurrency/fsm_workloads/snapshot_read_at_cluster_time_ddl_operations.js
index 07d1b578109..c6bf1e808bd 100644
--- a/jstests/concurrency/fsm_workloads/snapshot_read_at_cluster_time_ddl_operations.js
+++ b/jstests/concurrency/fsm_workloads/snapshot_read_at_cluster_time_ddl_operations.js
@@ -2,8 +2,7 @@
 
 /**
  * Perform point-in-time snapshot reads that span a 'find' and multiple 'getmore's concurrently with
- * CRUD operations. Index operations running concurrently with the snapshot read may cause
- * the read to fail with a SnapshotUnavailable error.
+ * CRUD operations.
  *
  * @tags: [creates_background_indexes, requires_fcv_47, requires_replication,
  * does_not_support_causal_consistency, requires_majority_read_concern]
@@ -17,7 +16,6 @@ var $config = (function() {
 
         snapshotScan: function snapshotScan(db, collName) {
             const readErrorCodes = [
-                ErrorCodes.SnapshotUnavailable,
                 ErrorCodes.ShutdownInProgress,
                 ErrorCodes.CursorNotFound,
                 ErrorCodes.QueryPlanKilled,
diff --git a/jstests/concurrency/fsm_workloads/snapshot_read_catalog_operations.js b/jstests/concurrency/fsm_workloads/snapshot_read_catalog_operations.js
index 87a0c2f0515..5b89d01d29f 100644
--- a/jstests/concurrency/fsm_workloads/snapshot_read_catalog_operations.js
+++ b/jstests/concurrency/fsm_workloads/snapshot_read_catalog_operations.js
@@ -5,8 +5,7 @@
  * snapshot reads and CRUD operations will all contend for locks on db and collName. Since the
  * snapshot read does not release its locks until the transaction is committed, it is expected that
  * once the read has begun, catalog operations with conflicting locks will block until the read is
- * finished. Additionally, index operations running concurrently with the snapshot read may cause
- * the read to fail with a SnapshotUnavailable error.
+ * finished.
  *
  * @tags: [creates_background_indexes, uses_transactions]
  */
@@ -32,7 +31,6 @@ var $config = (function() {
             const sortByAscending = sortOptions[Random.randInt(2)];
             const readErrorCodes = [
                 ErrorCodes.NoSuchTransaction,
-                ErrorCodes.SnapshotUnavailable,
                 ErrorCodes.SnapshotTooOld,
                 ErrorCodes.StaleChunkHistory,
                 ErrorCodes.LockTimeout,
diff --git a/jstests/libs/global_snapshot_reads_util.js b/jstests/libs/global_snapshot_reads_util.js
index 10f8f9ff74d..658141ebdbd 100644
--- a/jstests/libs/global_snapshot_reads_util.js
+++ b/jstests/libs/global_snapshot_reads_util.js
@@ -138,9 +138,17 @@ function SnapshotReadsTest({primaryDB, secondaryDB, awaitCommittedFn}) {
                     // This update is not visible to reads at insertTimestamp.
                     res = assert.commandWorked(primaryDB.runCommand(
                         {update: collName, updates: [{q: {}, u: {$set: {x: true}}, multi: true}]}));
-
                     jsTestLog(`Updated collection "${collName}" at timestamp ${
                         tojson(res.operationTime)}`);
+
+                    awaitCommittedFn(db, res.operationTime);
+
+                    // This index is not visible to reads at insertTimestamp and does not cause the
+                    // operation to fail.
+                    res = assert.commandWorked(primaryDB.runCommand(
+                        {createIndexes: collName, indexes: [{key: {x: 1}, name: 'x_1'}]}));
+                    jsTestLog(`Created an index on collection "${collName}" at timestamp ${
+                        tojson(res.operationTime)}`);
                     awaitCommittedFn(db, res.operationTime);
 
                     // Retrieve the rest of the read command's result set.
diff --git a/jstests/noPassthrough/index_stepdown_abort_prepare_conflict.js b/jstests/noPassthrough/index_stepdown_abort_prepare_conflict.js
index c47922aeb68..6b207866ed7 100644
--- a/jstests/noPassthrough/index_stepdown_abort_prepare_conflict.js
+++ b/jstests/noPassthrough/index_stepdown_abort_prepare_conflict.js
@@ -100,6 +100,7 @@ assert.commandWorked(newSession.abortTransaction_forTesting());
 
 IndexBuildTest.waitForIndexBuildToStop(newPrimary.getDB(dbName), collName, indexName);
 IndexBuildTest.waitForIndexBuildToStop(primary.getDB(dbName), collName, indexName);
+rst.awaitReplication();
 
 IndexBuildTest.assertIndexes(newPrimary.getDB(dbName).getCollection(collName), 1, ["_id_"], []);
 IndexBuildTest.assertIndexes(primaryColl, 1, ["_id_"], []);
diff --git a/jstests/noPassthrough/out_majority_read_replset.js b/jstests/noPassthrough/out_majority_read_replset.js
index 0ffba741b3e..4ca9bd83e0a 100644
--- a/jstests/noPassthrough/out_majority_read_replset.js
+++ b/jstests/noPassthrough/out_majority_read_replset.js
@@ -28,12 +28,22 @@ rst.awaitLastOpCommitted();
 
 stopReplicationOnSecondaries(rst);
 
-// Create the index that is not majority commited
-// This test create indexes with majority of nodes not available for replication. So, disabling
-// index build commit quorum.
+// Rename the collection temporarily and then back to its original name. This advances the minimum
+// visible snapshot and forces the $out to block until its snapshot advances.
+const tempColl = db.getName() + '.temp';
+assert.commandWorked(db.adminCommand({
+    renameCollection: sourceColl.getFullName(),
+    to: tempColl,
+}));
+assert.commandWorked(db.adminCommand({
+    renameCollection: tempColl,
+    to: sourceColl.getFullName(),
+}));
+
+// Create the index that is not majority committed
 assert.commandWorked(sourceColl.createIndex({state: 1}, {name: "secondIndex"}, 0));
 
-// Run the $out in the parallel shell as it will block in the metadata until the shapshot is
+// Run the $out in the parallel shell as it will block in the metadata until the snapshot is
 // advanced.
 const awaitShell = startParallelShell(`{
         const testDB = db.getSiblingDB("${name}");
@@ -58,7 +68,7 @@ assert.soon(function() {
     return assert.commandWorked(db.currentOp(filter)).inprog.length === 1;
 });
 
-// Restart data replicaiton and wait until the new write becomes visible.
+// Restart data replication and wait until the new write becomes visible.
 restartReplicationOnSecondaries(rst);
 rst.awaitLastOpCommitted();
 
diff --git a/jstests/noPassthrough/read_concern_snapshot_catalog_invalidation.js b/jstests/noPassthrough/read_concern_snapshot_catalog_invalidation.js
index b6cc5d4b21d..fb296a9d29e 100644
--- a/jstests/noPassthrough/read_concern_snapshot_catalog_invalidation.js
+++ b/jstests/noPassthrough/read_concern_snapshot_catalog_invalidation.js
@@ -42,13 +42,19 @@ function testCommand(cmd, curOpFilter) {
 
     waitForCurOpByFailPointNoNS(testDB, "hangAfterPreallocateSnapshot", curOpFilter);
 
-    // Create an index on the collection the command was executed against. This will move the
-    // collection's minimum visible timestamp to a point later than the point-in-time referenced
-    // by the transaction snapshot.
-    assert.commandWorked(testDB.runCommand({
-        createIndexes: kCollName,
-        indexes: [{key: {x: 1}, name: "x_1"}],
-        writeConcern: {w: "majority"}
+    // Rename the collection the command was executed against and then back to its original name.
+    // This will move the collection's minimum visible timestamp to a point later than the
+    // point-in-time referenced by the transaction snapshot.
+    const tempColl = testDB.getName() + '.temp';
+    assert.commandWorked(testDB.adminCommand({
+        renameCollection: testDB.getName() + '.' + kCollName,
+        to: tempColl,
+        writeConcern: {w: "majority"},
+    }));
+    assert.commandWorked(testDB.adminCommand({
+        renameCollection: tempColl,
+        to: testDB.getName() + '.' + kCollName,
+        writeConcern: {w: "majority"},
     }));
 
     // Disable the hang and check for parallel shell success. Success indicates that the command
diff --git a/jstests/noPassthrough/read_majority.js b/jstests/noPassthrough/read_majority.js
index 12a2c991641..227e04e7dc4 100644
--- a/jstests/noPassthrough/read_majority.js
+++ b/jstests/noPassthrough/read_majority.js
@@ -53,6 +53,13 @@ function testReadConcernLevel(level) {
         assert.eq(res.code, ErrorCodes.MaxTimeMSExpired);
     }
 
+    function assertNoSnapshotAvailableForReadConcernLevelByUUID(uuid) {
+        var res =
+            db.runCommand({find: uuid, batchSize: 2, readConcern: {level: level}, maxTimeMS: 1000});
+        assert.commandFailed(res);
+        assert.eq(res.code, ErrorCodes.MaxTimeMSExpired);
+    }
+
     function getCursorForReadConcernLevel() {
         var res = t.runCommand('find', {batchSize: 2, readConcern: {level: level}});
         assert.commandWorked(res);
@@ -145,30 +152,77 @@ function testReadConcernLevel(level) {
     assert.eq(cursor.next().version, 4);
     assert.eq(cursor.next().version, 4);
 
-    // Adding an index bumps the min snapshot for a collection as of SERVER-20260. This may
-    // change to just filter that index out from query planning as part of SERVER-20439.
+    // Adding an index does not bump the min snapshot for a collection. Collection scans are
+    // possible, however the index is not guaranteed to be usable until the majority-committed
+    // snapshot advances.
     t.createIndex({version: 1}, {}, 0);
-    assertNoSnapshotAvailableForReadConcernLevel();
+    assert.eq(getCursorForReadConcernLevel().itcount(), 10);
+    assert.eq(getAggCursorForReadConcernLevel().itcount(), 10);
 
     // To use the index, a snapshot created after the index was completed must be marked
     // committed.
     var newSnapshot = assert.commandWorked(db.adminCommand("makeSnapshot")).name;
-    assertNoSnapshotAvailableForReadConcernLevel();
     assert.commandWorked(db.adminCommand({"setCommittedSnapshot": newSnapshot}));
     assert.eq(getCursorForReadConcernLevel().itcount(), 10);
     assert.eq(getAggCursorForReadConcernLevel().itcount(), 10);
     assert(isIxscan(db, getExplainPlan({version: 1})));
 
-    // Dropping an index does bump the min snapshot.
+    // Dropping an index does not bump the min snapshot, so the query should succeed.
     t.dropIndex({version: 1});
-    assertNoSnapshotAvailableForReadConcernLevel();
+    assert.eq(getCursorForReadConcernLevel().itcount(), 10);
+    assert.eq(getAggCursorForReadConcernLevel().itcount(), 10);
+    assert(isCollscan(db, getExplainPlan({version: 1})));
 
-    // To use the collection again, a snapshot created after the dropIndex must be marked
-    // committed.
     newSnapshot = assert.commandWorked(db.adminCommand("makeSnapshot")).name;
+    assert.commandWorked(db.adminCommand({"setCommittedSnapshot": newSnapshot}));
+    assert.eq(getCursorForReadConcernLevel().itcount(), 10);
+    assert.eq(getAggCursorForReadConcernLevel().itcount(), 10);
+    assert(isCollscan(db, getExplainPlan({version: 1})));
+
+    // Get the UUID before renaming.
+    const collUuid = (() => {
+        const collectionInfos =
+            assert.commandWorked(db.runCommand({listCollections: 1})).cursor.firstBatch;
+        assert.eq(1, collectionInfos.length);
+        const info = collectionInfos[0];
+        assert.eq(t.getName(), info.name);
+        return info.info.uuid;
+    })();
+    assert(collUuid);
+
+    // Get a cursor before renaming.
+    cursor = getCursorForReadConcernLevel();  // Note: uses batchsize=2.
+    assert.eq(cursor.next().version, 4);
+    assert.eq(cursor.next().version, 4);
+    assert(!cursor.objsLeftInBatch());
+
+    // Even though renaming advances the minimum visible snapshot, we're querying by a namespace
+    // that no longer exists. Because of this, the query surprisingly returns no results instead of
+    // timing out. This violates read-committed semantics but is allowed by the current
+    // specification.
+    const tempNs = db.getName() + '.temp';
+    assert.commandWorked(db.adminCommand({renameCollection: t.getFullName(), to: tempNs}));
+    assert.eq(getCursorForReadConcernLevel().itcount(), 0);
+
+    // Trigger a getMore that should fail due to the rename.
+    let error = assert.throws(() => {
+        cursor.next();
+    });
+    assert.eq(error.code, ErrorCodes.QueryPlanKilled);
+
+    // Starting a new query by UUID will block because the minimum visible timestamp is ahead of the
+    // majority-committed snapshot.
+    assertNoSnapshotAvailableForReadConcernLevelByUUID(collUuid);
+
+    // Renaming back will cause queries to block again because the original namespace exists, and
+    // its minimum visible timestamp is ahead of the current majority-committed snapshot.
+    assert.commandWorked(db.adminCommand({renameCollection: tempNs, to: t.getFullName()}));
     assertNoSnapshotAvailableForReadConcernLevel();
+
+    newSnapshot = assert.commandWorked(db.adminCommand("makeSnapshot")).name;
     assert.commandWorked(db.adminCommand({"setCommittedSnapshot": newSnapshot}));
     assert.eq(getCursorForReadConcernLevel().itcount(), 10);
+    assert.eq(getAggCursorForReadConcernLevel().itcount(), 10);
 
     // Dropping the collection is visible in the committed snapshot, even though it hasn't been
     // marked committed yet. This is allowed by the current specification even though it
diff --git a/jstests/replsets/minimum_visible_with_cluster_time.js b/jstests/replsets/minimum_visible_with_cluster_time.js
index 58797e9e47f..7bbbff02522 100644
--- a/jstests/replsets/minimum_visible_with_cluster_time.js
+++ b/jstests/replsets/minimum_visible_with_cluster_time.js
@@ -73,23 +73,38 @@ for (let i = 0; i < 10; i++) {
     assert.commandWorked(
         coll.createIndex({x: 1}, {'name': 'x_1', 'expireAfterSeconds': 60 * 60 * 23}));
 
-    doMajorityRead(coll, 1);
+    // Majority read should eventually see new documents because it will not block on the index
+    // build.
+    assert.soonNoExcept(() => {
+        doMajorityRead(coll, 1);
+        return true;
+    });
 
     assert.commandWorked(coll.insert({x: 7, y: 2}));
     assert.commandWorked(coll.runCommand(
         'collMod', {'index': {'keyPattern': {x: 1}, 'expireAfterSeconds': 60 * 60 * 24}}));
-    doMajorityRead(coll, 2);
+    // Majority read should eventually see new documents because it will not block on the index
+    // build.
+    assert.soonNoExcept(() => {
+        doMajorityRead(coll, 2);
+        return true;
+    });
 
     assert.commandWorked(coll.insert({x: 7, y: 3}));
     assert.commandWorked(coll.dropIndexes());
 
-    doMajorityRead(coll, 3);
+    // Majority read should eventually see new documents because it will not block on the drop.
+    assert.soonNoExcept(() => {
+        doMajorityRead(coll, 3);
+        return true;
+    });
 
     assert.commandWorked(coll.insert({x: 7, y: 4}));
     const newCollNameI = collNameI + '_new';
     assert.commandWorked(coll.renameCollection(newCollNameI));
 
     coll = primary.getDB(dbName).getCollection(newCollNameI);
+    // Majority read should immediately see new documents because it blocks on the rename.
     doMajorityRead(coll, 4);
 }
 
diff --git a/jstests/replsets/read_committed_with_catalog_changes.js b/jstests/replsets/read_committed_with_catalog_changes.js
index 1afbaa40f10..d314187e561 100644
--- a/jstests/replsets/read_committed_with_catalog_changes.js
+++ b/jstests/replsets/read_committed_with_catalog_changes.js
@@ -21,7 +21,10 @@
  *  - reindex collection
  *  - compact collection
  *
- * @tags: [requires_majority_read_concern]
+ * @tags: [
+ *   requires_fcv_47,
+ *   requires_majority_read_concern,
+ * ]
  */
 
 load("jstests/libs/parallelTester.js");  // For Thread.
@@ -140,8 +143,22 @@ const testCases = {
             // So, disabling index build commit quorum.
             assert.commandWorked(db.coll.createIndex({x: 1}, {}, 0));
         },
-        blockedCollections: ['coll'],
-        unblockedCollections: ['other'],
+        blockedCollections: [],
+        unblockedCollections: ['coll', 'other'],
+    },
+    collMod: {
+        prepare: function(db) {
+            // This test create indexes with majority of nodes not available for replication.
+            // So, disabling index build commit quorum.
+            assert.commandWorked(db.coll.createIndex({x: 1}, {expireAfterSeconds: 60 * 60}, 0));
+            assert.commandWorked(db.coll.insert({_id: 1, x: 1}));
+        },
+        performOp: function(db) {
+            assert.commandWorked(db.coll.runCommand(
+                'collMod', {index: {keyPattern: {x: 1}, expireAfterSeconds: 60 * 61}}));
+        },
+        blockedCollections: [],
+        unblockedCollections: ['coll'],
     },
     dropIndex: {
         prepare: function(db) {
@@ -155,8 +172,8 @@ const testCases = {
         performOp: function(db) {
             assert.commandWorked(db.coll.dropIndex({x: 1}));
         },
-        blockedCollections: ['coll'],
-        unblockedCollections: ['other'],
+        blockedCollections: [],
+        unblockedCollections: ['coll', 'other'],
     },
 
     // Remaining case is a local-only operation.