db: refactor table cache newIters

This commit refactors the interface of the table cache `newIters` method to
support creation of any of a sstable's iterators, including its point iterator,
range deletion iterator and range key iterator. This is motivated by the
refactor planned in cockroachdb#2863, which in turn requires refactoring some of the
newIters call sites that determine whether ingested sstables overlap existing
tables.

There's also a minor benefit that a compaction setup no longer needs to open a
point iterator and then immediately close it when initializing the range
deletion iterators.

```
goos: linux
goarch: amd64
pkg: github.com/cockroachdb/pebble
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
              │ old-NewItersAlloc.txt │       new-NewItersAlloc.txt        │
              │        sec/op         │   sec/op     vs base               │
NewItersAlloc             427.0n ± 1%   412.1n ± 1%  -3.47% (p=0.000 n=10)

              │ old-NewItersAlloc.txt │     new-NewItersAlloc.txt      │
              │         B/op          │    B/op     vs base            │
NewItersAlloc              0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

              │ old-NewItersAlloc.txt │     new-NewItersAlloc.txt      │
              │       allocs/op       │ allocs/op   vs base            │
NewItersAlloc              0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal
```

compaction.go

@@ -1427,38 +1427,22 @@ func (c *compaction) newRangeDelIter(
 	bytesIterated *uint64,
 ) (keyspan.FragmentIterator, io.Closer, error) {
 	opts.level = l
-	iter, rangeDelIter, err := newIters(context.Background(), f.FileMetadata,
+	iterSet, err := newIters(context.Background(), f.FileMetadata,
 		&opts, internalIterOpts{
 			bytesIterated: &c.bytesIterated,
 			bufferPool:    &c.bufferPool,
-		})
+		}, iterRangeDeletions)
 	if err != nil {
 		return nil, nil, err
-	}
-	// TODO(peter): It is mildly wasteful to open the point iterator only to
-	// immediately close it. One way to solve this would be to add new
-	// methods to tableCache for creating point and range-deletion iterators
-	// independently. We'd only want to use those methods here,
-	// though. Doesn't seem worth the hassle in the near term.
-	if err = iter.Close(); err != nil {
-		if rangeDelIter != nil {
-			err = errors.CombineErrors(err, rangeDelIter.Close())
-		}
-		return nil, nil, err
-	}
-	if rangeDelIter == nil {
+	} else if iterSet.rangeDeletion == nil {
 		// The file doesn't contain any range deletions.
 		return nil, nil, nil
 	}
-
 	// Ensure that rangeDelIter is not closed until the compaction is
 	// finished. This is necessary because range tombstone processing
 	// requires the range tombstones to be held in memory for up to the
 	// lifetime of the compaction.
-	closer := rangeDelIter
-	rangeDelIter = noCloseIter{rangeDelIter}
-
-	return rangeDelIter, closer, nil
+	return noCloseIter{iterSet.rangeDeletion}, iterSet.rangeDeletion, nil
 }
 
 func (c *compaction) String() string {

compaction_test.go

@@ -2891,9 +2891,9 @@ func TestCompactionCheckOrdering(t *testing.T) {
 				}
 
 				newIters := func(
-					_ context.Context, _ *manifest.FileMetadata, _ *IterOptions, _ internalIterOpts,
-				) (internalIterator, keyspan.FragmentIterator, error) {
-					return &errorIter{}, nil, nil
+					_ context.Context, _ *manifest.FileMetadata, _ *IterOptions, _ internalIterOpts, _ iterKinds,
+				) (iterSet, error) {
+					return iterSet{point: &errorIter{}}, nil
 				}
 				result := "OK"
 				_, err := c.newInputIter(newIters, nil, nil)

db.go

@@ -3076,42 +3076,33 @@ func (d *DB) checkVirtualBounds(m *fileMetadata) {
 		return
 	}
 
-	if m.HasPointKeys {
-		pointIter, rangeDelIter, err := d.newIters(context.TODO(), m, nil, internalIterOpts{})
-		if err != nil {
-			panic(errors.Wrap(err, "pebble: error creating point iterator"))
-		}
+	iters, err := d.newIters.all(context.TODO(), m, nil, internalIterOpts{})
+	if err != nil {
+		panic(errors.Wrap(err, "pebble: error creating iterators"))
+	}
+	defer iters.CloseAll()
 
-		defer pointIter.Close()
-		if rangeDelIter != nil {
-			defer rangeDelIter.Close()
-		}
+	if m.HasPointKeys {
+		pointIter := iters.Point()
+		rangeDelIter := iters.RangeDeletion()
 
+		// Check that the lower bound is tight.
 		pointKey, _ := pointIter.First()
-		var rangeDel *keyspan.Span
-		if rangeDelIter != nil {
-			rangeDel, err = rangeDelIter.First()
-			if err != nil {
-				panic(err)
-			}
+		rangeDel, err := rangeDelIter.First()
+		if err != nil {
+			panic(err)
 		}
-
-		// Check that the lower bound is tight.
 		if (rangeDel == nil || d.cmp(rangeDel.SmallestKey().UserKey, m.SmallestPointKey.UserKey) != 0) &&
 			(pointKey == nil || d.cmp(pointKey.UserKey, m.SmallestPointKey.UserKey) != 0) {
 			panic(errors.Newf("pebble: virtual sstable %s lower point key bound is not tight", m.FileNum))
 		}
 
+		// Check that the upper bound is tight.
 		pointKey, _ = pointIter.Last()
-		rangeDel = nil
-		if rangeDelIter != nil {
-			rangeDel, err = rangeDelIter.Last()
-			if err != nil {
-				panic(err)
-			}
+		rangeDel, err = rangeDelIter.Last()
+		if err != nil {
+			panic(err)
 		}
-
-		// Check that the upper bound is tight.
 		if (rangeDel == nil || d.cmp(rangeDel.LargestKey().UserKey, m.LargestPointKey.UserKey) != 0) &&
 			(pointKey == nil || d.cmp(pointKey.UserKey, m.LargestPointKey.UserKey) != 0) {
 			panic(errors.Newf("pebble: virtual sstable %s upper point key bound is not tight", m.FileNum))
@@ -3123,32 +3114,24 @@ func (d *DB) checkVirtualBounds(m *fileMetadata) {
 				panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, key.UserKey))
 			}
 		}
-
-		if rangeDelIter != nil {
-			s, err := rangeDelIter.First()
-			for ; s != nil; s, err = rangeDelIter.Next() {
-				if d.cmp(s.SmallestKey().UserKey, m.SmallestPointKey.UserKey) < 0 {
-					panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.SmallestKey().UserKey))
-				}
-				if d.cmp(s.LargestKey().UserKey, m.LargestPointKey.UserKey) > 0 {
-					panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.LargestKey().UserKey))
-				}
+		s, err := rangeDelIter.First()
+		for ; s != nil; s, err = rangeDelIter.Next() {
+			if d.cmp(s.SmallestKey().UserKey, m.SmallestPointKey.UserKey) < 0 {
+				panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.SmallestKey().UserKey))
 			}
-			if err != nil {
-				panic(err)
+			if d.cmp(s.LargestKey().UserKey, m.LargestPointKey.UserKey) > 0 {
+				panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.LargestKey().UserKey))
 			}
 		}
+		if err != nil {
+			panic(err)
+		}
 	}
 
 	if !m.HasRangeKeys {
 		return
 	}
-
-	rangeKeyIter, err := d.tableNewRangeKeyIter(m, keyspan.SpanIterOptions{})
-	if err != nil {
-		panic(errors.Wrap(err, "pebble: error creating range key iterator"))
-	}
-	defer rangeKeyIter.Close()
+	rangeKeyIter := iters.RangeKey()
 
 	// Check that the lower bound is tight.
 	if s, err := rangeKeyIter.First(); err != nil {

external_iterator.go

@@ -139,9 +139,8 @@ func NewExternalIterWithContext(
 		// Add the readers to the Iterator so that Close closes them, and
 		// SetOptions can re-construct iterators from them.
 		externalReaders: readers,
-		newIters: func(
-			ctx context.Context, f *manifest.FileMetadata, opts *IterOptions,
-			internalOpts internalIterOpts) (internalIterator, keyspan.FragmentIterator, error) {
+		newIters: func(context.Context, *manifest.FileMetadata, *IterOptions,
+			internalIterOpts, iterKinds) (iterSet, error) {
 			// NB: External iterators are currently constructed without any
 			// `levelIters`. newIters should never be called. When we support
 			// organizing multiple non-overlapping files into a single level

flushable.go

@@ -219,7 +219,7 @@ func (s *ingestedFlushable) constructRangeDelIter(
 ) (keyspan.FragmentIterator, error) {
 	// Note that the keyspan level iter expects a non-nil iterator to be
 	// returned even if there is an error. So, we return the emptyKeyspanIter.
-	iter, rangeDelIter, err := s.newIters(context.Background(), file, nil, internalIterOpts{})
+	iter, rangeDelIter, err := s.newIters.TODO(context.Background(), file, nil, internalIterOpts{})
 	if err != nil {
 		return emptyKeyspanIter, err
 	}

get_iter_test.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble/internal/base"
-	"github.com/cockroachdb/pebble/internal/keyspan"
 	"github.com/cockroachdb/pebble/internal/manifest"
 	"github.com/cockroachdb/pebble/internal/testkeys"
 )
@@ -387,13 +386,13 @@ func TestGetIter(t *testing.T) {
 		// m is a map from file numbers to DBs.
 		m := map[FileNum]*memTable{}
 		newIter := func(
-			_ context.Context, file *manifest.FileMetadata, _ *IterOptions, _ internalIterOpts,
-		) (internalIterator, keyspan.FragmentIterator, error) {
+			_ context.Context, file *manifest.FileMetadata, _ *IterOptions, _ internalIterOpts, _ iterKinds,
+		) (iterSet, error) {
 			d, ok := m[file.FileNum]
 			if !ok {
-				return nil, nil, errors.New("no such file")
+				return iterSet{}, errors.New("no such file")
 			}
-			return d.newIter(nil), nil, nil
+			return iterSet{point: d.newIter(nil)}, nil
 		}
 
 		var files [numLevels][]*fileMetadata

ingest.go

@@ -1663,7 +1663,7 @@ func (d *DB) excise(
 			// This file will contain point keys
 			smallestPointKey := m.SmallestPointKey
 			var err error
-			iter, rangeDelIter, err = d.newIters(context.TODO(), m, &IterOptions{
+			iter, rangeDelIter, err = d.newIters.TODO(context.TODO(), m, &IterOptions{
 				CategoryAndQoS: sstable.CategoryAndQoS{
 					Category: "pebble-ingest",
 					QoSLevel: sstable.LatencySensitiveQoSLevel,
@@ -1781,7 +1781,7 @@ func (d *DB) excise(
 		largestPointKey := m.LargestPointKey
 		var err error
 		if iter == nil && rangeDelIter == nil {
-			iter, rangeDelIter, err = d.newIters(context.TODO(), m, &IterOptions{
+			iter, rangeDelIter, err = d.newIters.TODO(context.TODO(), m, &IterOptions{
 				CategoryAndQoS: sstable.CategoryAndQoS{
 					Category: "pebble-ingest",
 					QoSLevel: sstable.LatencySensitiveQoSLevel,

iterator_test.go

@@ -23,7 +23,6 @@ import (
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/internal/bytealloc"
 	"github.com/cockroachdb/pebble/internal/invalidating"
-	"github.com/cockroachdb/pebble/internal/keyspan"
 	"github.com/cockroachdb/pebble/internal/manifest"
 	"github.com/cockroachdb/pebble/internal/testkeys"
 	"github.com/cockroachdb/pebble/objstorage/objstorageprovider"
@@ -919,14 +918,14 @@ func TestIteratorSeekOpt(t *testing.T) {
 			oldNewIters := d.newIters
 			d.newIters = func(
 				ctx context.Context, file *manifest.FileMetadata, opts *IterOptions,
-				internalOpts internalIterOpts) (internalIterator, keyspan.FragmentIterator, error) {
-				iter, rangeIter, err := oldNewIters(ctx, file, opts, internalOpts)
-				iterWrapped := &iterSeekOptWrapper{
-					internalIterator:      iter,
+				internalOpts internalIterOpts, kinds iterKinds) (iterSet, error) {
+				iters, err := oldNewIters(ctx, file, opts, internalOpts, kinds)
+				iters.point = &iterSeekOptWrapper{
+					internalIterator:      iters.point,
 					seekGEUsingNext:       &seekGEUsingNext,
 					seekPrefixGEUsingNext: &seekPrefixGEUsingNext,
 				}
-				return iterWrapped, rangeIter, err
+				return iters, err
 			}
 			return s
 

level_checker.go

@@ -385,7 +385,7 @@ func checkRangeTombstones(c *checkConfig) error {
 		for f := files.First(); f != nil; f = files.Next() {
 			lf := files.Take()
 			//lower, upper := manifest.KeyRange(c.cmp, lf.Iter())
-			iterToClose, iter, err := c.newIters(
+			iterToClose, iter, err := c.newIters.TODO(
 				context.Background(), lf.FileMetadata, &IterOptions{level: manifest.Level(lsmLevel)}, internalIterOpts{})
 			if err != nil {
 				return err

level_checker_test.go

@@ -95,17 +95,20 @@ func TestCheckLevelsCornerCases(t *testing.T) {
 
 	var fileNum FileNum
 	newIters :=
-		func(_ context.Context, file *manifest.FileMetadata, _ *IterOptions, _ internalIterOpts) (internalIterator, keyspan.FragmentIterator, error) {
+		func(_ context.Context, file *manifest.FileMetadata, _ *IterOptions, _ internalIterOpts, _ iterKinds) (iterSet, error) {
 			r := readers[file.FileNum]
 			rangeDelIter, err := r.NewRawRangeDelIter()
 			if err != nil {
-				return nil, nil, err
+				return iterSet{}, err
 			}
 			iter, err := r.NewIter(nil /* lower */, nil /* upper */)
 			if err != nil {
-				return nil, nil, err
+				return iterSet{}, err
 			}
-			return iter, rangeDelIter, nil
+			return iterSet{
+				point:         iter,
+				rangeDeletion: rangeDelIter,
+			}, nil
 		}
 
 	fm := &failMerger{}

level_iter.go

@@ -16,35 +16,76 @@ import (
 	"github.com/cockroachdb/pebble/sstable"
 )
 
-// tableNewIters creates a new point and range-del iterator for the given file
-// number.
+// tableNewIters creates new iterators (point, range deletion and/or range key)
+// for the given file metadata. Which of the various iterator kinds the user is
+// requesting is specified with the iterKinds bitmap.
 //
-// On success, the internalIterator is not-nil and must be closed; the
-// FragmentIterator can be nil.
-// TODO(radu): always return a non-nil FragmentIterator.
+// On success, the requested subset of iters.{point,rangeDel,rangeKey} are
+// populated with iterators.
 //
-// On error, the iterators are nil.
+// If a point iterator is requested and the operation was successful,
+// iters.point is guaranteed to be non-nil and must be closed when the caller is
+// finished.
 //
-// The only (non-test) implementation of tableNewIters is tableCacheContainer.newIters().
+// If a range deletion or range key iterator is requested, the corresponding
+// iterator may be nil if the table does not contain any keys of the
+// corresponding kind. The returned iterSet type provides RangeDeletion() and
+// RangeKey() convenience methods that return non-nil empty iterators that may
+// be used if the caller requires a non-nil iterator.
+//
+// On error, all iterators are nil.
+//
+// The only (non-test) implementation of tableNewIters is
+// tableCacheContainer.newIters().
 type tableNewIters func(
 	ctx context.Context,
 	file *manifest.FileMetadata,
 	opts *IterOptions,
 	internalOpts internalIterOpts,
-) (internalIterator, keyspan.FragmentIterator, error)
+	kinds iterKinds,
+) (iterSet, error)
+
+// all returns an iterator set containing iterators over all the keys that meta
+// contains.
+func (f tableNewIters) all(
+	ctx context.Context, file *fileMetadata, opts *IterOptions, internalOpts internalIterOpts,
+) (iterSet, error) {
+	var kinds iterKinds
+	if file.HasPointKeys {
+		kinds |= iterPointKeys
+		kinds |= iterRangeDeletions
+	}
+	if file.HasRangeKeys {
+		kinds |= iterRangeKeys
+	}
+	return f(ctx, file, opts, internalOpts, kinds)
+}
+
+// TODO implements the old tableNewIters interface that always attempted to open
+// a point iterator and a range deletion iterator. All call sites should be
+// updated to use `f` directly.
+func (f tableNewIters) TODO(
+	ctx context.Context,
+	file *manifest.FileMetadata,
+	opts *IterOptions,
+	internalOpts internalIterOpts,
+) (internalIterator, keyspan.FragmentIterator, error) {
+	iters, err := f(ctx, file, opts, internalOpts, iterPointKeys|iterRangeDeletions)
+	if err != nil {
+		return nil, nil, err
+	}
+	return iters.point, iters.rangeDeletion, nil
+}
 
 // tableNewRangeDelIter takes a tableNewIters and returns a TableNewSpanIter
 // for the rangedel iterator returned by tableNewIters.
 func tableNewRangeDelIter(ctx context.Context, newIters tableNewIters) keyspan.TableNewSpanIter {
 	return func(file *manifest.FileMetadata, iterOptions keyspan.SpanIterOptions) (keyspan.FragmentIterator, error) {
-		iter, rangeDelIter, err := newIters(ctx, file, nil, internalIterOpts{})
-		if iter != nil {
-			_ = iter.Close()
-		}
-		if rangeDelIter == nil {
-			rangeDelIter = emptyKeyspanIter
+		iters, err := newIters(ctx, file, nil, internalIterOpts{}, iterRangeDeletions)
+		if err != nil {
+			return nil, err
 		}
-		return rangeDelIter, err
+		return iters.RangeDeletion(), nil
 	}
 }
 
@@ -676,7 +717,7 @@ func (l *levelIter) loadFile(file *fileMetadata, dir int) loadFileReturnIndicato
 
 		var rangeDelIter keyspan.FragmentIterator
 		var iter internalIterator
-		iter, rangeDelIter, l.err = l.newIters(l.ctx, l.iterFile, &l.tableOpts, l.internalOpts)
+		iter, rangeDelIter, l.err = l.newIters.TODO(l.ctx, l.iterFile, &l.tableOpts, l.internalOpts)
 		l.iter = iter
 		if l.err != nil {
 			return noFileLoaded