storage: implement RangeKeyChanged() for MVCCIncrementalIterator

This patch implements `RangeKeyChanged()` for `MVCCIncrementalIterator`.
It only fires if the time bound range keys change, not if a
`Next(Key)IgnoringTime()` operation reveals additional range keys.

Release justification: bug fixes and low-risk updates to new functionality

Release note: None

pkg/storage/mvcc_history_test.go

@@ -1605,16 +1605,15 @@ func cmdIterScan(e *evalCtx) error {
 	// adjust e.iterRangeKeys to comply with the previous positioning operation.
 	// The previous position already passed this check, so it doesn't matter that
 	// we're fudging e.rangeKeys.
-	if _, ok := e.bareIter().(*MVCCIncrementalIterator); !ok {
-		if e.iter.RangeKeyChanged() {
-			if e.iterRangeKeys.IsEmpty() {
-				e.iterRangeKeys = MVCCRangeKeyStack{
-					Bounds:   roachpb.Span{Key: keys.MinKey.Next(), EndKey: keys.MaxKey},
-					Versions: MVCCRangeKeyVersions{{Timestamp: hlc.MinTimestamp}},
-				}
-			} else {
-				e.iterRangeKeys.Clear()
+	if e.iter.RangeKeyChanged() {
+		if e.iterRangeKeys.IsEmpty() {
+			e.iterRangeKeys = MVCCRangeKeyStack{
+				// NB: Clone MinKey/MaxKey, since we write into these byte slices later.
+				Bounds:   roachpb.Span{Key: keys.MinKey.Next().Clone(), EndKey: keys.MaxKey.Clone()},
+				Versions: MVCCRangeKeyVersions{{Timestamp: hlc.MinTimestamp}},
 			}
+		} else {
+			e.iterRangeKeys.Clear()
 		}
 	}
 
@@ -1757,12 +1756,15 @@ func printIter(e *evalCtx) {
 }
 
 func checkAndUpdateRangeKeyChanged(e *evalCtx) bool {
-	// MVCCIncrementalIterator does not yet support RangeKeyChanged().
-	if _, ok := e.bareIter().(*MVCCIncrementalIterator); ok {
-		return false
-	}
 	rangeKeyChanged := e.iter.RangeKeyChanged()
 	rangeKeys := e.iter.RangeKeys()
+
+	// For MVCCIncrementalIterator, Next(Key)IgnoringTime() may reveal additional
+	// range key versions, but RangeKeyChanged only applies to the filtered set.
+	if incrIter, ok := e.bareIter().(*MVCCIncrementalIterator); ok && incrIter.ignoringTime {
+		rangeKeys = incrIter.rangeKeys
+	}
+
 	if rangeKeyChanged != !rangeKeys.Equal(e.iterRangeKeys) {
 		e.t.Fatalf("incorrect RangeKeyChanged=%t (was:%s is:%s) at %s\n",
 			rangeKeyChanged, e.iterRangeKeys, rangeKeys, e.td.Pos)

pkg/storage/mvcc_incremental_iterator.go

@@ -102,6 +102,10 @@ type MVCCIncrementalIterator struct {
 	// rangeKeysIgnoringTime contains the complete range keys at the current location.
 	rangeKeysIgnoringTime MVCCRangeKeyStack
 
+	// rangeKeyChanged is true if i.rangeKeys changed during the previous
+	// positioning operation.
+	rangeKeyChanged bool
+
 	// ignoringTime is true if the iterator is currently ignoring time bounds,
 	// i.e. following a call to NextIgnoringTime().
 	ignoringTime bool
@@ -243,8 +247,10 @@ func (i *MVCCIncrementalIterator) SeekGE(startKey MVCCKey) {
 			startKey = MakeMVCCMetadataKey(tbiKey)
 		}
 	}
+	prevRangeKey := i.rangeKeys.Bounds.Key.Clone()
 	i.iter.SeekGE(startKey)
 	i.advance(true /* seeked */)
+	i.rangeKeyChanged = !prevRangeKey.Equal(i.rangeKeys.Bounds.Key) // Is there a better way?
 }
 
 // Close implements SimpleMVCCIterator.
@@ -470,6 +476,8 @@ func (i *MVCCIncrementalIterator) updateRangeKeys() (bool, bool) {
 // intent policy is MVCCIncrementalIterIntentPolicyError.
 func (i *MVCCIncrementalIterator) advance(seeked bool) {
 	i.ignoringTime = false
+	i.rangeKeyChanged = false
+	hadRange := !i.rangeKeys.IsEmpty()
 	for {
 		if !i.updateValid() {
 			return
@@ -491,6 +499,7 @@ func (i *MVCCIncrementalIterator) advance(seeked bool) {
 		var newRangeKey bool
 		if rangeKeyChanged {
 			i.hasPoint, i.hasRange = i.updateRangeKeys()
+			i.rangeKeyChanged = hadRange || i.hasRange // !hasRange → !hasRange is no change
 			newRangeKey = i.hasRange
 
 			// If we're on a visible, bare range key then we're done. If the range key
@@ -596,8 +605,12 @@ func (i *MVCCIncrementalIterator) RangeKeys() MVCCRangeKeyStack {
 }
 
 // RangeKeyChanged implements SimpleMVCCIterator.
+//
+// RangeKeyChanged only applies to the filtered set of range keys. If an
+// IgnoringTime() operation reveals addition range keys or versions, these do
+// not trigger RangeKeyChanged().
 func (i *MVCCIncrementalIterator) RangeKeyChanged() bool {
-	panic("not implemented")
+	return i.rangeKeyChanged
 }
 
 // UnsafeValue implements SimpleMVCCIterator.
@@ -612,16 +625,21 @@ func (i *MVCCIncrementalIterator) UnsafeValue() []byte {
 // intent policy.
 func (i *MVCCIncrementalIterator) updateIgnoreTime() {
 	i.ignoringTime = true
+	i.rangeKeyChanged = false
+	hadRange := !i.rangeKeys.IsEmpty()
 	for {
 		if !i.updateValid() {
 			return
 		}
 
 		if i.iter.RangeKeyChanged() {
-			if i.hasPoint, i.hasRange = i.updateRangeKeys(); !i.hasPoint {
+			i.hasPoint, i.hasRange = i.updateRangeKeys()
+			i.rangeKeyChanged = hadRange || i.hasRange // !hasRange → !hasRange is no change
+			if !i.hasPoint {
+				i.meta.Reset()
 				return
 			}
-		} else {
+		} else if !i.hasPoint {
 			i.hasPoint = true
 		}
 
@@ -653,6 +671,8 @@ func (i *MVCCIncrementalIterator) updateIgnoreTime() {
 // non-incremental iteration by moving the underlying non-TBI iterator forward.
 // Intents within and outside the (StartTime, EndTime] time range are handled
 // according to the iterator policy.
+//
+// RangeKeyChanged() will only fire if the time bound range keys change.
 func (i *MVCCIncrementalIterator) NextIgnoringTime() {
 	i.iter.Next()
 	i.updateIgnoreTime()
@@ -662,6 +682,8 @@ func (i *MVCCIncrementalIterator) NextIgnoringTime() {
 // in a non-incremental iteration by moving the underlying non-TBI iterator
 // forward. Intents within and outside the (StartTime, EndTime] time range are
 // handled according to the iterator policy.
+//
+// RangeKeyChanged() will only fire if the time bound range keys change.
 func (i *MVCCIncrementalIterator) NextKeyIgnoringTime() {
 	i.iter.NextKey()
 	i.updateIgnoreTime()