Skip to content

Commit

Permalink
storage: adjust prefix point-synthesizing behavior
Browse files Browse the repository at this point in the history 
Update the point-synthesizing iterator to save range key state even when in
prefix iteration mode. Previously the point-synthesizing iterator avoided
copying the range key state when used within prefix iteration mode. This led to
a bug where an iterator could be moved into an exhausted position, at which the
previous range key buffers are no longer valid.

Release note: None
  • Loading branch information
@jbowens
jbowens committed Feb 24, 2023
1 parent e5d71c5 commit 415b0ee
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 27 deletions.
35 changes: 11 additions & 24 deletions pkg/storage/point_synthesizing_iter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,17 +288,11 @@ func (i *PointSynthesizingIter) updateRangeKeys() {
// last call to updateRangeKeys().
if i.rangeKeyChanged {
i.rangeKeyChanged = false
if i.prefix {
// A prefix iterator will always be at the start bound of the range key,
// and never move onto a different range key, so we can omit the cloning.
i.rangeKeys = i.iter.RangeKeys().Versions
} else {
rangeKeys := i.iter.RangeKeys()
i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeKeys.Bounds.Key...)
rangeKeys.Versions.CloneInto(&i.rangeKeysBuf)
i.rangeKeys = i.rangeKeysBuf
i.rangeKeysStartPassed = false // we'll compare below
}
rangeKeys := i.iter.RangeKeys()
i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeKeys.Bounds.Key...)
rangeKeys.Versions.CloneInto(&i.rangeKeysBuf)
i.rangeKeys = i.rangeKeysBuf
i.rangeKeysStartPassed = false // we'll compare below
}

// Update the current iterator position.
Expand Down Expand Up @@ -899,19 +893,12 @@ func (i *PointSynthesizingIter) assertInvariants() error {
}

// rangeKeysStart must be set, and rangeKeysPos must be at or after it.
// prefix iterators do not set rangeKeysStart.
if !i.prefix {
if len(i.rangeKeysStart) == 0 {
return errors.AssertionFailedf("no rangeKeysStart at %s", i.iter.UnsafeKey())
}
if i.rangeKeysPos.Compare(i.rangeKeysStart) < 0 {
return errors.AssertionFailedf("rangeKeysPos %s not after rangeKeysStart %s",
i.rangeKeysPos, i.rangeKeysStart)
}
} else {
if len(i.rangeKeysStart) != 0 {
return errors.AssertionFailedf("rangeKeysStart set to %s for prefix iterator", i.rangeKeysStart)
}
if len(i.rangeKeysStart) == 0 {
return errors.AssertionFailedf("no rangeKeysStart at %s", i.iter.UnsafeKey())
}
if i.rangeKeysPos.Compare(i.rangeKeysStart) < 0 {
return errors.AssertionFailedf("rangeKeysPos %s not after rangeKeysStart %s",
i.rangeKeysPos, i.rangeKeysStart)
}

// rangeKeysStartPassed must match rangeKeysPos and rangeKeysStart. Note that
Expand Down
107 changes: 104 additions & 3 deletions pkg/storage/testdata/mvcc_histories/range_tombstone_gets
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
# Sets up the following dataset, where x is tombstone, o-o is range tombstone, [] is intent.
#
# T
# 6 [e6]
# 6 [e6] o6
# 5 f5
# 4 o-----------------------o o-------o [j-l)@4 has localTs=3
# 3 x d3 f3
# 2 o---------------o h2
# 2 o---------------o h2 o--------o
# 1 a1 x c1 f1
# a b c d e f g h i j k l
# a b c d e f g h i j k l m n o p
#
run ok
put k=a ts=1 v=a1
Expand All @@ -21,12 +21,14 @@ del k=a ts=3
put k=d ts=3 v=d3
put k=f ts=3 v=f3
put k=h ts=2 v=h2
del_range_ts k=n end=p ts=2
del_range_ts k=c end=i ts=4
put k=f ts=5 v=f5
del_range_ts k=j end=l ts=4 localTs=3
with t=A
txn_begin k=e ts=6
put k=e v=e6
put k=o ts=6 v=o6
----
del: "b": found key false
del: "a": found key false
Expand All @@ -36,6 +38,7 @@ rangekey: {a-c}/[2.000000000,0=/<empty>]
rangekey: {c-e}/[4.000000000,0=/<empty> 2.000000000,0=/<empty>]
rangekey: {e-i}/[4.000000000,0=/<empty>]
rangekey: {j-l}/[4.000000000,0={localTs=3.000000000,0}/<empty>]
rangekey: {n-p}/[2.000000000,0=/<empty>]
data: "a"/3.000000000,0 -> /<empty>
data: "a"/1.000000000,0 -> /BYTES/a1
data: "b"/1.000000000,0 -> /<empty>
Expand All @@ -47,6 +50,7 @@ data: "f"/5.000000000,0 -> /BYTES/f5
data: "f"/3.000000000,0 -> /BYTES/f3
data: "f"/1.000000000,0 -> /BYTES/f1
data: "h"/2.000000000,0 -> /BYTES/h2
data: "o"/6.000000000,0 -> /BYTES/o6

# Run gets for all keys and all timestamps.
run ok
Expand Down Expand Up @@ -184,6 +188,87 @@ get: "h" -> /BYTES/h2 @2.000000000,0
get: "h" -> /<empty> @4.000000000,0
get: "h" -> /<empty> @4.000000000,0

run ok
get k=l ts=1
get k=l ts=2
get k=l ts=3
get k=l ts=4
get k=l ts=5
get k=l ts=6
get k=l ts=1 tombstones
get k=l ts=2 tombstones
get k=l ts=3 tombstones
get k=l ts=4 tombstones
get k=l ts=5 tombstones
get k=l ts=6 tombstones
----
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>
get: "l" -> <no data>

run ok
get k=n ts=1
get k=n ts=2
get k=n ts=3
get k=n ts=4
get k=n ts=5
get k=n ts=6
get k=n ts=1 tombstones
get k=n ts=2 tombstones
get k=n ts=3 tombstones
get k=n ts=4 tombstones
get k=n ts=5 tombstones
get k=n ts=6 tombstones
----
get: "n" -> <no data>
get: "n" -> <no data>
get: "n" -> <no data>
get: "n" -> <no data>
get: "n" -> <no data>
get: "n" -> <no data>
get: "n" -> <no data>
get: "n" -> /<empty> @2.000000000,0
get: "n" -> /<empty> @2.000000000,0
get: "n" -> /<empty> @2.000000000,0
get: "n" -> /<empty> @2.000000000,0
get: "n" -> /<empty> @2.000000000,0

run ok
get k=o ts=1
get k=o ts=2
get k=o ts=3
get k=o ts=4
get k=o ts=5
get k=o ts=6
get k=o ts=1 tombstones
get k=o ts=2 tombstones
get k=o ts=3 tombstones
get k=o ts=4 tombstones
get k=o ts=5 tombstones
get k=o ts=6 tombstones
----
get: "o" -> <no data>
get: "o" -> <no data>
get: "o" -> <no data>
get: "o" -> <no data>
get: "o" -> <no data>
get: "o" -> /BYTES/o6 @6.000000000,0
get: "o" -> <no data>
get: "o" -> /<empty> @2.000000000,0
get: "o" -> /<empty> @2.000000000,0
get: "o" -> /<empty> @2.000000000,0
get: "o" -> /<empty> @2.000000000,0
get: "o" -> /BYTES/o6 @6.000000000,0

# failOnMoreRecent: c
run error
get k=c ts=1 failOnMoreRecent
Expand Down Expand Up @@ -328,3 +413,19 @@ get k=k ts=1 globalUncertaintyLimit=4 localUncertaintyLimit=3
----
get: "k" -> <no data>
error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 1.000000000,0 encountered previous write with future timestamp 4.000000000,0 within uncertainty interval `t <= (local=3.000000000,0, global=0,0)`; observed timestamps: []

# Test a particular case where:
# - a tombstone must be synthesized due to a range tombstone
# - the uncertainty limit is such that we must check for uncertainty
# - the only point at the key is not visible at the read timstamp, but is also
# not outside the uncertainty limit
#
# In these circumstances, the scanner will seekVersion to find the first visible
# key (there is none), invalidating the underlying Pebble iterator. Although the
# underlying Pebble iterator has been invalidated, the scanner should still
# succeed in synthesizing a tombstone at the range key timestamp retrieved
# before the iterator was invalidated.
run ok
get k=o ts=5 tombstones globalUncertaintyLimit=6 localUncertaintyLimit=5
----
get: "o" -> /<empty> @2.000000000,0

0 comments on commit 415b0ee

Please sign in to comment.