sstable: don't skipBackward past empty blocks with hideObsolete

Currently, if we skipBackward in singleLevelIterator and come
across a seemingly empty data block, we stop right there. This
is not safe if hide obsolete points is true, as the block could
have just been obsolete in its entirety. This change makes the
iterator continue iterating backward until some other termination
condition is exhausted.

This matches the behaviour for two-level iterators, as well as
for skipForward in single-level iterators.

Fixes #3761.

Author: itsbilal

sstable/reader_iter_single_lvl.go

@@ -1447,7 +1447,15 @@ func (i *singleLevelIterator) skipBackward() *base.InternalKV {
 		}
 		kv := i.data.Last()
 		if kv == nil {
-			return nil
+			// The block iter could have hid some obsolete points, so it isn't
+			// safe to assume that there are no keys if we keep skipping backwards.
+			// Check the previous block, but check the lower bound before doing
+			// that.
+			if i.lower != nil && i.cmp(indexKey.K.UserKey, i.lower) < 0 {
+				i.exhaustedBounds = -1
+				return nil
+			}
+			continue
 		}
 		if i.blockLower != nil && i.cmp(kv.K.UserKey, i.blockLower) < 0 {
 			i.exhaustedBounds = -1

sstable/reader_test.go

@@ -545,6 +545,17 @@ func TestReaderHideObsolete(t *testing.T) {
 				t, opts, "testdata/reader_hide_obsolete",
 				nil /* Reader */, true)
 		})
+		opts = WriterOptions{
+			TableFormat:    TableFormatPebblev4,
+			BlockSize:      blockSize,
+			IndexBlockSize: 1 << 30, // Force a single-level index block.
+			Comparer:       testkeys.Comparer,
+		}
+		t.Run(fmt.Sprintf("singleLevel/blockSize=%s", dName), func(t *testing.T) {
+			runTestReader(
+				t, opts, "testdata/reader_hide_obsolete",
+				nil /* Reader */, true)
+		})
 	}
 }
 
@@ -797,6 +808,13 @@ func runTestReader(t *testing.T, o WriterOptions, dir string, r *Reader, printVa
 						require.True(t, retHideObsoletePoints)
 					}
 				}
+				if d.HasArg("hide-obsolete-points-without-filter") {
+					var hideObsoletePoints bool
+					d.ScanArgs(t, "hide-obsolete-points-without-filter", &hideObsoletePoints)
+					if hideObsoletePoints {
+						transforms.HideObsoletePoints = true
+					}
+				}
 				var filterer *BlockPropertiesFilterer
 				if len(bpfs) > 0 {
 					filterer = newBlockPropertiesFilterer(bpfs, nil, nil)

sstable/testdata/reader_hide_obsolete/iter

@@ -316,3 +316,29 @@ d.SINGLEDEL.40:
 d.MERGE.30:D30
 ----
 MERGE not supported in a strict-obsolete sstable
+
+# Regression test for #3761. If an entire block contains obsolete points,
+# skipBackward should still skip to blocks earlier in the sstable.
+
+build writing-to-lowest-level
+a.SET.10:A10
+force-obsolete: b.DEL.5:
+force-obsolete: b.SETWITHDEL.2:foo
+force-obsolete: c.DEL.0:
+force-obsolete: cc.DEL.5:
+----
+
+iter hide-obsolete-points-without-filter=true
+last
+----
+<a:10>:A10
+
+iter hide-obsolete-points-without-filter=true
+seek-lt d
+----
+<a:10>:A10
+
+iter hide-obsolete-points-without-filter=true
+seek-lt c
+----
+<a:10>:A10