Repository: cockroachdb/pebble (go)

Issue:
sstable: missing keys when setting obsolete bits
```

		===== SEED =====
		1688175492068207000
		===== DIFF =====
		_meta/230630-213812.0682724060275/{standard-000,standard-026}
		@@ -237,11 +237,11 @@
 db.Merge("sxvlqq@1", "sle") // <nil> #236
 iter2.Prev("") // [false] pebble: unsupported reverse prefix iteration #237
 iter2.Next("bcvxkinko@2") // [invalid] cannot use limit with prefix iteration #238
 iter2.SeekLT("kbdzmck", "") // [true,"ipptgioar@2","nysdtymiefduy",["bcvxkinko","kbdzmck")=>{"@1"=""}*] <nil> #239
 db.SingleDelete("dijofczaiws@4", true /* maybeReplaceDelete */) // <nil> #240
-iter2.SeekGE("dzkhkzwn@2", "") // [true,"dzkhkzwn@2","omwnllrnaaekxcatmlw",["bcvxkinko","kbdzmck")=>{"@1"=""}] <nil> #241
+iter2.SeekGE("dzkhkzwn@2", "") // [true,"dzkhkzwn@2","",["bcvxkinko","kbdzmck")=>{"@1"=""}] <nil> #241
 iter2.NextPrefix() // [true,"hnsrnnu@1","vcmshucrmtu",["bcvxkinko","kbdzmck")=>{"@1"=""}] <nil> #242
 iter2.SeekPrefixGE("kbdzmck@3") // [true,"kbdzmck@3","xmhwzwzlize",<no range>*] <nil> #243
 iter2.Last() // [true,"wwzibulql@3","bqesltimayqruwsbzr",<no range>] <nil> #244
 db.DeleteRange("ifbca@1", "xujqxudpzwy@2") // <nil> #245
 iter2.First() // [true,"aognljopzy@3","qodk",<no range>] <nil> #246
@@ -257,11 +257,11 @@
 db.Delete("ifbca@1") // <nil> #256
 iter2.Next("") // [true,"awiaql@2","efsdxoodwkl",["awiaql","bcvxkinko")=>{""="v","@1"=""}] <nil> #257
 db.Get("awiaql@1") // [""] pebble: not found #258
 iter2.SeekLT("kbdzmck@1", "") // [true,"kbdzmck@3","xmhwzwzlize",<no range>*] <nil> #259
 db.Merge("spmxspivm@4", "nklocdzvc") // <nil> #260
-iter2.SeekPrefixGE("dzkhkzwn@2") // [true,"dzkhkzwn@2","omwnllrnaaekxcatmlw",["dzkhkzwn","dzkhkzwn\x00")=>{"@1"=""}*] <nil> #261
+iter2.SeekPrefixGE("dzkhkzwn@2") // [true,"dzkhkzwn@2","",["dzkhkzwn","dzkhkzwn\x00")=>{"@1"=""}*] <nil> #261
 iter2.SetBounds("", "kbdzmck") // <nil> #262
 iter2.SeekLT("kbdzmck", "") // [true,"ipptgioar@2","nysdtymiefduy",["bcvxkinko","kbdzmck")=>{"@1"=""}*] <nil> #263
 iter2.Next("") // [false] <nil> #264
 iter2.Prev("") // [true,"ipptgioar@2","nysdtymiefduy",["bcvxkinko","kbdzmck")=>{"@1"=""}*] <nil> #265
 iter2.SetBounds("spmxspivm@4", "sxvlqq@1") // <nil> #266
```

Pull request:
db,sstable: bug fixes for keys with obsolete bit
- MERGE followed by DEL* previously caused the DEL* to be marked obsolete. This is incorrect since there may be a SET*/MERGE is a lower level that ought to be deleted, but will now get merged.

- A pointer to IterOptions was passed to tableCacheContainer.newIters, and IterOptions.PointKeyFilters was modified by the callee to append the obsolete key block property filter. Since the levelIter reuses the same IterOptions (levelIter.tableOpts), the obsolete key block property filter was getting used even for a file whose seqnum does not permit it. tableCacheShard.newIters is now changed to reuse the memory of the PointKeyFilters slice without altering the original slice.

- getIter was previously not setting the levelIterBoundaryContext, which prevented it from realizing whether a key returned should be ignored. This was not previously a bug since (a) there are no iterator bounds, so isSyntheticIterBoundsKey is never true (this is still true), (b) isIgnorableBoundaryKey was not true for some unclear reasons. (b) is now true when we exhaust the iterator by skipping blocks due to block property filters. This bug is fixed by setting and using levelIterBoundaryContext in getIter.

- compactionIter.snapshotPinned is no longer used to set forceObsolete, since it has a false positive for MERGE followed by older SET, when the latter is preserved due to a snapshot. Instead, we introduce forceObsoleteDueToRangeDel, which only handles obsolescence due to RANGEDEL.

Fixes #2705

Patch:
diff --git a/compaction.go b/compaction.go
index c1c7ed63..ff4d2778 100644
--- a/compaction.go
+++ b/compaction.go
@@ -3271,10 +3271,7 @@ func (d *DB) runCompaction(
 					return nil, pendingOutputs, stats, err
 				}
 			}
-			// iter.snapshotPinned is broader than whether the point was covered by
-			// a RANGEDEL, but it is harmless to pass true when the callee will also
-			// independently discover that the point is obsolete.
-			if err := tw.AddWithForceObsolete(*key, val, iter.snapshotPinned); err != nil {
+			if err := tw.AddWithForceObsolete(*key, val, iter.forceObsoleteDueToRangeDel); err != nil {
 				return nil, pendingOutputs, stats, err
 			}
 			if iter.snapshotPinned {
diff --git a/compaction_iter.go b/compaction_iter.go
index 5bcfa95a..31f68497 100644
--- a/compaction_iter.go
+++ b/compaction_iter.go
@@ -207,14 +207,6 @@ type compactionIter struct {
 	// its elision. This field only applies to point keys, and not to range
 	// deletions or range keys.
 	//
-	// snapshotPinned is also used to set the forceObsolete value in the call to
-	// Writer.AddWithForceObsolete. Note that in that call, it is sufficient to
-	// mark all keys obsoleted by RANGEDELs as forceObsolete=true and that the
-	// implementation of Writer.AddWithForceObsolete will itself discover other
-	// causes of obsolescence. We mention this since in the presence of MERGE,
-	// obsolescence due to multiple keys at the same user key is not fully
-	// represented by snapshotPinned=true:
-	//
 	// For MERGE, it is possible that doing the merge is interrupted even when
 	// the next point key is in the same stripe. This can happen if the loop in
 	// mergeNext gets interrupted by sameStripeNonSkippable.
@@ -222,6 +214,21 @@ type compactionIter struct {
 	// SET/MERGE/DEL with the same seqnum, so the RANGEDEL does not necessarily
 	// delete the subsequent SET/MERGE/DEL keys.
 	snapshotPinned bool
+	// forceObsoleteDueToRangeDel is set to true in a subset of the cases that
+	// snapshotPinned is true. This value is true when the point is obsolete due
+	// to a RANGEDEL but could not be deleted due to a snapshot.
+	//
+	// NB: it may seem that the additional cases that snapshotPinned captures
+	// are harmless in that they can also be used to mark a point as obsolete
+	// (it is merely a duplication of some logic that happens in
+	// Writer.AddWithForceObsolete), but that is not quite accurate as of this
+	// writing -- snapshotPinned originated in stats collection and for a
+	// sequence MERGE, SET, where the MERGE cannot merge with the (older) SET
+	// due to a snapshot, the snapshotPinned value for the SET is true.
+	//
+	// TODO(sumeer,jackson): improve the logic of snapshotPinned and reconsider
+	// whether we need forceObsoleteDueToRangeDel.
+	forceObsoleteDueToRangeDel bool
 	// The index of the snapshot for the current key within the snapshots slice.
 	curSnapshotIdx    int
 	curSnapshotSeqNum uint64
@@ -410,6 +417,9 @@ func (i *compactionIter) Next() (*InternalKey, []byte) {
 			// key is in the same snapshot stripe. Hence, snapshotPinned is by
 			// definition false in those cases.
 			i.snapshotPinned = true
+			i.forceObsoleteDueToRangeDel = true
+		} else {
+			i.forceObsoleteDueToRangeDel = false
 		}
 
 		switch i.iterKey.Kind() {
diff --git a/get_iter.go b/get_iter.go
index 0ac0484e..17d7de31 100644
--- a/get_iter.go
+++ b/get_iter.go
@@ -162,8 +162,14 @@ func (g *getIter) Next() (*InternalKey, base.LazyValue) {
 				g.levelIter.init(context.Background(), iterOpts, g.cmp, nil /* split */, g.newIters,
 					files, manifest.L0Sublevel(n), internalIterOpts{})
 				g.levelIter.initRangeDel(&g.rangeDelIter)
+				bc := levelIterBoundaryContext{}
+				g.levelIter.initBoundaryContext(&bc)
 				g.iter = &g.levelIter
 				g.iterKey, g.iterValue = g.iter.SeekGE(g.key, base.SeekGEFlagsNone)
+				if bc.isSyntheticIterBoundsKey || bc.isIgnorableBoundaryKey {
+					g.iterKey = nil
+					g.iterValue = base.LazyValue{}
+				}
 				continue
 			}
 			g.level++
@@ -181,9 +187,15 @@ func (g *getIter) Next() (*InternalKey, base.LazyValue) {
 		g.levelIter.init(context.Background(), iterOpts, g.cmp, nil /* split */, g.newIters,
 			g.version.Levels[g.level].Iter(), manifest.Level(g.level), internalIterOpts{})
 		g.levelIter.initRangeDel(&g.rangeDelIter)
+		bc := levelIterBoundaryContext{}
+		g.levelIter.initBoundaryContext(&bc)
 		g.level++
 		g.iter = &g.levelIter
 		g.iterKey, g.iterValue = g.iter.SeekGE(g.key, base.SeekGEFlagsNone)
+		if bc.isSyntheticIterBoundsKey || bc.isIgnorableBoundaryKey {
+			g.iterKey = nil
+			g.iterValue = base.LazyValue{}
+		}
 	}
 }
 
diff --git a/ingest.go b/ingest.go
index cbcf1594..183216e6 100644
--- a/ingest.go
+++ b/ingest.go
@@ -1732,8 +1732,10 @@ func (d *DB) ingestApply(
 		// for files, and if they are, we should signal those compactions to error
 		// out.
 		for level := range current.Levels {
-			iter := current.Levels[level].Iter()
-			for m := iter.SeekGE(d.cmp, exciseSpan.Start); m != nil && d.cmp(m.Smallest.UserKey, exciseSpan.End) < 0; m = iter.Next() {
+			overlaps := current.Overlaps(level, d.cmp, exciseSpan.Start, exciseSpan.End, true /* exclusiveEnd */)
+			iter := overlaps.Iter()
+
+			for m := iter.First(); m != nil; m = iter.Next() {
 				excised, err := d.excise(exciseSpan, m, ve, level)
 				if err != nil {
 					return nil, err
diff --git a/objstorage/objstorageprovider/provider.go b/objstorage/objstorageprovider/provider.go
index da753c50..6de07d69 100644
--- a/objstorage/objstorageprovider/provider.go
+++ b/objstorage/objstorageprovider/provider.go
@@ -184,9 +184,9 @@ func open(settings Settings) (p *provider, _ error) {
 
 // Close is part of the objstorage.Provider interface.
 func (p *provider) Close() error {
-	var err error
+	err := p.sharedClose()
 	if p.fsDir != nil {
-		err = p.fsDir.Close()
+		err = firstError(err, p.fsDir.Close())
 		p.fsDir = nil
 	}
 	if objiotracing.Enabled {
diff --git a/objstorage/objstorageprovider/shared.go b/objstorage/objstorageprovider/shared.go
index 19f993b2..74b929b3 100644
--- a/objstorage/objstorageprovider/shared.go
+++ b/objstorage/objstorageprovider/shared.go
@@ -111,6 +111,22 @@ func (p *provider) sharedInit() error {
 	return nil
 }
 
+func (p *provider) sharedClose() error {
+	if p.st.Shared.StorageFactory == nil {
+		return nil
+	}
+	var err error
+	if p.shared.cache != nil {
+		err = p.shared.cache.Close()
+		p.shared.cache = nil
+	}
+	if p.shared.catalog != nil {
+		err = firstError(err, p.shared.catalog.Close())
+		p.shared.catalog = nil
+	}
+	return err
+}
+
 // SetCreatorID is part of the objstorage.Provider interface.
 func (p *provider) SetCreatorID(creatorID objstorage.CreatorID) error {
 	if p.st.Shared.StorageFactory == nil {
diff --git a/sstable/format.go b/sstable/format.go
index a1d9be01..a42ed7c3 100644
--- a/sstable/format.go
+++ b/sstable/format.go
@@ -91,7 +91,8 @@ const (
 // obsolete, i.e., obsolete within the sstable, and we can guarantee that at
 // most one point will be exposed per user key. MERGE keys create more
 // complexity: MERGE followed by MERGE causes multiple keys to not be
-// obsolete. Same applies for MERGE followed by SET/SETWITHDEL. Note that:
+// obsolete. Same applies for MERGE followed by SET/SETWITHDEL/DEL*. Note
+// that:
 //
 // - For regular sst iteration, the obsolete marking is a performance
 //   optimization, and multiple keys for the same userkey can be handled by
diff --git a/sstable/writer.go b/sstable/writer.go
index a499d2a0..883ee7ce 100644
--- a/sstable/writer.go
+++ b/sstable/writer.go
@@ -852,32 +852,35 @@ func (w *Writer) makeAddPointDecisionV3(
 	} else {
 		cmpUser = w.compare(prevPointUserKey, key.UserKey)
 	}
-	keyKindIsSetOrMerge := keyKind == InternalKeyKindSet || keyKind == InternalKeyKindSetWithDelete ||
-		keyKind == InternalKeyKindMerge
-	keyKindIsPointDelete := keyKind == InternalKeyKindDelete ||
-		keyKind == InternalKeyKindSingleDelete || keyKind == InternalKeyKindDeleteSized
-	if !keyKindIsSetOrMerge && !keyKindIsPointDelete {
+	// Ensure that no one adds a point key kind without considering the obsolete
+	// handling for that kind.
+	switch keyKind {
+	case InternalKeyKindSet, InternalKeyKindSetWithDelete, InternalKeyKindMerge,
+		InternalKeyKindDelete, InternalKeyKindSingleDelete, InternalKeyKindDeleteSized:
+	default:
 		panic(errors.AssertionFailedf("unexpected key kind %s", keyKind.String()))
 	}
 	// If same user key, then the current key is obsolete if any of the
 	// following is true:
 	// C1 The prev key was obsolete.
-	// C2 The prev key was not a MERGE
-	// C3 The current key is not a SET or SETWITHDELETE or MERGE, then it is
-	//    obsolete. This rule excludes SET, SETWITHDELETE, MERGE, since they
-	//    will be merged with the previous key.
+	// C2 The prev key was not a MERGE. When the previous key is a MERGE we must
+	//    preserve SET* and MERGE since their values will be merged into the
+	//    previous key. We also must preserve DEL* since there may be an older
+	//    SET*/MERGE in a lower level that must not be merged with the MERGE --
+	//    if we omit the DEL* that lower SET*/MERGE will become visible.
 	//
 	// Regardless of whether it is the same user key or not
-	// C4 The current key is some kind of point delete, and we are writing to
-	//    the lowest level, then it is also obsolete.
+	// C3 The current key is some kind of point delete, and we are writing to
+	//    the lowest level, then it is also obsolete. The correctness of this
+	//    relies on the same user key not spanning multiple sstables in a level.
 	//
 	// C1 ensures that for a user key there is at most one transition from
 	// !obsolete to obsolete. Consider a user key k, for which the first n keys
 	// are not obsolete. We consider the various value of n:
 	//
 	// n = 0: This happens due to forceObsolete being set by the caller, or due
-	// to C4. forceObsolete must only be set due a RANGEDEL, and that RANGEDEL
-	// must also delete all the lower seqnums for the same user key. C4 triggers
+	// to C3. forceObsolete must only be set due a RANGEDEL, and that RANGEDEL
+	// must also delete all the lower seqnums for the same user key. C3 triggers
 	// due to a point delete and that deletes all the lower seqnums for the same
 	// user key.
 	//
@@ -885,12 +888,13 @@ func (w *Writer) makeAddPointDecisionV3(
 	// MERGE, or the current key is some kind of point delete.
 	//
 	// n > 1: This is due to a sequence of MERGE keys, potentially followed by a
-	// single SET or SETWITHDELETE.
-	isObsolete = (cmpUser == 0 &&
-		(prevKeyKind != InternalKeyKindMerge || prevPointKeyInfo.isObsolete || !keyKindIsSetOrMerge)) ||
-		(w.writingToLowestLevel &&
-			(keyKind == InternalKeyKindDelete || keyKind == InternalKeyKindSingleDelete ||
-				keyKind == InternalKeyKindDeleteSized))
+	// single non-MERGE key.
+	isObsoleteC1AndC2 := cmpUser == 0 &&
+		(prevPointKeyInfo.isObsolete || prevKeyKind != InternalKeyKindMerge)
+	isObsoleteC3 := w.writingToLowestLevel &&
+		(keyKind == InternalKeyKindDelete || keyKind == InternalKeyKindSingleDelete ||
+			keyKind == InternalKeyKindDeleteSized)
+	isObsolete = isObsoleteC1AndC2 || isObsoleteC3
 	// TODO(sumeer): storing isObsolete SET and SETWITHDEL in value blocks is
 	// possible, but requires some care in documenting and checking invariants.
 	// There is code that assumes nothing in value blocks because of single MVCC
@@ -942,8 +946,7 @@ func (w *Writer) addPoint(key InternalKey, value []byte, forceObsolete bool) err
 	if err != nil {
 		return err
 	}
-	// Temporarily disable `isObsolete`.
-	isObsolete = false && w.tableFormat >= TableFormatPebblev4 && (isObsolete || forceObsolete)
+	isObsolete = w.tableFormat >= TableFormatPebblev4 && (isObsolete || forceObsolete)
 	w.lastPointKeyInfo.isObsolete = isObsolete
 	var valueStoredWithKey []byte
 	var prefix valuePrefix
diff --git a/table_cache.go b/table_cache.go
index 90acb406..67f18337 100644
--- a/table_cache.go
+++ b/table_cache.go
@@ -402,6 +402,7 @@ func (c *tableCacheShard) newIters(
 	}
 
 	hideObsoletePoints := false
+	var pointKeyFilters []BlockPropertyFilter
 	if opts != nil {
 		// This code is appending (at most one filter) in-place to
 		// opts.PointKeyFilters even though the slice is shared for iterators in
@@ -417,7 +418,7 @@ func (c *tableCacheShard) newIters(
 		//
 		// An alternative would be to have different slices for different sstable
 		// iterators, but that requires more work to avoid allocations.
-		hideObsoletePoints, opts.PointKeyFilters =
+		hideObsoletePoints, pointKeyFilters =
 			v.reader.TryAddBlockPropertyFilterForHideObsoletePoints(
 				opts.snapshotForHideObsoletePoints, file.LargestSeqNum, opts.PointKeyFilters)
 	}
@@ -426,7 +427,7 @@ func (c *tableCacheShard) newIters(
 	var err error
 	if opts != nil {
 		ok, filterer, err = c.checkAndIntersectFilters(v, opts.TableFilter,
-			opts.PointKeyFilters, internalOpts.boundLimitedFilter)
+			pointKeyFilters, internalOpts.boundLimitedFilter)
 	}
 	if err != nil {
 		c.unrefValue(v)


Test patch:
diff --git a/compaction_iter_test.go b/compaction_iter_test.go
index 603f6e8d..131859d5 100644
--- a/compaction_iter_test.go
+++ b/compaction_iter_test.go
@@ -181,6 +181,7 @@ func TestCompactionIter(t *testing.T) {
 				allowZeroSeqnum = false
 				printSnapshotPinned := false
 				printMissizedDels := false
+				printForceObsolete := false
 				for _, arg := range d.CmdArgs {
 					switch arg.Key {
 					case "snapshots":
@@ -207,6 +208,8 @@ func TestCompactionIter(t *testing.T) {
 						printSnapshotPinned = true
 					case "print-missized-dels":
 						printMissizedDels = true
+					case "print-force-obsolete":
+						printForceObsolete = true
 					default:
 						return fmt.Sprintf("%s: unknown arg: %s", d.Cmd, arg.Key)
 					}
@@ -260,6 +263,13 @@ func TestCompactionIter(t *testing.T) {
 								snapshotPinned = " (pinned)"
 							}
 						}
+						forceObsolete := ""
+						if printForceObsolete {
+							forceObsolete = " (not force obsolete)"
+							if iter.forceObsoleteDueToRangeDel {
+								forceObsolete = " (force obsolete)"
+							}
+						}
 						v := string(iter.Value())
 						if iter.Key().Kind() == base.InternalKeyKindDeleteSized && len(iter.Value()) > 0 {
 							vn, n := binary.Uvarint(iter.Value())
@@ -269,7 +279,7 @@ func TestCompactionIter(t *testing.T) {
 								v = fmt.Sprintf("varint(%d)", vn)
 							}
 						}
-						fmt.Fprintf(&b, "%s:%s%s\n", iter.Key(), v, snapshotPinned)
+						fmt.Fprintf(&b, "%s:%s%s%s\n", iter.Key(), v, snapshotPinned, forceObsolete)
 						if iter.Key().Kind() == InternalKeyKindRangeDelete {
 							iter.rangeDelFrag.Add(keyspan.Span{
 								Start: append([]byte{}, iter.Key().UserKey...),
diff --git a/objstorage/objstorageprovider/provider_test.go b/objstorage/objstorageprovider/provider_test.go
index 46aa0846..120794c0 100644
--- a/objstorage/objstorageprovider/provider_test.go
+++ b/objstorage/objstorageprovider/provider_test.go
@@ -359,13 +359,13 @@ func TestSharedMultipleLocators(t *testing.T) {
 	checkObject(p1, file2, 1)
 
 	// Check that the object still works after close/reopen.
-	p1.Close()
+	require.NoError(t, p1.Close())
 	p1, err = Open(st1)
 	require.NoError(t, err)
 	checkObject(p1, file2, 1)
-	p1.Close()
+	require.NoError(t, p1.Close())
 
-	p2.Close()
+	require.NoError(t, p2.Close())
 
 	// Try to attach an object to a provider that doesn't recognize the locator.
 	st3 := DefaultSettings(vfs.NewMem(), "")
@@ -373,13 +373,13 @@ func TestSharedMultipleLocators(t *testing.T) {
 	p3, err := Open(st3)
 	require.NoError(t, err)
 	require.NoError(t, p3.SetCreatorID(3))
-	_, err = p1.AttachSharedObjects([]objstorage.SharedObjectToAttach{{
+	_, err = p3.AttachSharedObjects([]objstorage.SharedObjectToAttach{{
 		FileNum:  file2,
 		FileType: base.FileTypeTable,
 		Backing:  b2,
 	}})
 	require.Error(t, err)
-	p3.Close()
+	require.NoError(t, p3.Close())
 }
 
 func TestAttachCustomObject(t *testing.T) {
diff --git a/objstorage/objstorageprovider/testdata/provider/shared_attach b/objstorage/objstorageprovider/testdata/provider/shared_attach
index 1b8bd92b..be60d5ff 100644
--- a/objstorage/objstorageprovider/testdata/provider/shared_attach
+++ b/objstorage/objstorageprovider/testdata/provider/shared_attach
@@ -65,6 +65,7 @@ close
 ----
 <local fs> sync: p1
 <local fs> sync: p1/SHARED-CATALOG-000001
+<local fs> close: p1/SHARED-CATALOG-000001
 <local fs> close: p1
 
 open p2 2
diff --git a/objstorage/objstorageprovider/testdata/provider/shared_basic b/objstorage/objstorageprovider/testdata/provider/shared_basic
index 0abfc86b..bebc95cc 100644
--- a/objstorage/objstorageprovider/testdata/provider/shared_basic
+++ b/objstorage/objstorageprovider/testdata/provider/shared_basic
@@ -54,6 +54,7 @@ close
 ----
 <local fs> sync: p1
 <local fs> sync: p1/SHARED-CATALOG-000001
+<local fs> close: p1/SHARED-CATALOG-000001
 <local fs> close: p1
 
 # Test that the objects are there on re-open.
@@ -127,4 +128,5 @@ close
 <local fs> sync: p1
 <local fs> remove: p1/SHARED-CATALOG-000001
 <local fs> sync: p1/SHARED-CATALOG-000002
+<local fs> close: p1/SHARED-CATALOG-000002
 <local fs> close: p1
diff --git a/objstorage/objstorageprovider/testdata/provider/shared_no_ref b/objstorage/objstorageprovider/testdata/provider/shared_no_ref
index e7520e77..8b6d079c 100644
--- a/objstorage/objstorageprovider/testdata/provider/shared_no_ref
+++ b/objstorage/objstorageprovider/testdata/provider/shared_no_ref
@@ -67,6 +67,7 @@ size: 100
 close
 ----
 <local fs> sync: p1/SHARED-CATALOG-000001
+<local fs> close: p1/SHARED-CATALOG-000001
 <local fs> close: p1
 
 # Test that the objects are there on re-open.
diff --git a/snapshot_test.go b/snapshot_test.go
index a1d19ac5..eb272edc 100644
--- a/snapshot_test.go
+++ b/snapshot_test.go
@@ -7,6 +7,7 @@ package pebble
 import (
 	"bytes"
 	"fmt"
+	"math/rand"
 	"reflect"
 	"runtime"
 	"strings"
@@ -60,15 +61,29 @@ func TestSnapshot(t *testing.T) {
 	}
 	defer close()
 
+	randVersion := func() FormatMajorVersion {
+		minVersion := FormatUnusedPrePebblev1MarkedCompacted
+		return FormatMajorVersion(int(minVersion) + rand.Intn(
+			int(internalFormatNewest)-int(minVersion)+1))
+	}
 	datadriven.RunTest(t, "testdata/snapshot", func(t *testing.T, td *datadriven.TestData) string {
 		switch td.Cmd {
 		case "define":
 			close()
 
 			var err error
-			d, err = Open("", &Options{
-				FS: vfs.NewMem(),
-			})
+			options := &Options{
+				FS:                 vfs.NewMem(),
+				FormatMajorVersion: randVersion(),
+			}
+			if td.HasArg("block-size") {
+				var blockSize int
+				td.ScanArgs(t, "block-size", &blockSize)
+				options.Levels = make([]LevelOptions, 1)
+				options.Levels[0].BlockSize = blockSize
+				options.Levels[0].IndexBlockSize = blockSize
+			}
+			d, err = Open("", options)
 			if err != nil {
 				return err.Error()
 			}
@@ -119,6 +134,12 @@ func TestSnapshot(t *testing.T) {
 			}
 			return ""
 
+		case "db-state":
+			d.mu.Lock()
+			s := d.mu.versions.currentVersion().String()
+			d.mu.Unlock()
+			return s
+
 		case "iter":
 			var iter *Iterator
 			if len(td.CmdArgs) == 1 {
diff --git a/sstable/reader_test.go b/sstable/reader_test.go
index 4ea17eaa..9509ef42 100644
--- a/sstable/reader_test.go
+++ b/sstable/reader_test.go
@@ -492,8 +492,6 @@ func TestReader(t *testing.T) {
 }
 
 func TestReaderHideObsolete(t *testing.T) {
-	t.Skip("#2705")
-
 	blockSizes := map[string]int{
 		"1bytes":   1,
 		"5bytes":   5,
diff --git a/sstable/testdata/reader_hide_obsolete/iter b/sstable/testdata/reader_hide_obsolete/iter
index 0582ba56..5f6eab79 100644
--- a/sstable/testdata/reader_hide_obsolete/iter
+++ b/sstable/testdata/reader_hide_obsolete/iter
@@ -142,14 +142,14 @@ prev
 <b:16>:B16
 <c:30>:C30
 <c:28>:C28
-.
+<c:26>:
+<c:26>:
 <c:28>:C28
 <c:30>:C30
 <b:16>:B16
 <b:18>:B18
 <b:20>:B20
 <a:3>:A
-.
 
 build
 b.MERGE.20:B20
@@ -182,10 +182,12 @@ first
 next
 next
 next
+next
 ----
 <b:20>:B20
 <b:16>:B16
 <c:30>:C30
+<c:28>:
 .
 
 build
diff --git a/sstable/testdata/writer_value_blocks b/sstable/testdata/writer_value_blocks
index 8f1428bd..e98a4b55 100644
--- a/sstable/testdata/writer_value_blocks
+++ b/sstable/testdata/writer_value_blocks
@@ -132,7 +132,7 @@ layout
         72    record (21 = 3 [0] + 14 + 4) [restart]
                 blue@8#16,1:value handle {valueLen:6 blockNum:0 offsetInBlock:5}
         93    [restart 72]
-       101    [trailer compression=none checksum=0xdc74261]
+       101    [trailer compression=none checksum=0x4e65b9b6]
        106  data (29)
        106    record (21 = 3 [0] + 14 + 4) [restart]
                 blue@6#16,1:value handle {valueLen:15 blockNum:1 offsetInBlock:0}
@@ -146,71 +146,71 @@ layout
        173    block:38/29 [restart]
        192    [restart 173]
        200    [trailer compression=none checksum=0x21d27815]
-       205  index (27)
+       205  index (30)
        205    block:72/29 [restart]
-       224    [restart 205]
-       232    [trailer compression=none checksum=0xbae26eb3]
-       237  index (22)
-       237    block:106/29 [restart]
-       251    [restart 237]
-       259    [trailer compression=none checksum=0x802be702]
-       264  top-index (77)
-       264    block:140/28 [restart]
-       285    block:173/27 [restart]
-       305    block:205/27 [restart]
-       325    block:237/22 [restart]
-       340    [restart 264]
-       344    [restart 285]
-       348    [restart 305]
-       352    [restart 325]
-       341    [trailer compression=snappy checksum=0x6b2d79b]
-       346  value-block (11)
-       362  value-block (15)
-       382  value-index (8)
-       395  properties (676)
-       395    obsolete-key (16) [restart]
-       411    pebble.num.value-blocks (27)
-       438    pebble.num.values.in.value-blocks (21)
-       459    pebble.value-blocks.size (21)
-       480    rocksdb.block.based.table.index.type (43)
-       523    rocksdb.block.based.table.prefix.filtering (20)
-       543    rocksdb.block.based.table.whole.key.filtering (23)
-       566    rocksdb.comparator (37)
-       603    rocksdb.compression (16)
-       619    rocksdb.compression_options (106)
-       725    rocksdb.data.size (14)
-       739    rocksdb.deleted.keys (15)
-       754    rocksdb.external_sst_file.global_seqno (41)
-       795    rocksdb.external_sst_file.version (14)
-       809    rocksdb.filter.size (15)
-       824    rocksdb.index.partitions (20)
-       844    rocksdb.index.size (9)
-       853    rocksdb.merge.operands (18)
-       871    rocksdb.merge.operator (24)
-       895    rocksdb.num.data.blocks (19)
-       914    rocksdb.num.entries (11)
-       925    rocksdb.num.range-deletions (19)
-       944    rocksdb.prefix.extractor.name (31)
-       975    rocksdb.property.collectors (34)
-      1009    rocksdb.raw.key.size (16)
-      1025    rocksdb.raw.value.size (14)
-      1039    rocksdb.top-level.index.size (24)
-      1063    [restart 395]
-      1071    [trailer compression=none checksum=0x8f5517f7]
-      1076  meta-index (64)
-      1076    pebble.value_index block:382/8 value-blocks-index-lengths: 1(num), 2(offset), 1(length) [restart]
-      1103    rocksdb.properties block:395/676 [restart]
-      1128    [restart 1076]
-      1132    [restart 1103]
-      1140    [trailer compression=none checksum=0xb465a0c2]
-      1145  footer (53)
-      1145    checksum type: crc32c
-      1146    meta: offset=1076, length=64
-      1149    index: offset=264, length=77
-      1152    [padding]
-      1186    version: 4
-      1190    magic number: 0xf09faab3f09faab3
-      1198  EOF
+       227    [restart 205]
+       235    [trailer compression=none checksum=0xba0b26fe]
+       240  index (22)
+       240    block:106/29 [restart]
+       254    [restart 240]
+       262    [trailer compression=none checksum=0x802be702]
+       267  top-index (85)
+       267    block:140/28 [restart]
+       288    block:173/27 [restart]
+       308    block:205/30 [restart]
+       331    block:240/22 [restart]
+       346    [restart 267]
+       350    [restart 288]
+       354    [restart 308]
+       358    [restart 331]
+       352    [trailer compression=snappy checksum=0x8bd0d63a]
+       357  value-block (11)
+       373  value-block (15)
+       393  value-index (8)
+       406  properties (676)
+       406    obsolete-key (16) [restart]
+       422    pebble.num.value-blocks (27)
+       449    pebble.num.values.in.value-blocks (21)
+       470    pebble.value-blocks.size (21)
+       491    rocksdb.block.based.table.index.type (43)
+       534    rocksdb.block.based.table.prefix.filtering (20)
+       554    rocksdb.block.based.table.whole.key.filtering (23)
+       577    rocksdb.comparator (37)
+       614    rocksdb.compression (16)
+       630    rocksdb.compression_options (106)
+       736    rocksdb.data.size (14)
+       750    rocksdb.deleted.keys (15)
+       765    rocksdb.external_sst_file.global_seqno (41)
+       806    rocksdb.external_sst_file.version (14)
+       820    rocksdb.filter.size (15)
+       835    rocksdb.index.partitions (20)
+       855    rocksdb.index.size (9)
+       864    rocksdb.merge.operands (18)
+       882    rocksdb.merge.operator (24)
+       906    rocksdb.num.data.blocks (19)
+       925    rocksdb.num.entries (11)
+       936    rocksdb.num.range-deletions (19)
+       955    rocksdb.prefix.extractor.name (31)
+       986    rocksdb.property.collectors (34)
+      1020    rocksdb.raw.key.size (16)
+      1036    rocksdb.raw.value.size (14)
+      1050    rocksdb.top-level.index.size (24)
+      1074    [restart 406]
+      1082    [trailer compression=none checksum=0xbf6fe705]
+      1087  meta-index (64)
+      1087    pebble.value_index block:393/8 value-blocks-index-lengths: 1(num), 2(offset), 1(length) [restart]
+      1114    rocksdb.properties block:406/676 [restart]
+      1139    [restart 1087]
+      1143    [restart 1114]
+      1151    [trailer compression=none checksum=0x5a8a2a98]
+      1156  footer (53)
+      1156    checksum type: crc32c
+      1157    meta: offset=1087, length=64
+      1160    index: offset=267, length=85
+      1163    [padding]
+      1197    version: 4
+      1201    magic number: 0xf09faab3f09faab3
+      1209  EOF
 
 # Require that [c,e) must be in-place.
 build in-place-bound=(c,e)
diff --git a/testdata/compaction_iter_delete_sized b/testdata/compaction_iter_delete_sized
index 4de8d2e8..d74a8af6 100644
--- a/testdata/compaction_iter_delete_sized
+++ b/testdata/compaction_iter_delete_sized
@@ -146,15 +146,15 @@ a#3,1:bcd[base]
 b#2,2:ab
 .
 
-iter snapshots=3
+iter snapshots=3 print-snapshot-pinned print-force-obsolete
 first
 next
 next
 next
 ----
-a#3,2:d
-a#2,1:bc[base]
-b#2,2:ab
+a#3,2:d (not pinned) (not force obsolete)
+a#2,1:bc[base] (pinned) (not force obsolete)
+b#2,2:ab (not pinned) (not force obsolete)
 .
 
 define
@@ -282,15 +282,15 @@ a.MERGE.1:c
 a.MERGE.0:d
 ----
 
-iter snapshots=(1,2)
+iter snapshots=(1,2) print-snapshot-pinned print-force-obsolete
 first
 next
 next
 next
 ----
-a#2,2:b
-a#1,2:c
-a#0,2:d
+a#2,2:b (not pinned) (not force obsolete)
+a#1,2:c (pinned) (not force obsolete)
+a#0,2:d (pinned) (not force obsolete)
 .
 
 define
@@ -334,7 +334,7 @@ b-c#1
 c-d#4
 .
 
-iter snapshots=3
+iter snapshots=3 print-snapshot-pinned print-force-obsolete
 first
 next
 next
@@ -342,10 +342,10 @@ next
 next
 tombstones
 ----
-a#2,18:b
-a#1,15:c
-b#4,15:d
-b#2,1:e
+a#2,18:b (not pinned) (not force obsolete)
+a#1,15:c (not pinned) (not force obsolete)
+b#4,15:d (not pinned) (not force obsolete)
+b#2,1:e (pinned) (force obsolete)
 .
 a-b#1
 b-c#4
@@ -353,7 +353,7 @@ b-c#1
 c-d#4
 .
 
-iter snapshots=4
+iter snapshots=4 print-snapshot-pinned print-force-obsolete
 first
 next
 next
@@ -362,11 +362,11 @@ next
 next
 tombstones
 ----
-a#2,18:b
-a#1,15:c
-b#4,15:d
-b#2,1:e
-c#3,1:f
+a#2,18:b (not pinned) (not force obsolete)
+a#1,15:c (not pinned) (not force obsolete)
+b#4,15:d (not pinned) (not force obsolete)
+b#2,1:e (pinned) (force obsolete)
+c#3,1:f (pinned) (force obsolete)
 .
 a-b#1
 b-c#4
@@ -544,18 +544,18 @@ f.SET.9:f
 f.SET.8:f
 ----
 
-iter snapshots=(9,10)
+iter snapshots=(9,10) print-snapshot-pinned print-force-obsolete
 first
 next
 tombstones f
 next
 tombstones
 ----
-a#10,15:k
-f#9,1:f
+a#10,15:k (not pinned) (not force obsolete)
+f#9,1:f (pinned) (force obsolete)
 a-f#10
 .
-f#8,1:f
+f#8,1:f (pinned) (force obsolete)
 f-k#10
 .
 
diff --git a/testdata/manual_compaction_set_with_del_sstable_Pebblev4 b/testdata/manual_compaction_set_with_del_sstable_Pebblev4
index f97cb073..a705d096 100644
--- a/testdata/manual_compaction_set_with_del_sstable_Pebblev4
+++ b/testdata/manual_compaction_set_with_del_sstable_Pebblev4
@@ -861,3 +861,28 @@ compact a-z L1
 2:
   000006:[b#0,SET-b#0,SET]
   000007:[c#0,SET-c#0,SET]
+
+define snapshots=(10)
+L1
+  a.MERGE.15:a15
+L2
+  a.SET.5:a5
+----
+1:
+  000004:[a#15,MERGE-a#15,MERGE]
+2:
+  000005:[a#5,SET-a#5,SET]
+
+compact a-z
+----
+3:
+  000006:[a#15,MERGE-a#0,SET]
+
+# Fix for #2705. When snapshotPinned was used to set force obsolete, the
+# merged value would be a15 since the SET was incorrectly ignored.
+iter
+first
+next
+----
+a: (a5a15, .)
+.
diff --git a/testdata/snapshot b/testdata/snapshot
index d05dfdb2..93998128 100644
--- a/testdata/snapshot
+++ b/testdata/snapshot
@@ -149,3 +149,32 @@ prev
 a:123
 .
 a:123
+
+# Fix for #2705. levelIter encounters two files where the first has seqnum
+# below snapshot seqnum, so obsolete points can be hidden. The second file has
+# a more recent seqnum so obsolete points cannot be hidden. But the in-place
+# modification of the filters slice was causing obsolete points to be hidden
+# in the second file.
+define block-size=1
+set a 1
+compact a-b
+set c 2
+snapshot 1
+set c 3
+compact c-d
+----
+
+db-state
+----
+6:
+  000005:[a#10,SET-a#10,SET]
+  000007:[c#12,SET-c#11,SET]
+
+iter snapshot=1
+first
+next
+next
+----
+a:1
+c:2
+.