From 3a0bea13be8f3ebc5dd4fdbddd8fd38d02b39317 Mon Sep 17 00:00:00 2001
From: Bilal Akhtar <bilal@cockroachlabs.com>
Date: Mon, 22 Jul 2024 16:24:24 -0400
Subject: [PATCH] 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.
---
 sstable/reader_iter_single_lvl.go          |  6 +++++
 sstable/reader_test.go                     | 18 +++++++++++++++
 sstable/testdata/reader_hide_obsolete/iter | 26 ++++++++++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/sstable/reader_iter_single_lvl.go b/sstable/reader_iter_single_lvl.go
index b32293c6e3..04b779af2e 100644
--- a/sstable/reader_iter_single_lvl.go
+++ b/sstable/reader_iter_single_lvl.go
@@ -1447,6 +1447,12 @@ func (i *singleLevelIterator) skipBackward() *base.InternalKV {
 		}
 		kv := i.data.Last()
 		if kv == nil {
+			if i.transforms.HideObsoletePoints {
+				// 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.
+				continue
+			}
 			return nil
 		}
 		if i.blockLower != nil && i.cmp(kv.K.UserKey, i.blockLower) < 0 {
diff --git a/sstable/reader_test.go b/sstable/reader_test.go
index e529ada778..29b278df58 100644
--- a/sstable/reader_test.go
+++ b/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)
diff --git a/sstable/testdata/reader_hide_obsolete/iter b/sstable/testdata/reader_hide_obsolete/iter
index 5f6eab7967..1aba92f326 100644
--- a/sstable/testdata/reader_hide_obsolete/iter
+++ b/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