db: fix ingested flushable range deletion iterator

When an ingested flushable creates a range deletion iterator, it uses the
keyspanimpl.LevelIter to expose a unified view of all the range deletions
across the files of the atomically ingested sstables. When loading a sstable's
range deletion iterator failed, it returned a non-nil iterator to work around
idiosyncroncies of the keyspanimpl.LevelIter. This could cause the LevelIter to
mistakenly think it had already loaded a file's range deletions. This commit
fixes ingestedFlushable.constructRangeDelIter to return a nil FragmentIterator
if there's an error, and refactors (keyspanimpl.LevelIter).loadFile.

Fix #3592.

flushable.go

@@ -237,10 +237,10 @@ func (s *ingestedFlushable) constructRangeDelIter(
  file *manifest.FileMetadata, _ keyspan.SpanIterOptions,
 ) (keyspan.FragmentIterator, error) {
  iters, err := s.newIters(context.Background(), file, nil, internalIterOpts{}, iterRangeDeletions)
- // Note that the keyspan level iter expects a non-nil iterator to be
- // returned even if there is an error. So, we return iters.RangeDeletion()
- // regardless of the value of err.
- return iters.RangeDeletion(), err
+ if err != nil {
+  return nil, err
+ }
+ return iters.RangeDeletion(), nil
 }
 
 // newRangeDelIter is part of the flushable interface.

internal/keyspan/keyspanimpl/level_iter.go

@@ -132,44 +132,40 @@ const (
 )
 
 func (l *LevelIter) loadFile(file *manifest.FileMetadata, dir int) loadFileReturnIndicator {
- indicator := noFileLoaded
  if l.iterFile == file {
  if l.err != nil {
  return noFileLoaded
  }
  if l.iter != nil {
- // We are already at the file, but we would need to check for bounds.
- // Set indicator accordingly.
- indicator = fileAlreadyLoaded
+ // We are already at the file.
+ return fileAlreadyLoaded
  }
  // We were already at file, but don't have an iterator, probably because the file was
  // beyond the iteration bounds. It may still be, but it is also possible that the bounds
  // have changed. We handle that below.
  }
 
  // Note that LevelIter.Close() can be called multiple times.
- if indicator != fileAlreadyLoaded {
- if err := l.Close(); err != nil {
- return noFileLoaded
- }
+ if err := l.Close(); err != nil {
+ return noFileLoaded
  }
 
  l.iterFile = file
+ l.iter = nil
  if file == nil {
  return noFileLoaded
  }
- if indicator != fileAlreadyLoaded {
- l.iter, l.err = l.newIter(file, l.tableOpts)
- if l.wrapFn != nil {
- l.iter = l.wrapFn(l.iter)
- }
- l.iter = keyspan.MaybeAssert(l.iter, l.cmp)
- indicator = newFileLoaded
- }
- if l.err != nil {
+ iter, err := l.newIter(file, l.tableOpts)
+ if err != nil {
+ l.err = err
  return noFileLoaded
  }
- return indicator
+ l.iter = iter
+ if l.wrapFn != nil {
+ l.iter = l.wrapFn(l.iter)
+ }
+ l.iter = keyspan.MaybeAssert(l.iter, l.cmp)
+ return newFileLoaded
 }
 
 // SeekGE implements keyspan.FragmentIterator.