db: remove ExternalIterForwardOnly

Remove the ExternalIterForwardOnly option that enabled a specific iterator
optimization for extenral iterators that would move exclusively monotonically
forward. The NextPrefix optimization removed the repeated SeekGE, Next usage of
external iterators, making this optimization unnecessary.

data_test.go

@@ -118,8 +118,6 @@ func runIterCmd(d *datadriven.TestData, iter *Iterator, closeIter bool) string {
 					op = "seekge"
 				case seekLTLastPositioningOp:
 					op = "seeklt"
-				case invalidatedLastPositionOp:
-					op = "invalidate"
 				}
 				fmt.Fprintf(&b, "%s=%q\n", field, op)
 			default:

external_iterator.go

@@ -44,21 +44,6 @@ func ExternalIterReaderOptions(opts ...sstable.ReaderOption) ExternalIterOption
 	return &externalIterReaderOptions{opts: opts}
 }
 
-// ExternalIterForwardOnly is an ExternalIterOption that specifies this iterator
-// will only be used for forward positioning operations (First, SeekGE, Next).
-// This could enable optimizations that take advantage of this invariant.
-// Behaviour when a reverse positioning operation is done on an iterator
-// opened with this option is unpredictable, though in most cases it should.
-type ExternalIterForwardOnly struct{}
-
-func (e ExternalIterForwardOnly) iterApply(iter *Iterator) {
-	iter.forwardOnly = true
-}
-
-func (e ExternalIterForwardOnly) readerOptions() []sstable.ReaderOption {
-	return nil
-}
-
 // NewExternalIter takes an input 2d array of sstable files which may overlap
 // across subarrays but not within a subarray (at least as far as points are
 // concerned; range keys are allowed to overlap arbitrarily even within a
@@ -223,14 +208,6 @@ func createExternalPointIter(ctx context.Context, it *Iterator) (internalIterato
 			if err != nil {
 				return nil, err
 			}
-			if rangeDelIter == nil && pointIter != nil && it.forwardOnly {
-				// TODO(bilal): Consider implementing range key pausing in
-				// simpleLevelIter so we can reduce mergingIterLevels even more by
-				// sending all sstable iterators to combinedIters, not just those
-				// corresponding to sstables without range deletes.
-				combinedIters = append(combinedIters, pointIter)
-				continue
-			}
 			mlevels = append(mlevels, mergingIterLevel{
 				iter:         pointIter,
 				rangeDelIter: rangeDelIter,

external_iterator_test.go

@@ -52,8 +52,6 @@ func TestExternalIterator(t *testing.T) {
 			var files [][]sstable.ReadableFile
 			for _, arg := range td.CmdArgs {
 				switch arg.Key {
-				case "fwd-only":
-					externalIterOpts = append(externalIterOpts, ExternalIterForwardOnly{})
 				case "mask-suffix":
 					opts.RangeKeyMasking.Suffix = []byte(arg.Vals[0])
 				case "lower":
@@ -309,9 +307,9 @@ func TestIterRandomizedMaybeFilteredKeys(t *testing.T) {
 	}
 }
 
-func BenchmarkExternalIter_NonOverlapping_SeekNextScan(b *testing.B) {
+func BenchmarkExternalIter_NonOverlapping_Scan(b *testing.B) {
 	ks := testkeys.Alpha(6)
-	opts := (&Options{}).EnsureDefaults()
+	opts := (&Options{Comparer: testkeys.Comparer}).EnsureDefaults()
 	iterOpts := &IterOptions{
 		KeyTypes: IterKeyTypePointsAndRanges,
 	}
@@ -338,40 +336,31 @@ func BenchmarkExternalIter_NonOverlapping_SeekNextScan(b *testing.B) {
 						filenames[i] = filename
 					}
 
-					for _, forwardOnly := range []bool{false, true} {
-						b.Run(fmt.Sprintf("forward-only=%t", forwardOnly), func(b *testing.B) {
-							var externalIterOpts []ExternalIterOption
-							if forwardOnly {
-								externalIterOpts = append(externalIterOpts, ExternalIterForwardOnly{})
+					b.ResetTimer()
+					for i := 0; i < b.N; i++ {
+						func() {
+							files := make([][]sstable.ReadableFile, fileCount)
+							for i := 0; i < fileCount; i++ {
+								f, err := fs.Open(filenames[i])
+								require.NoError(b, err)
+								files[i] = []sstable.ReadableFile{f}
 							}
 
-							for i := 0; i < b.N; i++ {
-								func() {
-									files := make([][]sstable.ReadableFile, fileCount)
-									for i := 0; i < fileCount; i++ {
-										f, err := fs.Open(filenames[i])
-										require.NoError(b, err)
-										files[i] = []sstable.ReadableFile{f}
-									}
+							it, err := NewExternalIter(opts, iterOpts, files)
+							require.NoError(b, err)
+							defer it.Close()
 
-									it, err := NewExternalIter(opts, iterOpts, files, externalIterOpts...)
-									require.NoError(b, err)
-									defer it.Close()
-
-									for k := 0; k+1 < len(keys); k += 2 {
-										if !it.SeekGE(keys[k]) {
-											b.Fatalf("key %q not found", keys[k])
-										}
-										if !it.Next() {
-											b.Fatalf("key %q not found", keys[k+1])
-										}
-										if !bytes.Equal(it.Key(), keys[k+1]) {
-											b.Fatalf("expected key %q, found %q", keys[k+1], it.Key())
-										}
-									}
-								}()
+							k := 0
+							for valid := it.First(); valid; valid = it.NextPrefix() {
+								if !bytes.Equal(it.Key(), keys[k]) {
+									b.Fatalf("expected key %q, found %q", keys[k+1], it.Key())
+								}
+								k++
+							}
+							if k != len(keys) {
+								b.Fatalf("k=%d, expected %d\n", k, len(keys))
 							}
-						})
+						}()
 					}
 				})
 			}

iterator.go

@@ -299,9 +299,6 @@ type Iterator struct {
 	//   batchIter, Seek[Prefix]GE set flags.BatchJustRefreshed()=true if this
 	//   bit is enabled.
 	batchJustRefreshed bool
-	// Used for an optimization in external iterators to reduce the number of
-	// merging levels.
-	forwardOnly bool
 	// batchOnlyIter is set to true for Batch.NewBatchOnlyIter.
 	batchOnlyIter bool
 	// closePointIterOnce is set to true if this point iter can only be Close()d
@@ -454,14 +451,6 @@ const (
 	// around calling CanDeterministicallySingleDelete at most once per external
 	// iterator position.
 	internalNextOp
-	// invalidatedLastPositionOp is similar to unknownLastPositionOp and the
-	// only reason to distinguish this is for the wider set of SeekGE
-	// optimizations we permit for the external iterator Iterator.forwardOnly
-	// case. Most code predicates should be doing equality comparisons with one
-	// of the seek* enum values, so this duplication should not result in code
-	// of the form:
-	//  if unknownLastPositionOp || invalidLastPositionOp
-	invalidatedLastPositionOp
 )
 
 // Limited iteration mode. Not for use with prefix iteration.
@@ -1275,7 +1264,6 @@ func (i *Iterator) SeekGEWithLimit(key []byte, limit []byte) IterValidityState {
 		i.rangeKey.updated = i.rangeKey.hasRangeKey && !i.Valid() && i.opts.rangeKeys()
 	}
 	lastPositioningOp := i.lastPositioningOp
-	hasPrefix := i.hasPrefix
 	// Set it to unknown, since this operation may not succeed, in which case
 	// the SeekGE following this should not make any assumption about iterator
 	// position.
@@ -1339,25 +1327,6 @@ func (i *Iterator) SeekGEWithLimit(key []byte, limit []byte) IterValidityState {
 			}
 		}
 	}
-	// Check for another TrySeekUsingNext optimization opportunity, currently
-	// specifically tailored to external iterators. This case is intended to
-	// trigger in instances of Seek-ing with monotonically increasing keys with
-	// Nexts interspersed. At the time of writing, this is the case for
-	// CockroachDB scans. This optimization is important for external iterators
-	// to avoid re-seeking within an already-exhausted sstable. It is not always
-	// a performance win more generally, so we restrict it to external iterators
-	// that are configured to only use forward positioning operations.
-	//
-	// TODO(jackson): This optimization should be obsolete once we introduce and
-	// use the NextPrefix iterator positioning operation.
-	if seekInternalIter && i.forwardOnly && lastPositioningOp != invalidatedLastPositionOp &&
-		i.pos == iterPosCurForward && !hasPrefix && i.iterValidityState == IterValid &&
-		i.cmp(key, i.iterKey.UserKey) > 0 {
-		flags = flags.EnableTrySeekUsingNext()
-		if invariants.Enabled && flags.TrySeekUsingNext() && !i.forceEnableSeekOpt && disableSeekOpt(key, uintptr(unsafe.Pointer(i))) {
-			flags = flags.DisableTrySeekUsingNext()
-		}
-	}
 	if seekInternalIter {
 		i.iterKey, i.iterValue = i.iter.SeekGE(key, flags)
 		i.stats.ForwardSeekCount[InternalIterCall]++
@@ -2733,7 +2702,7 @@ func (i *Iterator) SetOptions(o *IterOptions) {
 }
 
 func (i *Iterator) invalidate() {
-	i.lastPositioningOp = invalidatedLastPositionOp
+	i.lastPositioningOp = unknownLastPositionOp
 	i.hasPrefix = false
 	i.iterKey = nil
 	i.iterValue = LazyValue{}

testdata/external_iterator

@@ -26,7 +26,7 @@ next
 c: (c, .)
 .
 
-iter files=(2, 1) fwd-only
+iter files=(2, 1)
 first
 next
 ----
@@ -42,7 +42,7 @@ set f f
 # the rangedel. Since the point keys are assigned a higher sequence
 # number, they should NOT be shadowed by the rangedel.
 
-iter files=(3, 2, 1) fwd-only
+iter files=(3, 2, 1)
 first
 next
 next
@@ -65,7 +65,7 @@ build 5
 range-key-del b d
 ----
 
-iter files=(5, 4, 3, 2, 1) fwd-only
+iter files=(5, 4, 3, 2, 1)
 first
 next
 next
@@ -127,7 +127,7 @@ build ek
 range-key-set e k @5 foo
 ----
 
-iter files=(ag, ek) fwd-only
+iter files=(ag, ek)
 first
 next
 ----
@@ -147,7 +147,7 @@ set k@3 v
 set p@4 v
 ----
 
-iter files=(points, ag, ek) mask-suffix=@7 fwd-only
+iter files=(points, ag, ek) mask-suffix=@7
 first
 next
 next
@@ -170,7 +170,7 @@ range-key-set a k @4 bar
 range-key-set a k @1 bax
 ----
 
-iter files=(points, ag, ek, stacked) fwd-only
+iter files=(points, ag, ek, stacked)
 first
 next
 ----
@@ -179,7 +179,7 @@ a@4: (v, [a-k) @5=foo, @4=bar, @1=bax)
 
 # Test mutating the external iterator's options through SetOptions.
 
-iter files=(points, ag, ek) fwd-only
+iter files=(points, ag, ek)
 set-options key-types=point
 first
 next
@@ -194,11 +194,6 @@ c@2: (v, .)
 e@5: (v, .)
 k@3: (v, .)
 
-# Test the TrySeekUsingNext optimization that's enabled only for fwd-only
-# external iterators. Seed the database with keys like 'a', 'aa', 'aaa', etc so
-# that the index block uses a final separator that's beyond all the other live
-# keys.
-
 reset
 ----
 
@@ -227,9 +222,6 @@ set aaaaa@3 aaaaa@3
 set aaaaa@1 aaaaa@1
 ----
 
-# Note the absence of fwd-only. This iterator will not use the TrySeekUsingNext
-# optimization.
-
 iter files=(a, aa, aaa, aaaa, aaaaa)
 seek-ge a
 next
@@ -255,32 +247,3 @@ aaaaa@3: (aaaaa@3, .)
 aaaaa@1: (aaaaa@1, .)
 stats: (interface (dir, seek, step): (fwd, 5, 5), (rev, 0, 0)), (internal (dir, seek, step): (fwd, 5, 5), (rev, 0, 0)),
 (internal-stats: (block-bytes: (total 547B, cached 0B, read-time 0s)), (points: (count 10, key-bytes 50B, value-bytes 35B, tombstoned 0)), (separated: (count 15, bytes 65B, fetched 25B)))
-
-# Note the inclusion of fwd-only. This iterator will use the TrySeekUsingNext
-# optimization and loads ~half the block-bytes as a result.
-
-iter files=(a, aa, aaa, aaaa, aaaaa) fwd-only
-seek-ge a
-next
-seek-ge aa
-next
-seek-ge aaa
-next
-seek-ge aaaa
-next
-seek-ge aaaaa
-next
-stats
-----
-a@3: (a@3, .)
-a@1: (a@1, .)
-aa@3: (aa@3, .)
-aa@1: (aa@1, .)
-aaa@3: (aaa@3, .)
-aaa@1: (aaa@1, .)
-aaaa@3: (aaaa@3, .)
-aaaa@1: (aaaa@1, .)
-aaaaa@3: (aaaaa@3, .)
-aaaaa@1: (aaaaa@1, .)
-stats: (interface (dir, seek, step): (fwd, 5, 5), (rev, 0, 0)), (internal (dir, seek, step): (fwd, 5, 5), (rev, 0, 0)),
-(internal-stats: (block-bytes: (total 336B, cached 0B, read-time 0s)), (points: (count 10, key-bytes 50B, value-bytes 35B, tombstoned 0)), (separated: (count 5, bytes 25B, fetched 25B)))

testdata/iter_histories/iter_optimizations

@@ -340,7 +340,7 @@ seek-ge b@7
 .
 .
 .
-lastPositioningOp="invalidate"
+lastPositioningOp="unknown"
 b@6: (b@6, .)
 
 reset