Merge cockroachdb#83729

83729: storage: add nextKeyIgnoringTime() to MVCCIncrementalIterator r=erikgrinaker a=msbutler

The MVCCInvermentalIterator's NextKeyIgnoringTime() function was previously
deleted in cockroachdb#82691, as there wasn't any use for it at the time. Now, the new
Delete Range with predicate filtering will need it (cockroachdb#83676).

This PR also cleans up duplicate code used to test NextIgnoringTime and
NextKeyIgnoringTime.

Release note: None

Co-authored-by: Michael Butler <[email protected]>

pkg/storage/mvcc_history_test.go

@@ -89,6 +89,7 @@ var sstIterVerify = util.ConstantWithMetamorphicTestBool("mvcc-histories-sst-ite
 // iter_seek_intent_ge k=<key> txn=<name>
 // iter_next
 // iter_next_ignoring_time
+// iter_next_key_ignoring_time
 // iter_next_key
 // iter_prev
 // iter_scan      [reverse]
@@ -667,16 +668,17 @@ var commands = map[string]cmd{
 	"put_rangekey":   {typDataUpdate, cmdPutRangeKey},
 	"scan":           {typReadOnly, cmdScan},
 
-	"iter_new":                {typReadOnly, cmdIterNew},
-	"iter_new_incremental":    {typReadOnly, cmdIterNewIncremental}, // MVCCIncrementalIterator
-	"iter_seek_ge":            {typReadOnly, cmdIterSeekGE},
-	"iter_seek_lt":            {typReadOnly, cmdIterSeekLT},
-	"iter_seek_intent_ge":     {typReadOnly, cmdIterSeekIntentGE},
-	"iter_next":               {typReadOnly, cmdIterNext},
-	"iter_next_ignoring_time": {typReadOnly, cmdIterNextIgnoringTime}, // MVCCIncrementalIterator
-	"iter_next_key":           {typReadOnly, cmdIterNextKey},
-	"iter_prev":               {typReadOnly, cmdIterPrev},
-	"iter_scan":               {typReadOnly, cmdIterScan},
+	"iter_new":                    {typReadOnly, cmdIterNew},
+	"iter_new_incremental":        {typReadOnly, cmdIterNewIncremental}, // MVCCIncrementalIterator
+	"iter_seek_ge":                {typReadOnly, cmdIterSeekGE},
+	"iter_seek_lt":                {typReadOnly, cmdIterSeekLT},
+	"iter_seek_intent_ge":         {typReadOnly, cmdIterSeekIntentGE},
+	"iter_next":                   {typReadOnly, cmdIterNext},
+	"iter_next_ignoring_time":     {typReadOnly, cmdIterNextIgnoringTime},    // MVCCIncrementalIterator
+	"iter_next_key_ignoring_time": {typReadOnly, cmdIterNextKeyIgnoringTime}, // MVCCIncrementalIterator
+	"iter_next_key":               {typReadOnly, cmdIterNextKey},
+	"iter_prev":                   {typReadOnly, cmdIterPrev},
+	"iter_scan":                   {typReadOnly, cmdIterScan},
 
 	"sst_put":            {typDataUpdate, cmdSSTPut},
 	"sst_put_rangekey":   {typDataUpdate, cmdSSTPutRangeKey},
@@ -1432,6 +1434,12 @@ func cmdIterNextIgnoringTime(e *evalCtx) error {
 	return nil
 }
 
+func cmdIterNextKeyIgnoringTime(e *evalCtx) error {
+	e.mvccIncrementalIter().NextKeyIgnoringTime()
+	printIter(e)
+	return nil
+}
+
 func cmdIterNextKey(e *evalCtx) error {
 	e.iter.NextKey()
 	printIter(e)

pkg/storage/mvcc_incremental_iterator.go

@@ -137,13 +137,7 @@ const (
 	// keep collecting entries and intents or skip entries.
 	MVCCIncrementalIterIntentPolicyAggregate
 	// MVCCIncrementalIterIntentPolicyEmit will return intents to
-	// the caller if they are inside the time range. Intents
-	// outside of the time range will be filtered without error.
-	//
-	// TODO(ssd): If we relaxed the requirement that intents are
-	// filtered by time range, we could avoid parsing intents
-	// inside the iterator and leave it to the caller to deal
-	// with.
+	// the caller if they are inside or outside the time range.
 	MVCCIncrementalIterIntentPolicyEmit
 )
 
@@ -599,14 +593,11 @@ func (i *MVCCIncrementalIterator) UnsafeValue() []byte {
 	return i.iter.UnsafeValue()
 }
 
-// NextIgnoringTime returns the next key/value that would be encountered in a
-// non-incremental iteration by moving the underlying non-TBI iterator forward.
-// Intents in the time range (startTime,EndTime] are handled according to the
-// iterator policy.
-func (i *MVCCIncrementalIterator) NextIgnoringTime() {
+// updateIgnoreTime updates the iterator's metadata and handles intents depending on the iterator's
+// intent policy.
+func (i *MVCCIncrementalIterator) updateIgnoreTime() {
 	i.ignoringTime = true
 	for {
-		i.iter.Next()
 		if !i.updateValid() {
 			return
 		}
@@ -630,8 +621,16 @@ func (i *MVCCIncrementalIterator) NextIgnoringTime() {
 
 		// We have encountered an intent but it does not lie in the timestamp span
 		// (startTime, endTime] so we do not throw an error, and attempt to move to
-		// the next valid KV.
+		// the intent's corresponding provisional value.
+		//
+		// Note: it's important to surface the intent's provisional value as callers rely on observing
+		// any value -- provisional, or not -- to make decisions. MVCClearTimeRange, for example,
+		// flushes keys for deletion whenever it encounters a key outside (StartTime,EndTime].
+		//
+		// TODO(msbulter): investigate if it's clearer for the caller to emit the intent in
+		// addition to the provisional value.
 		if i.meta.Txn != nil && i.intentPolicy != MVCCIncrementalIterIntentPolicyEmit {
+			i.iter.Next()
 			continue
 		}
 
@@ -640,6 +639,24 @@ func (i *MVCCIncrementalIterator) NextIgnoringTime() {
 	}
 }
 
+// NextIgnoringTime returns the next key/value that would be encountered 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.
+func (i *MVCCIncrementalIterator) NextIgnoringTime() {
+	i.iter.Next()
+	i.updateIgnoreTime()
+}
+
+// NextKeyIgnoringTime returns the next distinct key that would be encountered
+// 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.
+func (i *MVCCIncrementalIterator) NextKeyIgnoringTime() {
+	i.iter.NextKey()
+	i.updateIgnoreTime()
+}
+
 // NumCollectedIntents returns number of intents encountered during iteration.
 // This is only the case when intent aggregation is enabled, otherwise it is
 // always 0.