internal/keyspan: simplify SeekLE, fix nil-pointer

Similar to #3209, this commit simplifies SeekLE for the new (as of #2146)
FragmentIterator semantics. It also fixes a nil-pointer access if
iter.Error()!= nil after Next-ing the iterator in the second else block.

internal/keyspan/iter.go

@@ -17,6 +17,11 @@ import (
 // positioning method. Some implementations (eg, keyspan.Iter) may provide
 // longer lifetimes but implementations need only guarantee stability until the
 // next positioning method.
+//
+// If any positioning method fails to find a span, the iterator is left
+// positioned at an exhausted position in the direction of iteration. For
+// example, a caller than finds SeekGE(k)=nil may call Prev to move the iterator
+// to the last span.
 type FragmentIterator interface {
 	// SeekGE moves the iterator to the first span covering a key greater than
 	// or equal to the given key. This is equivalent to seeking to the first

internal/keyspan/seek.go

@@ -6,45 +6,27 @@ package keyspan
 
 import "github.com/cockroachdb/pebble/internal/base"
 
-// SeekLE seeks to the span that contains or is before the target key.
-func SeekLE(cmp base.Compare, iter FragmentIterator, key []byte) *Span {
-	// NB: We use SeekLT in order to land on the proper span for a search
-	// key that resides in the middle of a span. Consider the scenario:
-	//
-	//     a---e
-	//         e---i
-	//
-	// The spans are indexed by their start keys `a` and `e`. If the
-	// search key is `c` we want to land on the span [a,e). If we were to
-	// use SeekGE then the search key `c` would land on the span [e,i) and
-	// we'd have to backtrack. The one complexity here is what happens for the
-	// search key `e`. In that case SeekLT will land us on the span [a,e)
-	// and we'll have to move forward.
-	iterSpan := iter.SeekLT(key)
-
+// SeekLE seeks to the span that contains or is before the target key. If an
+// error occurs while seeking iter, a nil span and non-nil error is returned.
+func SeekLE(cmp base.Compare, iter FragmentIterator, key []byte) (*Span, error) {
+	// Seek to the smallest span that contains a key ≥ key. If some span
+	// contains the key `key`, SeekGE will return it.
+	iterSpan := iter.SeekGE(key)
 	if iterSpan == nil {
-		if iter.Error() == nil {
-			// Advance the iterator once to see if the next span has a start key
-			// equal to key.
-			iterSpan = iter.Next()
-			if iterSpan == nil || cmp(key, iterSpan.Start) < 0 {
-				// The iterator is exhausted or we've hit the next span.
-				return nil
-			}
-		}
-	} else {
-		// Invariant: key > iterSpan.Start
-		if cmp(key, iterSpan.End) >= 0 {
-			// The current span lies entirely before the search key. Check to see if
-			// the next span contains the search key. If it doesn't, we'll backup
-			// and return to our earlier candidate.
-			iterSpan = iter.Next()
-			if (iterSpan == nil && iter.Error() == nil) || cmp(key, iterSpan.Start) < 0 {
-				// The next span is past our search key or there is no next span. Go
-				// back.
-				iterSpan = iter.Prev()
-			}
+		if err := iter.Error(); err != nil {
+			return nil, err
 		}
+		// Fallthrough to Prev()-ing.
+	} else if cmp(key, iterSpan.Start) >= 0 {
+		return iterSpan, nil
+	}
+
+	// No span covers exactly `key`. Step backwards to move onto the largest
+	// span < key.
+	iterSpan = iter.Prev()
+	if iterSpan == nil {
+		// NB: iter.Error() may be nil or non-nil.
+		return iterSpan, iter.Error()
 	}
-	return iterSpan
+	return iterSpan, nil
 }

internal/keyspan/seek_test.go

@@ -18,24 +18,32 @@ import (
 func TestSeek(t *testing.T) {
 	cmp := base.DefaultComparer.Compare
 	fmtKey := base.DefaultComparer.FormatKey
-	var iter FragmentIterator
 	var buf bytes.Buffer
+	var spans []Span
 
 	datadriven.RunTest(t, "testdata/seek", func(t *testing.T, d *datadriven.TestData) string {
 		buf.Reset()
 		switch d.Cmd {
 		case "build":
-			spans := buildSpans(t, cmp, fmtKey, d.Input, base.InternalKeyKindRangeDelete)
+			spans = buildSpans(t, cmp, fmtKey, d.Input, base.InternalKeyKindRangeDelete)
 			for _, s := range spans {
 				fmt.Fprintln(&buf, s)
 			}
-			iter = NewIter(cmp, spans)
 			return buf.String()
 		case "seek-ge", "seek-le":
+			var iter FragmentIterator = NewIter(cmp, spans)
+			if cmdArg, ok := d.Arg("probes"); ok {
+				iter = attachProbes(iter, probeContext{log: &buf},
+					parseProbes(cmdArg.Vals...)...)
+			}
+
 			seek := SeekLE
 			if d.Cmd == "seek-ge" {
-				seek = func(_ base.Compare, iter FragmentIterator, key []byte) *Span {
-					return iter.SeekGE(key)
+				seek = func(_ base.Compare, iter FragmentIterator, key []byte) (*Span, error) {
+					if s := iter.SeekGE(key); s != nil {
+						return s, nil
+					}
+					return nil, iter.Error()
 				}
 			}
 
@@ -48,12 +56,16 @@ func TestSeek(t *testing.T) {
 				if err != nil {
 					return err.Error()
 				}
-				span := seek(cmp, iter, []byte(parts[0]))
-				if span != nil {
+				span, err := seek(cmp, iter, []byte(parts[0]))
+				if err != nil {
+					fmt.Fprintf(&buf, "<nil> <err=%q>\n", err)
+				} else if span == nil {
+					fmt.Fprintln(&buf, "<nil>")
+				} else {
 					visible := span.Visible(seq)
 					span = &visible
+					fmt.Fprintln(&buf, span)
 				}
-				fmt.Fprintln(&buf, span)
 			}
 			return buf.String()
 		default:

internal/keyspan/testdata/seek

@@ -14,6 +14,21 @@ b-d:{(#1,RANGEDEL)}
 b-d:{}
 <nil>
 
+seek-ge probes=(ErrInjected,(Log "#  iter."))
+a 2
+b 2
+b 1
+d 2
+----
+#  iter.SeekGE("a") = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("b") = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("b") = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("d") = nil <err="injected error">
+<nil> <err="injected error">
+
 seek-le
 a 2
 b 2
@@ -25,6 +40,21 @@ b-d:{(#1,RANGEDEL)}
 b-d:{}
 b-d:{(#1,RANGEDEL)}
 
+seek-le probes=(ErrInjected,(Log "#  iter."))
+a 2
+b 2
+b 1
+d 2
+----
+#  iter.SeekGE("a") = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("b") = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("b") = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("d") = nil <err="injected error">
+<nil> <err="injected error">
+
 build
 3:  b-d
 2:  b-d
@@ -307,3 +337,33 @@ c-e:{}
 c-e:{}
 c-e:{}
 c-e:{(#5,RANGEDEL)}
+
+build
+1: a-c
+2: f-g
+----
+a-c:{(#1,RANGEDEL)}
+f-g:{(#2,RANGEDEL)}
+
+seek-le
+c 5
+d 5
+f 5
+----
+a-c:{(#1,RANGEDEL)}
+a-c:{(#1,RANGEDEL)}
+f-g:{(#2,RANGEDEL)}
+
+seek-le probes=((If OpPrev ErrInjected noop),(Log "#  iter."))
+c 5
+d 5
+f 5
+----
+#  iter.SeekGE("c") = f-g:{(#2,RANGEDEL)}
+#  iter.Prev() = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("d") = f-g:{(#2,RANGEDEL)}
+#  iter.Prev() = nil <err="injected error">
+<nil> <err="injected error">
+#  iter.SeekGE("f") = f-g:{(#2,RANGEDEL)}
+f-g:{(#2,RANGEDEL)}

level_iter_test.go

@@ -362,7 +362,14 @@ func (i *levelIterTestIter) rangeDelSeek(
 	if i.rangeDelIter != nil {
 		var t *keyspan.Span
 		if dir < 0 {
-			t = keyspan.SeekLE(i.levelIter.cmp, i.rangeDelIter, key)
+			var err error
+			t, err = keyspan.SeekLE(i.levelIter.cmp, i.rangeDelIter, key)
+			// TODO(jackson): Clean this up when the FragmentIterator interface
+			// is refactored to return an error return value from all
+			// positioning methods.
+			if err != nil {
+				panic(err)
+			}
 		} else {
 			t = i.rangeDelIter.SeekGE(key)
 		}

merging_iter.go

@@ -406,12 +406,11 @@ func (m *mergingIter) initMaxRangeDelIters(oldTopLevel int) error {
 		if l.rangeDelIter == nil {
 			continue
 		}
-		l.tombstone = keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.iterKey.UserKey)
-		if l.tombstone == nil {
-			if err := l.rangeDelIter.Error(); err != nil {
-				return err
-			}
+		tomb, err := keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.iterKey.UserKey)
+		if err != nil {
+			return err
 		}
+		l.tombstone = tomb
 	}
 	return nil
 }
@@ -967,12 +966,12 @@ func (m *mergingIter) isPrevEntryDeleted(item *mergingIterLevel) (bool, error) {
 			// levelIter in the future cannot contain item.iterKey). Also, it is it is possible that we
 			// will encounter parts of the range delete that should be ignored -- we handle that
 			// below.
-			l.tombstone = keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.iterKey.UserKey)
-			if l.tombstone == nil {
-				if err := l.rangeDelIter.Error(); err != nil {
-					return false, err
-				}
+
+			tomb, err := keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.iterKey.UserKey)
+			if err != nil {
+				return false, err
 			}
+			l.tombstone = tomb
 		}
 		if l.tombstone == nil {
 			continue
@@ -1281,12 +1280,11 @@ func (m *mergingIter) seekLT(key []byte, level int, flags base.SeekLTFlags) erro
 				withinLargestSSTableBound = cmpResult > 0 || (cmpResult == 0 && !l.isLargestUserKeyExclusive)
 			}
 
-			l.tombstone = keyspan.SeekLE(m.heap.cmp, rangeDelIter, key)
-			if l.tombstone == nil {
-				if err := rangeDelIter.Error(); err != nil {
-					return err
-				}
+			tomb, err := keyspan.SeekLE(m.heap.cmp, rangeDelIter, key)
+			if err != nil {
+				return err
 			}
+			l.tombstone = tomb
 			if l.tombstone != nil && l.tombstone.VisibleAt(m.snapshot) &&
 				l.tombstone.Contains(m.heap.cmp, key) && withinLargestSSTableBound {
 				// NB: Based on the comment above l.smallestUserKey <= key, and based

testdata/merging_iter

@@ -829,13 +829,13 @@ seek-prefix-ge b
 #  rangedelIter.opSpanSeekGE("a") = nil <err="injected error">
 err=injected error
 #  iter.Last() = (f#21,1,"21")
-#  rangedelIter.opSpanSeekLT("f") = nil <err="injected error">
+#  rangedelIter.opSpanSeekGE("f") = nil <err="injected error">
 err=injected error
 #  iter.SeekGE("boo") = (c#18,1,"18")
 #  rangedelIter.opSpanSeekGE("boo") = nil <err="injected error">
 err=injected error
 #  iter.SeekLT("coo") = (c#18,1,"18")
-#  rangedelIter.opSpanSeekLT("coo") = nil <err="injected error">
+#  rangedelIter.opSpanSeekGE("coo") = nil <err="injected error">
 err=injected error
 #  iter.SeekPrefixGE("b") = (b#19,1,"19")
 #  rangedelIter.opSpanSeekGE("b") = nil <err="injected error">