Skip to content

Commit

Permalink
Merge #45085
Browse files Browse the repository at this point in the history 
45085: storage/spanset: clarify and clean up "reversed" span checks r=nvanbenschoten a=nvanbenschoten

The notion of "reversed" span checks was introduced in 5176bac. That was a good change which allowed for proper validation of spans when using `spanset.Iterator.SeekLT`. However, the semantics around the `reversed` argument added to `SpanSet.checkAllowed` were strange and under-specified. Now that the start key of the span was exclusive, what did it mean to provide a reversed multi-key span to `checkAllowed`? Was the start key before or after the end key? Was it ok that only the exclusive portion of the span was being provided by all callers? Were reversed multi-key spans even supported? The comment said that "the reversed arguments makes the lower bound exclusive and the upper bound inclusive, i.e. [a,b) will be considered (a,b]". It's unclear whether this was mistakenly meaning that "[a,b) will be considered (b,a]". This all led to a terribly confusing condition: https://github.com/cockroachdb/cockroach/blob/5d69fd053ba52ae7ce94567b7b5fbb7cd857f1af/pkg/storage/spanset/spanset.go#L197.

This commit clarifies these semantics by removing the `reversed` flag while retaining roughly the same idea in a way that's consistent with the existing meaning of a "Span". `SpanSet.checkAllowed` now supports an extended span format with a nil start key and a non-nil end key (e.g. "[nil, c)"). In this form, s2.Key (inclusive) is considered to be the previous key to s2.EndKey (exclusive). This avoids any ambiguity around multi-key "reversed" spans and fits in better with the existing definition of a Span.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
  • Loading branch information
@nvanbenschoten
craig[bot] and nvanbenschoten committed Feb 14, 2020
2 parents e671a4e + aa13826 commit 0469f81
Show file tree
Hide file tree
Showing 4 changed files with 232 additions and 155 deletions.
244 changes: 158 additions & 86 deletions pkg/storage/batch_spanset_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
ss.AddNonMVCC(spanset.SpanReadWrite, roachpb.Span{Key: roachpb.Key("c"), EndKey: roachpb.Key("g")})
outsideKey := engine.MakeMVCCMetadataKey(roachpb.Key("a"))
outsideKey2 := engine.MakeMVCCMetadataKey(roachpb.Key("b"))
outsideKey3 := engine.MakeMVCCMetadataKey(roachpb.Key("m"))
outsideKey3 := engine.MakeMVCCMetadataKey(roachpb.Key("g"))
outsideKey4 := engine.MakeMVCCMetadataKey(roachpb.Key("m"))
insideKey := engine.MakeMVCCMetadataKey(roachpb.Key("c"))
insideKey2 := engine.MakeMVCCMetadataKey(roachpb.Key("d"))
insideKey3 := engine.MakeMVCCMetadataKey(roachpb.Key("f"))
Expand Down Expand Up @@ -67,56 +68,115 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
isWriteSpanErr := func(err error) bool {
return testutils.IsError(err, "cannot write undeclared span")
}
if err := batch.Clear(outsideKey); !isWriteSpanErr(err) {
t.Errorf("Clear: unexpected error %v", err)
}
if err := batch.ClearRange(outsideKey, outsideKey2); !isWriteSpanErr(err) {
t.Errorf("ClearRange: unexpected error %v", err)
}
{
iter := batch.NewIterator(engine.IterOptions{UpperBound: roachpb.KeyMax})
err := batch.ClearIterRange(iter, outsideKey.Key, outsideKey2.Key)
iter.Close()
if !isWriteSpanErr(err) {
t.Errorf("ClearIterRange: unexpected error %v", err)

t.Run("writes before range", func(t *testing.T) {
if err := batch.Clear(outsideKey); !isWriteSpanErr(err) {
t.Errorf("Clear: unexpected error %v", err)
}
}
if err := batch.Merge(outsideKey, nil); !isWriteSpanErr(err) {
t.Errorf("Merge: unexpected error %v", err)
}
if err := batch.Put(outsideKey, nil); !isWriteSpanErr(err) {
t.Errorf("Put: unexpected error %v", err)
}
if err := batch.ClearRange(outsideKey, outsideKey2); !isWriteSpanErr(err) {
t.Errorf("ClearRange: unexpected error %v", err)
}
{
iter := batch.NewIterator(engine.IterOptions{UpperBound: roachpb.KeyMax})
err := batch.ClearIterRange(iter, outsideKey.Key, outsideKey2.Key)
iter.Close()
if !isWriteSpanErr(err) {
t.Errorf("ClearIterRange: unexpected error %v", err)
}
}
if err := batch.Merge(outsideKey, nil); !isWriteSpanErr(err) {
t.Errorf("Merge: unexpected error %v", err)
}
if err := batch.Put(outsideKey, nil); !isWriteSpanErr(err) {
t.Errorf("Put: unexpected error %v", err)
}
})

// Reads inside the range work.
//lint:ignore SA1019 historical usage of deprecated batch.Get is OK
if value, err := batch.Get(insideKey); err != nil {
t.Errorf("failed to read inside the range: %+v", err)
} else if !bytes.Equal(value, []byte("value")) {
t.Errorf("failed to read previously written value, got %q", value)
}
t.Run("writes after range", func(t *testing.T) {
if err := batch.Clear(outsideKey3); !isWriteSpanErr(err) {
t.Errorf("Clear: unexpected error %v", err)
}
if err := batch.ClearRange(insideKey2, outsideKey4); !isWriteSpanErr(err) {
t.Errorf("ClearRange: unexpected error %v", err)
}
{
iter := batch.NewIterator(engine.IterOptions{UpperBound: roachpb.KeyMax})
err := batch.ClearIterRange(iter, insideKey2.Key, outsideKey4.Key)
iter.Close()
if !isWriteSpanErr(err) {
t.Errorf("ClearIterRange: unexpected error %v", err)
}
}
if err := batch.Merge(outsideKey3, nil); !isWriteSpanErr(err) {
t.Errorf("Merge: unexpected error %v", err)
}
if err := batch.Put(outsideKey3, nil); !isWriteSpanErr(err) {
t.Errorf("Put: unexpected error %v", err)
}
})

t.Run("reads inside range", func(t *testing.T) {
//lint:ignore SA1019 historical usage of deprecated batch.Get is OK
if value, err := batch.Get(insideKey); err != nil {
t.Errorf("failed to read inside the range: %+v", err)
} else if !bytes.Equal(value, []byte("value")) {
t.Errorf("failed to read previously written value, got %q", value)
}
//lint:ignore SA1019 historical usage of deprecated batch.GetProto is OK
if _, _, _, err := batch.GetProto(insideKey, nil); err != nil {
t.Errorf("GetProto: unexpected error %v", err)
}
if err := batch.Iterate(insideKey.Key, insideKey2.Key,
func(v engine.MVCCKeyValue) (bool, error) {
return false, nil
},
); err != nil {
t.Errorf("Iterate: unexpected error %v", err)
}
})

// Reads outside the range fail.
isReadSpanErr := func(err error) bool {
return testutils.IsError(err, "cannot read undeclared span")
}
//lint:ignore SA1019 historical usage of deprecated batch.Get is OK
if _, err := batch.Get(outsideKey); !isReadSpanErr(err) {
t.Errorf("Get: unexpected error %v", err)
}
//lint:ignore SA1019 historical usage of deprecated batch.GetProto is OK
if _, _, _, err := batch.GetProto(outsideKey, nil); !isReadSpanErr(err) {
t.Errorf("GetProto: unexpected error %v", err)
}
if err := batch.Iterate(outsideKey.Key, outsideKey2.Key,
func(v engine.MVCCKeyValue) (bool, error) {
return false, errors.Errorf("unexpected callback: %v", v)
},
); !isReadSpanErr(err) {
t.Errorf("Iterate: unexpected error %v", err)
}

func() {
t.Run("reads before range", func(t *testing.T) {
//lint:ignore SA1019 historical usage of deprecated batch.Get is OK
if _, err := batch.Get(outsideKey); !isReadSpanErr(err) {
t.Errorf("Get: unexpected error %v", err)
}
//lint:ignore SA1019 historical usage of deprecated batch.GetProto is OK
if _, _, _, err := batch.GetProto(outsideKey, nil); !isReadSpanErr(err) {
t.Errorf("GetProto: unexpected error %v", err)
}
if err := batch.Iterate(outsideKey.Key, insideKey2.Key,
func(v engine.MVCCKeyValue) (bool, error) {
return false, errors.Errorf("unexpected callback: %v", v)
},
); !isReadSpanErr(err) {
t.Errorf("Iterate: unexpected error %v", err)
}
})

t.Run("reads after range", func(t *testing.T) {
//lint:ignore SA1019 historical usage of deprecated batch.Get is OK
if _, err := batch.Get(outsideKey3); !isReadSpanErr(err) {
t.Errorf("Get: unexpected error %v", err)
}
//lint:ignore SA1019 historical usage of deprecated batch.GetProto is OK
if _, _, _, err := batch.GetProto(outsideKey3, nil); !isReadSpanErr(err) {
t.Errorf("GetProto: unexpected error %v", err)
}
if err := batch.Iterate(insideKey2.Key, outsideKey4.Key,
func(v engine.MVCCKeyValue) (bool, error) {
return false, errors.Errorf("unexpected callback: %v", v)
},
); !isReadSpanErr(err) {
t.Errorf("Iterate: unexpected error %v", err)
}
})

t.Run("forward scans", func(t *testing.T) {
iter := batch.NewIterator(engine.IterOptions{UpperBound: roachpb.KeyMax})
defer iter.Close()

Expand All @@ -125,16 +185,20 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
if _, err := iter.Valid(); !isReadSpanErr(err) {
t.Fatalf("Seek: unexpected error %v", err)
}
iter.SeekGE(outsideKey3)
if _, err := iter.Valid(); !isReadSpanErr(err) {
t.Fatalf("Seek: unexpected error %v", err)
}
// Seeking back in bounds restores validity.
iter.SeekGE(insideKey)
if ok, err := iter.Valid(); !ok {
if ok, err := iter.Valid(); !ok || err != nil {
t.Fatalf("expected valid iterator, err=%v", err)
}
if !reflect.DeepEqual(iter.Key(), insideKey) {
t.Fatalf("expected key %s, got %s", insideKey, iter.Key())
}
iter.Next()
if ok, err := iter.Valid(); !ok {
if ok, err := iter.Valid(); !ok || err != nil {
t.Fatalf("expected valid iterator, err=%v", err)
}
if !reflect.DeepEqual(iter.Key(), insideKey2) {
Expand All @@ -148,55 +212,63 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
// Scanning out of bounds sets Valid() to false but is not an error.
t.Errorf("unexpected error on iterator: %+v", err)
}
}()
})

// Same test in reverse. We commit the batch and wrap an iterator on
// the raw engine because we don't support bidirectional iteration
// over a pending batch.
if err := batch.Commit(true); err != nil {
t.Fatal(err)
}
iter := spanset.NewIterator(eng.NewIterator(engine.IterOptions{UpperBound: roachpb.KeyMax}), &ss)
defer iter.Close()
iter.SeekLT(outsideKey)
if _, err := iter.Valid(); !isReadSpanErr(err) {
t.Fatalf("SeekLT: unexpected error %v", err)
}
// Seeking back in bounds restores validity.
iter.SeekLT(insideKey3)
if ok, err := iter.Valid(); !ok {
t.Fatalf("expected valid iterator, err=%v", err)
}
if !reflect.DeepEqual(iter.Key(), insideKey2) {
t.Fatalf("expected key %s, got %s", insideKey2, iter.Key())
}
iter.Prev()
if ok, err := iter.Valid(); !ok {
t.Fatalf("expected valid iterator, err=%v", err)
}
if !reflect.DeepEqual(iter.Key(), insideKey) {
t.Fatalf("expected key %s, got %s", insideKey, iter.Key())
}
// Scan out of bounds.
iter.Prev()
if ok, err := iter.Valid(); ok {
t.Fatalf("expected invalid iterator; found valid at key %s", iter.Key())
} else if err != nil {
t.Errorf("unexpected error on iterator: %+v", err)
}

// Seeking back in bounds restores validity.
iter.SeekLT(insideKey2)
if ok, err := iter.Valid(); !ok {
t.Fatalf("expected valid iterator, err=%v", err)
}
// SeekLT to the lower bound is invalid.
iter.SeekLT(insideKey)
if ok, err := iter.Valid(); ok {
t.Fatalf("expected invalid iterator; found valid at key %s", iter.Key())
} else if !isReadSpanErr(err) {
t.Fatalf("SeekLT: unexpected error %v", err)
}
t.Run("reverse scans", func(t *testing.T) {
iter := spanset.NewIterator(eng.NewIterator(engine.IterOptions{UpperBound: roachpb.KeyMax}), &ss)
defer iter.Close()
iter.SeekLT(outsideKey)
if _, err := iter.Valid(); !isReadSpanErr(err) {
t.Fatalf("SeekLT: unexpected error %v", err)
}
// Seeking back in bounds restores validity.
iter.SeekLT(insideKey3)
if ok, err := iter.Valid(); !ok || err != nil {
t.Fatalf("expected valid iterator, err=%v", err)
}
if !reflect.DeepEqual(iter.Key(), insideKey2) {
t.Fatalf("expected key %s, got %s", insideKey2, iter.Key())
}
iter.Prev()
if ok, err := iter.Valid(); !ok || err != nil {
t.Fatalf("expected valid iterator, err=%v", err)
}
if !reflect.DeepEqual(iter.Key(), insideKey) {
t.Fatalf("expected key %s, got %s", insideKey, iter.Key())
}
// Scan out of bounds.
iter.Prev()
if ok, err := iter.Valid(); ok {
t.Fatalf("expected invalid iterator; found valid at key %s", iter.Key())
} else if err != nil {
t.Errorf("unexpected error on iterator: %+v", err)
}

// Seeking back in bounds restores validity.
iter.SeekLT(insideKey2)
if ok, err := iter.Valid(); !ok || err != nil {
t.Fatalf("expected valid iterator, err=%v", err)
}
// SeekLT to the lower bound is invalid.
iter.SeekLT(insideKey)
if ok, err := iter.Valid(); ok {
t.Fatalf("expected invalid iterator; found valid at key %s", iter.Key())
} else if !isReadSpanErr(err) {
t.Fatalf("SeekLT: unexpected error %v", err)
}
// SeekLT to upper bound restores validity.
iter.SeekLT(outsideKey3)
if ok, err := iter.Valid(); !ok || err != nil {
t.Fatalf("expected valid iterator, err=%v", err)
}
})
}

func TestSpanSetBatchTimestamps(t *testing.T) {
Expand Down
8 changes: 5 additions & 3 deletions pkg/storage/spanset/batch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,13 @@ func (i *Iterator) SeekGE(key engine.MVCCKey) {

// SeekLT is part of the engine.Iterator interface.
func (i *Iterator) SeekLT(key engine.MVCCKey) {
const spanKeyExclusive = true
// CheckAllowed{At} supports the span representation of [,key), which
// corresponds to the span [key.Prev(),).
revSpan := roachpb.Span{EndKey: key.Key}
if i.spansOnly {
i.err = i.spans.checkAllowed(SpanReadOnly, roachpb.Span{Key: key.Key}, spanKeyExclusive)
i.err = i.spans.CheckAllowed(SpanReadOnly, revSpan)
} else {
i.err = i.spans.checkAllowedAt(SpanReadOnly, roachpb.Span{Key: key.Key}, i.ts, spanKeyExclusive)
i.err = i.spans.CheckAllowedAt(SpanReadOnly, revSpan, i.ts)
}
if i.err == nil {
i.invalid = false
Expand Down
Loading

0 comments on commit 0469f81

Please sign in to comment.