From 98cadc9f9bcdb0150bd4f5a6072dd6c2504e63ad Mon Sep 17 00:00:00 2001
From: Jackson Owens <jackson@cockroachlabs.com>
Date: Tue, 7 May 2024 13:40:32 -0400
Subject: [PATCH] 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                               |  8 +++---
 internal/keyspan/keyspanimpl/level_iter.go | 32 ++++++++++------------
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/flushable.go b/flushable.go
index ce3b74fbb8..a2ed68c4fb 100644
--- a/flushable.go
+++ b/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.
diff --git a/internal/keyspan/keyspanimpl/level_iter.go b/internal/keyspan/keyspanimpl/level_iter.go
index 352beaeff1..b45c43406a 100644
--- a/internal/keyspan/keyspanimpl/level_iter.go
+++ b/internal/keyspan/keyspanimpl/level_iter.go
@@ -132,15 +132,13 @@ 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
@@ -148,28 +146,26 @@ func (l *LevelIter) loadFile(file *manifest.FileMetadata, dir int) loadFileRetur
 	}
 
 	// 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.