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 cockroachdb#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.