Fix and clarify ignore_unknown_options (#12989)

Summary:
`ignore_unknown_options=true` had an undocumented behavior of having no effect (disallow unknown options) if reading from the same or older major.minor version. Presumably this was intended to catch unintentional addition of new options in a patch release, but there is no automated version compatibility testing between patch releases. So this was a bad choice without such testing support, because it just means users would hit the failure in case of adding features to a patch release.

In this diff we respect ignore_unknown_options when reading a file from any newer version, even patch versions, and document this behavior in the API.

I don't think it's practical or necessary to test among patch releases in check_format_compatible.sh. This seems like an exceptional case of applying a *different semantics* to patch version updates than to minor/major versions.

Pull Request resolved: #12989

Test Plan: unit test updated (and refactored)

Reviewed By: jaykorean

Differential Revision: D62168738

Pulled By: pdillinger

fbshipit-source-id: fb3c3ef30f0bbad0d5ffcc4570fb9ef963e7daac

include/rocksdb/convenience.h

@@ -56,7 +56,9 @@ struct ConfigOptions {
                      // setting
   };
 
-  // When true, any unused options will be ignored and OK will be returned
+  // When true, any unused options will be ignored and OK will be returned.
+  // For options files that appear to be from the current version or earlier,
+  // unknown options are considered corruption regardless of this setting.
   bool ignore_unknown_options = false;
 
   // When true, any unsupported options will be ignored and OK will be returned

options/options_parser.cc

@@ -296,12 +296,13 @@ Status RocksDBOptionsParser::Parse(const ConfigOptions& config_options_in,
         return s;
       }
 
-      // If the option file is not generated by a higher minor version,
-      // there shouldn't be any unknown option.
+      // If the option file is not generated by a higher version, unknown
+      // option should only mean corruption.
       if (config_options.ignore_unknown_options &&
           section == kOptionSectionVersion) {
-        if (db_version[0] < ROCKSDB_MAJOR || (db_version[0] == ROCKSDB_MAJOR &&
-                                              db_version[1] <= ROCKSDB_MINOR)) {
+        using VTuple = std::tuple<int, int, int>;
+        if (VTuple(db_version[0], db_version[1], db_version[2]) <=
+            VTuple(ROCKSDB_MAJOR, ROCKSDB_MINOR, ROCKSDB_PATCH)) {
           config_options.ignore_unknown_options = false;
         }
       }

options/options_test.cc

@@ -3449,44 +3449,8 @@ TEST_F(OptionsParserTest, DuplicateCFOptions) {
 }
 
 TEST_F(OptionsParserTest, IgnoreUnknownOptions) {
-  for (int case_id = 0; case_id < 5; case_id++) {
-    DBOptions db_opt;
-    db_opt.max_open_files = 12345;
-    db_opt.max_background_flushes = 301;
-    db_opt.max_total_wal_size = 1024;
-    ColumnFamilyOptions cf_opt;
-
-    std::string version_string;
-    bool should_ignore = true;
-    if (case_id == 0) {
-      // same version
-      should_ignore = false;
-      version_string = std::to_string(ROCKSDB_MAJOR) + "." +
-                       std::to_string(ROCKSDB_MINOR) + ".0";
-    } else if (case_id == 1) {
-      // higher minor version
-      should_ignore = true;
-      version_string = std::to_string(ROCKSDB_MAJOR) + "." +
-                       std::to_string(ROCKSDB_MINOR + 1) + ".0";
-    } else if (case_id == 2) {
-      // higher major version.
-      should_ignore = true;
-      version_string = std::to_string(ROCKSDB_MAJOR + 1) + ".0.0";
-    } else if (case_id == 3) {
-      // lower minor version
-#if ROCKSDB_MINOR == 0
-      continue;
-#else
-      version_string = std::to_string(ROCKSDB_MAJOR) + "." +
-                       std::to_string(ROCKSDB_MINOR - 1) + ".0";
-      should_ignore = false;
-#endif
-    } else {
-      // lower major version
-      should_ignore = false;
-      version_string = std::to_string(ROCKSDB_MAJOR - 1) + "." +
-                       std::to_string(ROCKSDB_MINOR) + ".0";
-    }
+  auto testCase = [&](bool should_ignore, const std::string& version_string) {
+    SCOPED_TRACE(std::to_string(should_ignore) + ", " + version_string);
 
     std::string options_file_content =
         "# This is a testing option string.\n"
@@ -3519,16 +3483,45 @@ TEST_F(OptionsParserTest, IgnoreUnknownOptions) {
     RocksDBOptionsParser parser;
     ASSERT_NOK(parser.Parse(kTestFileName, fs_.get(), false,
                             4096 /* readahead_size */));
+    Status parse_status = parser.Parse(kTestFileName, fs_.get(),
+                                       true /* ignore_unknown_options */,
+                                       4096 /* readahead_size */);
     if (should_ignore) {
-      ASSERT_OK(parser.Parse(kTestFileName, fs_.get(),
-                             true /* ignore_unknown_options */,
-                             4096 /* readahead_size */));
+      ASSERT_OK(parse_status);
     } else {
-      ASSERT_NOK(parser.Parse(kTestFileName, fs_.get(),
-                              true /* ignore_unknown_options */,
-                              4096 /* readahead_size */));
+      ASSERT_NOK(parse_status);
     }
-  }
+  };
+
+  // Same version
+  testCase(false, GetRocksVersionAsString());
+  // Same except .0 patch
+  testCase(false, std::to_string(ROCKSDB_MAJOR) + "." +
+                      std::to_string(ROCKSDB_MINOR) + ".0");
+  // Higher major version
+  testCase(true, std::to_string(ROCKSDB_MAJOR + 1) + "." +
+                     std::to_string(ROCKSDB_MINOR) + ".0");
+  // Higher minor version
+  testCase(true, std::to_string(ROCKSDB_MAJOR) + "." +
+                     std::to_string(ROCKSDB_MINOR + 1) + ".0");
+  // Higher patch version
+  testCase(true, std::to_string(ROCKSDB_MAJOR) + "." +
+                     std::to_string(ROCKSDB_MINOR) + "." +
+                     std::to_string(ROCKSDB_PATCH + 1));
+  // Lower major version
+  testCase(false, std::to_string(ROCKSDB_MAJOR - 1) + "." +
+                      std::to_string(ROCKSDB_MINOR) + ".0");
+#if ROCKSDB_MINOR > 0
+  // Lower minor version
+  testCase(false, std::to_string(ROCKSDB_MAJOR) + "." +
+                      std::to_string(ROCKSDB_MINOR - 1) + ".0");
+#endif
+#if ROCKSDB_PATCH > 0
+  // Lower patch version
+  testCase(false, std::to_string(ROCKSDB_MAJOR) + "." +
+                      std::to_string(ROCKSDB_MINOR - 1) + "." +
+                      std::to_string(ROCKSDB_PATCH - 1));
+#endif
 }
 
 TEST_F(OptionsParserTest, ParseVersion) {