storage: use SetOptions() when reusing Pebble iterators

pkg/kv/kvserver/abortspan/abortspan.go

@@ -76,13 +76,9 @@ func (sc *AbortSpan) max() roachpb.Key {
 
 // ClearData removes all persisted items stored in the cache.
 func (sc *AbortSpan) ClearData(e storage.Engine) error {
-	// NB: The abort span is a Range-ID local key which has no versions or intents.
-	iter := e.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{UpperBound: sc.max()})
-	defer iter.Close()
-	b := e.NewUnindexedBatch(true /* writeOnly */)
+	b := e.NewUnindexedBatch(false /* writeOnly */)
 	defer b.Close()
-	err := b.ClearIterRange(iter, sc.min(), sc.max())
-	if err != nil {
+	if err := b.ClearIterRange(sc.min(), sc.max()); err != nil {
 		return err
 	}
 	return b.Commit(false /* sync */)

pkg/kv/kvserver/batch_spanset_test.go

@@ -78,9 +78,7 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
 			t.Errorf("ClearRange: unexpected error %v", err)
 		}
 		{
-			iter := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax})
-			err := batch.ClearIterRange(iter, outsideKey.Key, outsideKey2.Key)
-			iter.Close()
+			err := batch.ClearIterRange(outsideKey.Key, outsideKey2.Key)
 			if !isWriteSpanErr(err) {
 				t.Errorf("ClearIterRange: unexpected error %v", err)
 			}
@@ -101,9 +99,7 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
 			t.Errorf("ClearRange: unexpected error %v", err)
 		}
 		{
-			iter := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax})
-			err := batch.ClearIterRange(iter, insideKey2.Key, outsideKey4.Key)
-			iter.Close()
+			err := batch.ClearIterRange(outsideKey2.Key, outsideKey4.Key)
 			if !isWriteSpanErr(err) {
 				t.Errorf("ClearIterRange: unexpected error %v", err)
 			}
@@ -325,9 +321,7 @@ func TestSpanSetBatchTimestamps(t *testing.T) {
 			t.Errorf("Clear: unexpected error %v", err)
 		}
 		{
-			iter := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax})
-			err := batch.ClearIterRange(iter, wkey.Key, wkey.Key)
-			iter.Close()
+			err := batch.ClearIterRange(wkey.Key, wkey.Key)
 			if !isWriteSpanErr(err) {
 				t.Errorf("ClearIterRange: unexpected error %v", err)
 			}

pkg/kv/kvserver/batcheval/cmd_clear_range.go

@@ -116,12 +116,7 @@ func ClearRange(
 	if statsDelta.ContainsEstimates == 0 && statsDelta.Total() < ClearRangeBytesThreshold {
 		log.VEventf(ctx, 2, "delta=%d < threshold=%d; using non-range clear",
 			statsDelta.Total(), ClearRangeBytesThreshold)
-		iter := readWriter.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{
-			LowerBound: from,
-			UpperBound: to,
-		})
-		defer iter.Close()
-		if err = readWriter.ClearIterRange(iter, from, to); err != nil {
+		if err = readWriter.ClearIterRange(from, to); err != nil {
 			return result.Result{}, err
 		}
 		return pd, nil

pkg/kv/kvserver/batcheval/cmd_clear_range_test.go

@@ -35,9 +35,9 @@ type wrappedBatch struct {
 	clearRangeCount int
 }
 
-func (wb *wrappedBatch) ClearIterRange(iter storage.MVCCIterator, start, end roachpb.Key) error {
+func (wb *wrappedBatch) ClearIterRange(start, end roachpb.Key) error {
 	wb.clearIterCount++
-	return wb.Batch.ClearIterRange(iter, start, end)
+	return wb.Batch.ClearIterRange(start, end)
 }
 
 func (wb *wrappedBatch) ClearMVCCRangeAndIntents(start, end roachpb.Key) error {

pkg/kv/kvserver/spanset/batch.go

@@ -208,11 +208,6 @@ func (i *MVCCIterator) FindSplitKey(
 	return i.i.FindSplitKey(start, end, minSplitKey, targetSize)
 }
 
-// SetUpperBound is part of the storage.MVCCIterator interface.
-func (i *MVCCIterator) SetUpperBound(key roachpb.Key) {
-	i.i.SetUpperBound(key)
-}
-
 // Stats is part of the storage.MVCCIterator interface.
 func (i *MVCCIterator) Stats() storage.IteratorStats {
 	return i.i.Stats()
@@ -374,11 +369,6 @@ func (i *EngineIterator) UnsafeRawEngineKey() []byte {
 	return i.i.UnsafeRawEngineKey()
 }
 
-// SetUpperBound is part of the storage.EngineIterator interface.
-func (i *EngineIterator) SetUpperBound(key roachpb.Key) {
-	i.i.SetUpperBound(key)
-}
-
 // GetRawIter is part of the storage.EngineIterator interface.
 func (i *EngineIterator) GetRawIter() *pebble.Iterator {
 	return i.i.GetRawIter()
@@ -592,11 +582,11 @@ func (s spanSetWriter) ClearMVCCRange(start, end storage.MVCCKey) error {
 	return s.w.ClearMVCCRange(start, end)
 }
 
-func (s spanSetWriter) ClearIterRange(iter storage.MVCCIterator, start, end roachpb.Key) error {
+func (s spanSetWriter) ClearIterRange(start, end roachpb.Key) error {
 	if err := s.checkAllowedRange(start, end); err != nil {
 		return err
 	}
-	return s.w.ClearIterRange(iter, start, end)
+	return s.w.ClearIterRange(start, end)
 }
 
 func (s spanSetWriter) Merge(key storage.MVCCKey, value []byte) error {

pkg/storage/bench_pebble_test.go

@@ -340,9 +340,7 @@ func BenchmarkClearMVCCRange_Pebble(b *testing.B) {
 func BenchmarkClearIterRange_Pebble(b *testing.B) {
 	ctx := context.Background()
 	runClearRange(ctx, b, setupMVCCPebble, func(eng Engine, batch Batch, start, end MVCCKey) error {
-		iter := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax})
-		defer iter.Close()
-		return batch.ClearIterRange(iter, start.Key, end.Key)
+		return batch.ClearIterRange(start.Key, end.Key)
 	})
 }
 

pkg/storage/bench_test.go

@@ -1286,29 +1286,12 @@ func runClearRange(
 	})
 	defer eng.Close()
 
-	// It is not currently possible to ClearRange(NilKey, MVCCKeyMax) thanks to a
-	// variety of hacks inside of ClearRange that explode if provided the NilKey.
-	// So instead we start our ClearRange at the first key that actually exists.
-	//
-	// TODO(benesch): when those hacks are removed, don't bother computing the
-	// first key and simply ClearRange(NilKey, MVCCKeyMax).
-	//
-	// TODO(sumeer): we are now seeking starting at LocalMax, so the
-	// aforementioned issue is probably resolved. Clean this up.
-	iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax})
-	defer iter.Close()
-	iter.SeekGE(MVCCKey{Key: keys.LocalMax})
-	if ok, err := iter.Valid(); !ok {
-		b.Fatalf("unable to find first key (err: %v)", err)
-	}
-	firstKey := iter.Key()
-
 	b.SetBytes(rangeBytes)
 	b.ResetTimer()
 
 	for i := 0; i < b.N; i++ {
-		batch := eng.NewUnindexedBatch(true /* writeOnly */)
-		if err := clearRange(eng, batch, firstKey, MVCCKeyMax); err != nil {
+		batch := eng.NewUnindexedBatch(false /* writeOnly */)
+		if err := clearRange(eng, batch, MVCCKey{Key: keys.LocalMax}, MVCCKeyMax); err != nil {
 			b.Fatal(err)
 		}
 		// NB: We don't actually commit the batch here as we don't want to delete

pkg/storage/engine.go

@@ -169,26 +169,6 @@ type MVCCIterator interface {
 	// package-level MVCCFindSplitKey instead. For correct operation, the caller
 	// must set the upper bound on the iterator before calling this method.
 	FindSplitKey(start, end, minSplitKey roachpb.Key, targetSize int64) (MVCCKey, error)
-	// SetUpperBound installs a new upper bound for this iterator. The caller
-	// can modify the parameter after this function returns. This must not be a
-	// nil key. When Reader.ConsistentIterators is true, prefer creating a new
-	// iterator.
-	//
-	// Due to the rare use, we are limiting this method to not switch an
-	// iterator from a global key upper-bound to a local key upper-bound (it
-	// simplifies some code in intentInterleavingIter) or vice versa. Iterator
-	// reuse already happens under-the-covers for most Reader implementations
-	// when constructing a new iterator, and that is a much cleaner solution.
-	//
-	// TODO(sumeer): this method is rarely used and is a source of complexity
-	// since intentInterleavingIter needs to fiddle with the bounds of its
-	// underlying iterators when this is called. Currently only used by
-	// pebbleBatch.ClearIterRange to modify the upper bound of the iterator it
-	// is given: this use is unprincipled and there is a comment in that code
-	// about it. The caller is already usually setting the bounds accurately,
-	// and in some cases the callee is tightening the upper bound. Remove that
-	// use case and remove this from the interface.
-	SetUpperBound(roachpb.Key)
 	// Stats returns statistics about the iterator.
 	Stats() IteratorStats
 	// SupportsPrev returns true if MVCCIterator implementation supports reverse
@@ -239,10 +219,6 @@ type EngineIterator interface {
 	// Value returns the current value as a byte slice.
 	// REQUIRES: latest positioning function returned valid=true.
 	Value() []byte
-	// SetUpperBound installs a new upper bound for this iterator. When
-	// Reader.ConsistentIterators is true, prefer creating a new iterator.
-	// TODO(sumeer): remove this method.
-	SetUpperBound(roachpb.Key)
 	// GetRawIter is a low-level method only for use in the storage package,
 	// that returns the underlying pebble Iterator.
 	GetRawIter() *pebble.Iterator
@@ -326,8 +302,8 @@ const (
 	// Specifically:
 	// - If both bounds are set they must not span from local to global.
 	// - Any bound (lower or upper), constrains the iterator for its lifetime to
-	//   one of local or global keys. The iterator will not tolerate a seek or
-	//   SetUpperBound call that violates this constraint.
+	//   one of local or global keys. The iterator will not tolerate a seek that
+	//   violates this constraint.
 	// We could, with significant code complexity, not constrain an iterator for
 	// its lifetime, and allow a seek that specifies a global (local) key to
 	// change the constraint to global (local). This would allow reuse of the
@@ -563,17 +539,11 @@ type Writer interface {
 	// It is safe to modify the contents of the arguments after it returns.
 	ClearMVCCRange(start, end MVCCKey) error
 
-	// ClearIterRange removes a set of entries, from start (inclusive) to end
-	// (exclusive). Similar to Clear and ClearRange, this method actually
-	// removes entries from the storage engine. Unlike ClearRange, the entries
-	// to remove are determined by iterating over iter and per-key storage
-	// tombstones (not MVCC tombstones) are generated. If the MVCCIterator was
-	// constructed using MVCCKeyAndIntentsIterKind, any separated intents/locks
-	// will also be cleared.
-	//
-	// It is safe to modify the contents of the arguments after ClearIterRange
-	// returns.
-	ClearIterRange(iter MVCCIterator, start, end roachpb.Key) error
+	// ClearIterRange removes all keys in the given span using an iterator to
+	// iterate over point keys and remove them from the storage engine using
+	// per-key storage tombstones (not MVCC tombstones). Any separated
+	// intents/locks will also be cleared.
+	ClearIterRange(start, end roachpb.Key) error
 
 	// Merge is a high-performance write operation used for values which are
 	// accumulated over several writes. Multiple values can be merged

pkg/storage/engine_test.go

@@ -1013,9 +1013,7 @@ func TestEngineDeleteIterRange(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 	testEngineDeleteRange(t, func(engine Engine, start, end MVCCKey) error {
-		iter := engine.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax})
-		defer iter.Close()
-		return engine.ClearIterRange(iter, start.Key, end.Key)
+		return engine.ClearIterRange(start.Key, end.Key)
 	})
 }
 

pkg/storage/intent_interleaving_iter.go

@@ -80,7 +80,7 @@ const (
 // bounds, the call to newIntentInterleavingIter must have specified at least
 // one of the lower or upper bound. We use that to "constrain" the iterator as
 // either a local key iterator or global key iterator and panic if a caller
-// violates that in a subsequent SeekGE/SeekLT/SetUpperBound call.
+// violates that in a subsequent SeekGE/SeekLT call.
 type intentInterleavingIter struct {
 	prefix     bool
 	constraint intentInterleavingIterConstraint
@@ -954,17 +954,6 @@ func (i *intentInterleavingIter) FindSplitKey(
 	return findSplitKeyUsingIterator(i, start, end, minSplitKey, targetSize)
 }
 
-func (i *intentInterleavingIter) SetUpperBound(key roachpb.Key) {
-	i.iter.SetUpperBound(key)
-	// Preceding call to SetUpperBound has confirmed that key != nil.
-	if i.constraint != notConstrained {
-		i.checkConstraint(key, true)
-	}
-	var intentUpperBound roachpb.Key
-	intentUpperBound, i.intentKeyBuf = keys.LockTableSingleKey(key, i.intentKeyBuf)
-	i.intentIter.SetUpperBound(intentUpperBound)
-}
-
 func (i *intentInterleavingIter) Stats() IteratorStats {
 	stats := i.iter.Stats()
 	intentStats := i.intentIter.Stats()
@@ -988,12 +977,7 @@ func (i *intentInterleavingIter) SupportsPrev() bool {
 // hints. It uses pebble.Iterator.Clone to ensure that the two iterators see
 // the identical engine state.
 func newMVCCIteratorByCloningEngineIter(iter EngineIterator, opts IterOptions) MVCCIterator {
-	pIter := iter.GetRawIter()
-	it := newPebbleIterator(nil, pIter, opts, StandardDurability)
-	if iter == nil {
-		panic("couldn't create a new iterator")
-	}
-	return it
+	return newPebbleIterator(nil, iter.GetRawIter(), opts, StandardDurability)
 }
 
 // unsageMVCCIterator is used in RaceEnabled test builds to randomly inject

pkg/storage/intent_interleaving_iter_test.go

@@ -184,7 +184,7 @@ func checkAndOutputIter(iter MVCCIterator, b *strings.Builder) {
 // - iter: for iterating, is defined as
 //   iter [lower=<lower>] [upper=<upper>] [prefix=<true|false>]
 //   followed by newline separated sequence of operations:
-//     next, prev, seek-lt, seek-ge, set-upper, next-key, stats
+//     next, prev, seek-lt, seek-ge, next-key, stats
 //
 // Keys are interpreted as:
 // - starting with L is interpreted as a local-range key.
@@ -362,10 +362,6 @@ func TestIntentInterleavingIter(t *testing.T) {
 						iter.NextKey()
 						fmt.Fprintf(&b, "next-key: ")
 						checkAndOutputIter(iter, &b)
-					case "set-upper":
-						k := scanRoachKey(t, d, "k")
-						iter.SetUpperBound(k)
-						fmt.Fprintf(&b, "set-upper %s\n", string(makePrintableKey(MVCCKey{Key: k}).Key))
 					case "stats":
 						stats := iter.Stats()
 						// Setting non-deterministic InternalStats to empty.
@@ -395,17 +391,13 @@ func TestIntentInterleavingIterBoundaries(t *testing.T) {
 		iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter)
 		defer iter.Close()
 		require.Equal(t, constrainedToLocal, iter.constraint)
-		iter.SetUpperBound(keys.LocalMax)
-		require.Equal(t, constrainedToLocal, iter.constraint)
 		iter.SeekLT(MVCCKey{Key: keys.LocalMax})
 	}()
 	func() {
 		opts := IterOptions{UpperBound: keys.LocalMax}
 		iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter)
 		defer iter.Close()
 		require.Equal(t, constrainedToLocal, iter.constraint)
-		iter.SetUpperBound(keys.LocalMax)
-		require.Equal(t, constrainedToLocal, iter.constraint)
 	}()
 	require.Panics(t, func() {
 		opts := IterOptions{UpperBound: keys.LocalMax}
@@ -420,13 +412,6 @@ func TestIntentInterleavingIterBoundaries(t *testing.T) {
 		defer iter.Close()
 		require.Equal(t, constrainedToGlobal, iter.constraint)
 	}()
-	require.Panics(t, func() {
-		opts := IterOptions{LowerBound: keys.LocalMax}
-		iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter)
-		defer iter.Close()
-		require.Equal(t, constrainedToGlobal, iter.constraint)
-		iter.SetUpperBound(keys.LocalMax)
-	})
 	func() {
 		opts := IterOptions{LowerBound: keys.LocalMax}
 		iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter)
@@ -923,14 +908,12 @@ func BenchmarkIntentInterleavingSeekGEAndIter(b *testing.B) {
 						iter = state.eng.NewMVCCIterator(MVCCKeyIterKind, opts)
 					}
 					b.ResetTimer()
-					var unsafeKey MVCCKey
 					for i := 0; i < b.N; i++ {
 						j := i % len(seekKeys)
 						upperIndex := j + 1
+						scanTo := MVCCKey{Key: endKey}
 						if upperIndex < len(seekKeys) {
-							iter.SetUpperBound(seekKeys[upperIndex])
-						} else {
-							iter.SetUpperBound(endKey)
+							scanTo.Key = seekKeys[upperIndex]
 						}
 						iter.SeekGE(MVCCKey{Key: seekKeys[j]})
 						for {
@@ -941,11 +924,12 @@ func BenchmarkIntentInterleavingSeekGEAndIter(b *testing.B) {
 							if !valid {
 								break
 							}
-							unsafeKey = iter.UnsafeKey()
+							if iter.UnsafeKey().Compare(scanTo) >= 0 {
+								break
+							}
 							iter.Next()
 						}
 					}
-					_ = unsafeKey
 				})
 		}
 	})