Skip to content

Commit

Permalink
internal/keyspan: lift visibility filtering out of Seek{GE,LE}
Browse files Browse the repository at this point in the history 
Alter `keyspan.SeekGE` and `keyspan.SeekLE` to omit any sequence number
filtering. The caller must now filter a Span's keys if necessary. This avoids
extra work in the case that there are no visible keys overlapping the search
key. Previously SeekGE and SeekLE would next or prev until they found a visible
key. Now, the caller can simply use the empty span.

This is partially motivated by the batch range deletion slowdown observed in
impact.
  • Loading branch information
@jbowens
jbowens committed May 6, 2022
1 parent d79f961 commit 4c9483a
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 86 deletions.
30 changes: 4 additions & 26 deletions internal/keyspan/seek.go
View file
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
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
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
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
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
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

0 comments on commit 4c9483a

Please sign in to comment.