Merge #28798

28798: release-2.0: storage/engine: invalidate cached iterator state when necessary r=benesch a=petermattis

Backport 1/1 commits from #28794.

/cc @cockroachdb/release

---

`Iterator.{MVCCGet,MVCCScan,FindSplitKey,ComputeStats}` need to
invalidate the cached iterator state. They were previously failing to do
so which could lead to rare scenarios where a key could be considered
present in the database which had never been written.

Fixes #28025

Release note (bug fix): Fix rare scenario where the value written for
one system key was seen when another system key was read leading to the
violation of internal invariants.


Co-authored-by: Peter Mattis <petermattis@gmail.com>

pkg/storage/engine/mvcc_test.go

@@ -1282,6 +1282,70 @@ func TestMVCCGetProtoInconsistent(t *testing.T) {
 	}
 }
 
+// Regression test for #28205: MVCCGet and MVCCScan, FindSplitKey, and
+// ComputeStats need to invalidate the cached iterator data.
+func TestMVCCInvalidateIterator(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	for _, which := range []string{"get", "scan", "findSplitKey", "computeStats"} {
+		t.Run(which, func(t *testing.T) {
+			engine := createTestEngine()
+			defer engine.Close()
+
+			ctx := context.Background()
+			ts1 := hlc.Timestamp{WallTime: 1}
+			ts2 := hlc.Timestamp{WallTime: 2}
+
+			key := roachpb.Key("a")
+			if err := MVCCPut(ctx, engine, nil, key, ts1, value1, nil); err != nil {
+				t.Fatal(err)
+			}
+
+			var prefix bool
+			switch which {
+			case "get":
+				prefix = true
+			case "scan", "findSplitKey", "computeStats":
+			}
+
+			// Use a batch which internally caches the iterator.
+			batch := engine.NewBatch()
+			defer batch.Close()
+
+			{
+				// Seek the iter to a valid position.
+				iter := batch.NewIterator(prefix)
+				iter.Seek(MakeMVCCMetadataKey(key))
+				iter.Close()
+			}
+
+			var err error
+			switch which {
+			case "get":
+				_, _, err = MVCCGet(ctx, batch, key, ts2, true, nil)
+			case "scan":
+				_, _, _, err = MVCCScan(ctx, batch, key, roachpb.KeyMax, math.MaxInt64, ts2, true, nil)
+			case "findSplitKey":
+				_, err = MVCCFindSplitKey(ctx, batch, roachpb.RKeyMin, roachpb.RKeyMax, 64<<20, true)
+			case "computeStats":
+				iter := batch.NewIterator(prefix)
+				_, err = iter.ComputeStats(NilKey, MVCCKeyMax, 0)
+				iter.Close()
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			// Verify that the iter is invalid.
+			iter := batch.NewIterator(prefix)
+			defer iter.Close()
+			if ok, _ := iter.Valid(); ok {
+				t.Fatalf("iterator should not be valid")
+			}
+		})
+	}
+}
+
 func TestMVCCScan(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	engine := createTestEngine()

pkg/storage/engine/rocksdb.go

@@ -2030,6 +2030,14 @@ func (r *rocksDBIterator) UnsafeValue() []byte {
 	return cSliceToUnsafeGoBytes(r.value)
 }
 
+func (r *rocksDBIterator) clearState() {
+	r.valid = false
+	r.reseek = true
+	r.key = C.DBKey{}
+	r.value = C.DBSlice{}
+	r.err = nil
+}
+
 func (r *rocksDBIterator) setState(state C.DBIterState) {
 	r.valid = bool(state.valid)
 	r.reseek = false
@@ -2041,6 +2049,7 @@ func (r *rocksDBIterator) setState(state C.DBIterState) {
 func (r *rocksDBIterator) ComputeStats(
 	start, end MVCCKey, nowNanos int64,
 ) (enginepb.MVCCStats, error) {
+	r.clearState()
 	result := C.MVCCComputeStats(r.iter, goToCKey(start), goToCKey(end), C.int64_t(nowNanos))
 	stats, err := cStatsToGoStats(result, nowNanos)
 	if util.RaceEnabled {
@@ -2067,6 +2076,7 @@ func (r *rocksDBIterator) FindSplitKey(
 	start, end, minSplitKey MVCCKey, targetSize int64, allowMeta2Splits bool,
 ) (MVCCKey, error) {
 	var splitKey C.DBString
+	r.clearState()
 	status := C.MVCCFindSplitKey(r.iter, goToCKey(start), goToCKey(end), goToCKey(minSplitKey),
 		C.int64_t(targetSize), C.bool(allowMeta2Splits), &splitKey)
 	if err := statusToError(status); err != nil {
@@ -2085,6 +2095,7 @@ func (r *rocksDBIterator) MVCCGet(
 		return nil, nil, emptyKeyError()
 	}
 
+	r.clearState()
 	state := C.MVCCGet(
 		r.iter, goToCSlice(key), goToCTimestamp(timestamp),
 		goToCTxn(txn), C.bool(consistent), C.bool(tombstones),
@@ -2143,6 +2154,7 @@ func (r *rocksDBIterator) MVCCScan(
 		return nil, 0, nil, emptyKeyError()
 	}
 
+	r.clearState()
 	state := C.MVCCScan(
 		r.iter, goToCSlice(start), goToCSlice(end),
 		goToCTimestamp(timestamp), C.int64_t(max),