Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/keyspan: lift visibility filtering out of Seek{GE,LE} #1687

Merged
merged 1 commit into from
May 18, 2022
Merged

internal/keyspan: lift visibility filtering out of Seek{GE,LE} #1687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 4 additions & 26 deletions internal/keyspan/seek.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,9 @@ package keyspan

import "github.com/cockroachdb/pebble/internal/base"

// TODO(jackson): Move snapshot visiblity outside of SeekGE/SeekLE and into the
// callsite. This should simplify the code some more.

// SeekGE seeks to the span that contains the target key or the first span past
// the target key. The snapshot parameter controls the visibility of keys (only
// spans older than the snapshot sequence number are visible).
func SeekGE(cmp base.Compare, iter FragmentIterator, key []byte, snapshot uint64) Span {
// the target key.
func SeekGE(cmp base.Compare, iter FragmentIterator, key []byte) Span {
// NB: We use SeekLT in order to land on the proper span for a search
// key that resides in the middle of a span. Consider the scenario:
//
Expand All @@ -35,21 +31,11 @@ func SeekGE(cmp base.Compare, iter FragmentIterator, key []byte, snapshot uint64
// guaranteed to lie at or past the search key.
iterSpan = iter.Next()
}
// The iterator is positioned at a valid iterSpan which is the first
// iterator position that satisfies the requirement that it contains or is
// past the target key. It may not be visible based on the snapshot. So now
// we only need to move forward and return the first span with visible keys.
iterSpan = iterSpan.Visible(snapshot)
for iterSpan.Valid() && iterSpan.Empty() {
iterSpan = iter.Next().Visible(snapshot)
}
return iterSpan
}

// SeekLE seeks to the span that contains or is before the target key. The
// snapshot parameter controls the visibility of keys (only spans older than the
// snapshot sequence number are visible).
func SeekLE(cmp base.Compare, iter FragmentIterator, key []byte, snapshot uint64) Span {
// SeekLE seeks to the span that contains or is before the target key.
func SeekLE(cmp base.Compare, iter FragmentIterator, key []byte) Span {
// NB: We use SeekLT in order to land on the proper span for a search
// key that resides in the middle of a span. Consider the scenario:
//
Expand Down Expand Up @@ -86,13 +72,5 @@ func SeekLE(cmp base.Compare, iter FragmentIterator, key []byte, snapshot uint64
}
}
}

// We're positioned at a span that contains or is before the search key. It
// may not be visible based on the snapshot. So now we only need to move
// backward and return the first span with visible keys.
iterSpan = iterSpan.Visible(snapshot)
for iterSpan.Valid() && iterSpan.Empty() {
iterSpan = iter.Prev().Visible(snapshot)
}
return iterSpan
}
5 changes: 4 additions & 1 deletion internal/keyspan/seek_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ func TestSeek(t *testing.T) {
if err != nil {
return err.Error()
}
span := seek(cmp, iter, []byte(parts[0]), seq)
span := seek(cmp, iter, []byte(parts[0]))
if span.Valid() {
span = span.Visible(seq)
}
fmt.Fprintln(&buf, span)
}
return buf.String()
Expand Down
101 changes: 51 additions & 50 deletions internal/keyspan/testdata/seek
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ d 2
----
b-d:{(#1,RANGEDEL)}
b-d:{(#1,RANGEDEL)}
<invalid>
b-d:{}
<invalid>

seek-le
Expand All @@ -22,7 +22,7 @@ d 2
----
<invalid>
b-d:{(#1,RANGEDEL)}
<invalid>
b-d:{}
b-d:{(#1,RANGEDEL)}

build
Expand All @@ -44,7 +44,7 @@ b-d:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}
b-d:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}
b-d:{(#2,RANGEDEL) (#1,RANGEDEL)}
b-d:{(#1,RANGEDEL)}
<invalid>
b-d:{}
<invalid>

seek-le
Expand All @@ -59,7 +59,7 @@ d 4
b-d:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}
b-d:{(#2,RANGEDEL) (#1,RANGEDEL)}
b-d:{(#1,RANGEDEL)}
<invalid>
b-d:{}
b-d:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}

build
Expand All @@ -76,7 +76,7 @@ d 3
e 3
----
b-d:{(#1,RANGEDEL)}
<invalid>
d-f:{}
d-f:{(#2,RANGEDEL)}
d-f:{(#2,RANGEDEL)}

Expand All @@ -90,7 +90,7 @@ f 3
----
<invalid>
b-d:{(#1,RANGEDEL)}
b-d:{(#1,RANGEDEL)}
d-f:{}
d-f:{(#2,RANGEDEL)}
d-f:{(#2,RANGEDEL)}
d-f:{(#2,RANGEDEL)}
Expand Down Expand Up @@ -127,22 +127,22 @@ s 1
z 2
----
a-f:{(#3,RANGEDEL)}
f-j:{(#2,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
a-f:{}
a-f:{}
a-f:{}
f-j:{(#3,RANGEDEL) (#2,RANGEDEL)}
f-j:{(#2,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
f-j:{}
f-j:{}
j-m:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
j-m:{}
m-s:{(#2,RANGEDEL) (#1,RANGEDEL)}
m-s:{(#1,RANGEDEL)}
<invalid>
m-s:{}
s-z:{(#1,RANGEDEL)}
<invalid>
s-z:{}
<invalid>

seek-le
Expand All @@ -166,22 +166,22 @@ s 1
z 2
----
a-f:{(#3,RANGEDEL)}
<invalid>
<invalid>
<invalid>
a-f:{}
a-f:{}
a-f:{}
f-j:{(#3,RANGEDEL) (#2,RANGEDEL)}
f-j:{(#2,RANGEDEL)}
<invalid>
<invalid>
f-j:{}
f-j:{}
j-m:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
j-m:{}
m-s:{(#2,RANGEDEL) (#1,RANGEDEL)}
m-s:{(#1,RANGEDEL)}
<invalid>
m-s:{}
s-z:{(#1,RANGEDEL)}
<invalid>
s-z:{}
s-z:{(#1,RANGEDEL)}

build
Expand Down Expand Up @@ -216,22 +216,22 @@ s 1
z 4
----
a-f:{(#1,RANGEDEL)}
<invalid>
a-f:{}
f-j:{(#2,RANGEDEL) (#1,RANGEDEL)}
f-j:{(#1,RANGEDEL)}
<invalid>
f-j:{}
j-m:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
j-m:{}
m-s:{(#3,RANGEDEL) (#2,RANGEDEL)}
m-s:{(#2,RANGEDEL)}
<invalid>
<invalid>
m-s:{}
m-s:{}
s-z:{(#3,RANGEDEL)}
<invalid>
<invalid>
<invalid>
s-z:{}
s-z:{}
s-z:{}
<invalid>

seek-le
Expand All @@ -257,25 +257,25 @@ z 3
z 2
----
a-f:{(#1,RANGEDEL)}
<invalid>
a-f:{}
f-j:{(#2,RANGEDEL) (#1,RANGEDEL)}
f-j:{(#1,RANGEDEL)}
<invalid>
f-j:{}
j-m:{(#3,RANGEDEL) (#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#2,RANGEDEL) (#1,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
j-m:{}
m-s:{(#3,RANGEDEL) (#2,RANGEDEL)}
m-s:{(#2,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
m-s:{}
m-s:{}
s-z:{(#3,RANGEDEL)}
m-s:{(#2,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
<invalid>
s-z:{}
s-z:{}
s-z:{}
s-z:{(#3,RANGEDEL)}
m-s:{(#2,RANGEDEL)}
j-m:{(#1,RANGEDEL)}
s-z:{}
s-z:{}

build
1: a-c
Expand All @@ -286,11 +286,12 @@ build
a-c:{(#5,RANGEDEL) (#3,RANGEDEL) (#1,RANGEDEL)}
c-e:{(#5,RANGEDEL)}

# Regression test for a bug where seek-le was failing to find the most
# recent version of a tombstone. The problematic case was "seek-le c
# 4". The seeking code was finding the tombstone c-e#5, determining it
# wasn't visible and then return the immediately preceding tombstone
# a-c#1, instead of the correct tombstone a-c#3.
# Regression test for a bug where seek-le was failing to find the most recent
# version of a tombstone. The bug existed when seek-{ge,le} performed snapshot
# filtering, and the problematic case was "seek-le c 4". The seeking code was
# finding the tombstone c-e#5, determining it wasn't visible and then return the
# immediately preceding tombstone a-c#1. Now we return c-e:{} immediately,
# because the span c-e covers c and contains no visible keys.

seek-le
c 1
Expand All @@ -300,9 +301,9 @@ c 4
c 5
c 6
----
<invalid>
a-c:{(#1,RANGEDEL)}
a-c:{(#1,RANGEDEL)}
a-c:{(#3,RANGEDEL) (#1,RANGEDEL)}
a-c:{(#3,RANGEDEL) (#1,RANGEDEL)}
c-e:{}
c-e:{}
c-e:{}
c-e:{}
c-e:{}
c-e:{(#5,RANGEDEL)}
2 changes: 1 addition & 1 deletion level_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func (m *simpleMergingIter) positionRangeDels() {
if l.rangeDelIter == nil {
continue
}
l.tombstone = keyspan.SeekGE(m.heap.cmp, l.rangeDelIter, item.key.UserKey, m.snapshot)
l.tombstone = keyspan.SeekGE(m.heap.cmp, l.rangeDelIter, item.key.UserKey).Visible(m.snapshot)
}
}

Expand Down
4 changes: 2 additions & 2 deletions level_iter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,9 +356,9 @@ func (i *levelIterTestIter) rangeDelSeek(
var tombstone keyspan.Span
if i.rangeDelIter != nil {
if dir < 0 {
tombstone = keyspan.SeekLE(i.levelIter.cmp, i.rangeDelIter, key, 1000)
tombstone = keyspan.SeekLE(i.levelIter.cmp, i.rangeDelIter, key).Visible(1000)
} else {
tombstone = keyspan.SeekGE(i.levelIter.cmp, i.rangeDelIter, key, 1000)
tombstone = keyspan.SeekGE(i.levelIter.cmp, i.rangeDelIter, key).Visible(1000)
}
}
if ikey == nil {
Expand Down
30 changes: 24 additions & 6 deletions merging_iter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,10 @@ func (m *mergingIter) initMinRangeDelIters(oldTopLevel int) {
if l.rangeDelIter == nil {
continue
}
l.tombstone = keyspan.SeekGE(m.heap.cmp, l.rangeDelIter, item.key.UserKey, m.snapshot)
l.tombstone = keyspan.SeekGE(m.heap.cmp, l.rangeDelIter, item.key.UserKey)
if l.tombstone.Valid() {
l.tombstone = l.tombstone.Visible(m.snapshot)
}
}
}

Expand All @@ -344,7 +347,10 @@ func (m *mergingIter) initMaxRangeDelIters(oldTopLevel int) {
if l.rangeDelIter == nil {
continue
}
l.tombstone = keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.key.UserKey, m.snapshot)
l.tombstone = keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.key.UserKey)
if l.tombstone.Valid() {
l.tombstone = l.tombstone.Visible(m.snapshot)
}
}
}

Expand Down Expand Up @@ -570,7 +576,10 @@ func (m *mergingIter) isNextEntryDeleted(item *mergingIterItem) bool {
// levelIter in the future cannot contain item.key). Also, it is possible that we
// will encounter parts of the range delete that should be ignored -- we handle that
// below.
l.tombstone = keyspan.SeekGE(m.heap.cmp, l.rangeDelIter, item.key.UserKey, m.snapshot)
l.tombstone = keyspan.SeekGE(m.heap.cmp, l.rangeDelIter, item.key.UserKey)
if l.tombstone.Valid() {
l.tombstone = l.tombstone.Visible(m.snapshot)
}
}
if !l.tombstone.Valid() {
continue
Expand Down Expand Up @@ -728,7 +737,10 @@ func (m *mergingIter) isPrevEntryDeleted(item *mergingIterItem) bool {
// levelIter in the future cannot contain item.key). Also, it is it is possible that we
// will encounter parts of the range delete that should be ignored -- we handle that
// below.
l.tombstone = keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.key.UserKey, m.snapshot)
l.tombstone = keyspan.SeekLE(m.heap.cmp, l.rangeDelIter, item.key.UserKey)
if l.tombstone.Valid() {
l.tombstone = l.tombstone.Visible(m.snapshot)
}
}
if !l.tombstone.Valid() {
continue
Expand Down Expand Up @@ -871,7 +883,10 @@ func (m *mergingIter) seekGE(key []byte, level int, trySeekUsingNext bool) {
// so we can have a sstable with bounds [c#8, i#InternalRangeDelSentinel], and the
// tombstone is [b, k)#8 and the seek key is i: levelIter.SeekGE(i) will move past
// this sstable since it realizes the largest key is a InternalRangeDelSentinel.
l.tombstone = keyspan.SeekGE(m.heap.cmp, rangeDelIter, key, m.snapshot)
l.tombstone = keyspan.SeekGE(m.heap.cmp, rangeDelIter, key)
if l.tombstone.Valid() {
l.tombstone = l.tombstone.Visible(m.snapshot)
}
if !l.tombstone.Empty() && l.tombstone.Contains(m.heap.cmp, key) &&
(l.smallestUserKey == nil || m.heap.cmp(l.smallestUserKey, key) <= 0) {
// NB: Based on the comment above l.largestUserKey >= key, and based on the
Expand Down Expand Up @@ -957,7 +972,10 @@ func (m *mergingIter) seekLT(key []byte, level int) {
withinLargestSSTableBound = cmpResult > 0 || (cmpResult == 0 && !l.isLargestUserKeyRangeDelSentinel)
}

l.tombstone = keyspan.SeekLE(m.heap.cmp, rangeDelIter, key, m.snapshot)
l.tombstone = keyspan.SeekLE(m.heap.cmp, rangeDelIter, key)
if l.tombstone.Valid() {
l.tombstone = l.tombstone.Visible(m.snapshot)
}
if !l.tombstone.Empty() && l.tombstone.Contains(m.heap.cmp, key) && withinLargestSSTableBound {
// NB: Based on the comment above l.smallestUserKey <= key, and based
// on the containment condition tombstone.Start.UserKey <= key, so the
Expand Down