SERVER-48971 collMod cannot ever accept the new 'recordPreImages' field when in FCV 4.2 because v4.2 secondaries replicate it

2 files changed, 70 insertions, 66 deletions

diff --git a/jstests/noPassthrough/record_preimage_startup_validation.js b/jstests/noPassthrough/record_preimage_startup_validation.js
index 6755f2439da..f9f3f16b23f 100644
--- a/jstests/noPassthrough/record_preimage_startup_validation.js
+++ b/jstests/noPassthrough/record_preimage_startup_validation.js
@@ -1,95 +1,99 @@
-/*
- * This test validates that we accept/reject the recordPreImage flag via collMode and create
- * commands only if the node is a member of a replica set that is not a shard/config server,
- * and that we require the FCV to be 4.4.
+/**
+ * Validate that we accept/reject the 'recordPreImages' flag via collMod and create commands only if
+ * the node is a member of a replica set, and that the flag is only accepted in FCV 4.4.
  *
  * @tags: [requires_replication, requires_persistence]
  */
 (function() {
 'use strict';
 
-// Start a mongod that's not in a replica set
-let conn = MongoRunner.runMongod({});
+/**
+ * Test that a standalone cannot record pre-images.
+ */
 
-// Check that we cannot record pre-images on a standalone
+let conn = MongoRunner.runMongod({});
 assert.commandFailedWithCode(conn.getDB("test").runCommand({create: "test", recordPreImages: true}),
                              ErrorCodes.InvalidOptions);
-
 MongoRunner.stopMongod(conn);
 
-// Start a 1-node repl set to be used for the rest of the test
-let rsTest = new ReplSetTest({nodes: 1});
-rsTest.startSet();
-rsTest.initiate();
-
-// Check that we cannot set recordPreImages on the local or admin databases.
-let adminDB = rsTest.getPrimary().getDB("admin");
-assert.commandFailedWithCode(adminDB.runCommand({create: "preimagecoll", recordPreImages: true}),
-                             ErrorCodes.InvalidOptions);
-let localDB = rsTest.getPrimary().getDB("local");
-assert.commandFailedWithCode(localDB.runCommand({create: "preimagecoll", recordPreImages: true}),
-                             ErrorCodes.InvalidOptions);
-
-let testDB = rsTest.getPrimary().getDB("test");
-const findCollectionInfo = function(collName) {
-    var all = testDB.getCollectionInfos();
-    if (all.length == 0) {
+/**
+ * Find the collection information on database 'nodeDB' for collection 'collName'.
+ */
+const findCollectionInfo = function(nodeDB, collName) {
+    let collInfos = nodeDB.getCollectionInfos();
+    if (collInfos.length == 0) {
         return {};
     }
-    all = all.filter(function(z) {
+    let collInfo = collInfos.filter(function(z) {
         return z.name == collName;
     });
-    assert.eq(all.length, 1);
-    return all[0];
+    assert.eq(collInfo.length, 1);
+    return collInfo[0];
 };
 
-assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: lastStableFCV}));
-
-// This should fail because we've set the FCV to lastStableFCV (4.2).
-assert.commandFailedWithCode(testDB.runCommand({create: "test", recordPreImages: true}),
-                             ErrorCodes.InvalidOptions);
-// Check that failing to set the option doesn't accidentally set it anyways.
-assert.eq(findCollectionInfo("test").options, undefined);
+/**
+ * Start a 1-node repl set to be used for the rest of the test.
+ */
 
-assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: latestFCV}));
+let replTest = new ReplSetTest({nodes: 1});
+replTest.startSet();
+replTest.initiate();
 
-// Check the positive test cases. We should be able to set this flag via create or collMod.
-assert.commandWorked(testDB.runCommand({create: "test", recordPreImages: true}));
-assert.eq(findCollectionInfo("test").options.recordPreImages, true);
+let adminDB = replTest.getPrimary().getDB("admin");
+let localDB = replTest.getPrimary().getDB("local");
+let testDB = replTest.getPrimary().getDB("test");
+const collName1 = "testColl1";
+const collName2 = "testColl2";
+const collName3 = "testColl3";
 
-assert.commandWorked(testDB.runCommand({create: "test2"}));
-assert.commandWorked(testDB.runCommand({collMod: "test2", recordPreImages: true}));
-assert.eq(findCollectionInfo("test2").options.recordPreImages, true);
+// Check that we cannot set recordPreImages on the local or admin databases.
+assert.commandFailedWithCode(adminDB.runCommand({create: "preimagecoll", recordPreImages: true}),
+                             ErrorCodes.InvalidOptions);
+assert.commandFailedWithCode(localDB.runCommand({create: "preimagecoll", recordPreImages: true}),
+                             ErrorCodes.InvalidOptions);
 
-// Downgrade the FCV
+// Should not be able to set or unset recordPreImages in lastStableFCV 4.2. Creating oplog entries
+// with the new recordPreImages field would crash v4.2 secondaries to which the field is unknown.
 assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: lastStableFCV}));
 
-// We should be able to unset the flag when the FCV is downgraded
-assert.commandWorked(testDB.runCommand({collMod: "test", recordPreImages: false}));
-// But we shouldn't be able to set it.
-assert.commandFailedWithCode(testDB.runCommand({collMod: "test", recordPreImages: true}),
+// Check the create collection command in FCV 4.2 fails.
+// Note: recordPreImages: false can succeed, but has no effect because it sets nothing.
+assert.commandFailedWithCode(testDB.runCommand({create: collName1, recordPreImages: true}),
                              ErrorCodes.InvalidOptions);
 
-// The server should fail to start with any collections having the flag set.
-assert.throws(() => {
-    rsTest.restart(rsTest.getPrimary());
-});
-
-rsTest.stopSet();
-}());
-(function() {
-rsTest = new ReplSetTest({nodes: 1});
-rsTest.startSet();
-rsTest.initiate();
-adminDB = rsTest.getPrimary().getDB("admin");
-testDB = rsTest.getPrimary().getDB("test");
+// Check the collMod command in FCV 4.2 fails.
+assert.writeOK(testDB.getCollection(collName1).insert({_id: 1}));  // create the collection.
+assert.commandFailedWithCode(testDB.runCommand({collMod: collName1, recordPreImages: true}),
+                             ErrorCodes.InvalidOptions);
+// Check that failing to set the option doesn't accidentally set it anyways.
+assert.eq(findCollectionInfo(testDB, collName1).options, {});
 
+// Check the positive test cases. We should be able to set the recordPreImages field via create or
+// collMod in FCV 4.4.
 assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: latestFCV}));
-assert.commandWorked(testDB.runCommand({create: "test", recordPreImages: true}));
 
+// Check the create collection command in FCV 4.4 succeeds.
+assert.commandWorked(testDB.runCommand({create: collName2, recordPreImages: true}));
+assert.eq(findCollectionInfo(testDB, collName2).options.recordPreImages, true);
+
+// Check the collMod collection command in FCV 4.4 succeeds.
+assert.commandWorked(testDB.runCommand({create: collName3}));
+assert.commandWorked(testDB.runCommand({collMod: collName3, recordPreImages: true}));
+assert.eq(findCollectionInfo(testDB, collName3).options.recordPreImages, true);
+assert.commandWorked(testDB.runCommand({collMod: collName3, recordPreImages: false}));
+assert.eq(findCollectionInfo(testDB, collName3).options, {});
+
+// Restarting the server while in FCV 4.4 when recordPreImages flags are set should be successful.
 assert.doesNotThrow(() => {
-    rsTest.restart(rsTest.getPrimary());
+    replTest.restart(replTest.getPrimary());
+});
+
+// The server should fail to restart when the recordPreImages field is found on startup in FCV 4.2.
+assert.commandWorked(replTest.getPrimary().getDB("admin").runCommand(
+    {setFeatureCompatibilityVersion: lastStableFCV}));
+assert.throws(() => {
+    replTest.restart(replTest.getPrimary());
 });
 
-rsTest.stopSet();
+replTest.stopSet();
 }());
diff --git a/src/mongo/db/catalog/collection_impl.cpp b/src/mongo/db/catalog/collection_impl.cpp
index c145bd1bc8a..0e02b848366 100644
--- a/src/mongo/db/catalog/collection_impl.cpp
+++ b/src/mongo/db/catalog/collection_impl.cpp
@@ -301,6 +301,8 @@ void CollectionImpl::init(OperationContext* opCtx) {
     // are not supported with certain combinations of action and level.
     _validationAction = uassertStatusOK(_parseValidationAction(collectionOptions.validationAction));
     _validationLevel = uassertStatusOK(_parseValidationLevel(collectionOptions.validationLevel));
+
+    // The recordPreImages field should only be present in FCV 4.4.
     if (collectionOptions.recordPreImages) {
         uassertStatusOK(validatePreImageRecording(opCtx, _ns));
         _recordPreImages = true;
@@ -873,9 +875,7 @@ bool CollectionImpl::getRecordPreImages() const {
 }
 
 void CollectionImpl::setRecordPreImages(OperationContext* opCtx, bool val) {
-    if (val) {
-        uassertStatusOK(validatePreImageRecording(opCtx, _ns));
-    }
+    uassertStatusOK(validatePreImageRecording(opCtx, _ns));
     DurableCatalog::get(opCtx)->setRecordPreImages(opCtx, getCatalogId(), val);
     _recordPreImages = val;
 }