HashTable.cpp

@@ -18,15 +18,21 @@ struct string_hash {
   }
 };
 
+// Reads more than this many versions behind `newestVersion` are `TooOld`, so
+// that a caller whose read version fell out of the version window is told to
+// retry. See the `addWrites` postcondition in ConflictSet.h.
+constexpr int64_t kVersionWindow = 2'000'000'000;
+
 struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
-  Impl(int64_t oldestVersion) : oldestVersion(oldestVersion) {}
+  Impl(int64_t oldestVersion)
+      : oldestVersion(oldestVersion), newestVersion(oldestVersion) {}
   void check(const ConflictSet::ReadRange *reads, ConflictSet::Result *results,
              int count) const {
     for (int i = 0; i < count; ++i) {
       auto key =
           std::string_view((const char *)reads[i].begin.p, reads[i].begin.len);
       auto version = reads[i].readVersion;
-      if (version < oldestVersion) {
+      if (version < oldestVersion || version < newestVersion - kVersionWindow) {
         results[i] = TooOld;
         continue;
       }
@@ -38,6 +44,10 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
 
   void addWrites(const ConflictSet::WriteRange *writes, int count,
                  int64_t writeVersion) {
+    // Advance newestVersion before the loop, so that it also advances when
+    // count == 0, as documented in ConflictSet.h.
+    assert(writeVersion >= newestVersion);
+    newestVersion = writeVersion;
     for (int i = 0; i < count; ++i) {
       auto &max = map[std::string((const char *)writes[i].begin.p,
                                   writes[i].begin.len)];
@@ -84,6 +94,7 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
 private:
   int64_t keyUpdates = 0;
   int64_t oldestVersion;
+  int64_t newestVersion;
   std::unordered_map<std::string, int64_t, string_hash, std::equal_to<>> map;
   std::string removalKey;
 };

test_conflict_set.py

@@ -205,8 +205,32 @@ def test_internal_version_zero():
 def test_two_billion_versions():
     with DebugConflictSet() as cs:
         cs.addWrites(int(2e9) + 1)
-        cs.check(read(0, b"\x00", b"\xff"))
+        assert cs.check(read(0, b"\x00", b"\xff")) == [Result.TOO_OLD]
-        cs.check(read(1, b"\x00", b"\xff"))
+        assert cs.check(read(1, b"\x00", b"\xff")) == [Result.COMMIT]
+
+
+def test_two_billion_version_window():
+    # Regression test for issue #98: `addWrites` advances `newestVersion`, so a
+    # read whose readVersion fell more than two billion versions behind it is
+    # TooOld. `hash_table` used to discard writeVersion entirely, so it reported
+    # Conflict/Commit for reads far outside the version window.
+    window = 2 * 10**9
+    newest = 2**32
+
+    def probe(implementation, read_version, *writes):
+        with ConflictSet(0, build_dir=build_dir, implementation=implementation) as cs:
+            cs.addWrites(newest, *writes)
+            return cs.check(read(read_version, b"k"))[0]
+
+    for implementation in ("radix_tree", "skip_list", "hash_table"):
+        # The window advances even when addWrites added no writes at all.
+        assert probe(implementation, 0, write(b"k")) == Result.TOO_OLD
+        assert probe(implementation, 0) == Result.TOO_OLD
+        assert probe(implementation, newest - window - 1, write(b"k")) == Result.TOO_OLD
+        assert probe(implementation, newest - window - 1) == Result.TOO_OLD
+        # Exactly `window` versions behind newestVersion is still evaluable.
+        assert probe(implementation, newest - window, write(b"k")) == Result.CONFLICT
+        assert probe(implementation, newest - window) == Result.COMMIT
 
 
 def test_positive_bytes():