diff --git a/pkg/storage/batch_spanset_test.go b/pkg/storage/batch_spanset_test.go index 4897bb4382ab..0d6d4c25f500 100644 --- a/pkg/storage/batch_spanset_test.go +++ b/pkg/storage/batch_spanset_test.go @@ -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")) @@ -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() @@ -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) { @@ -148,7 +212,7 @@ 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 @@ -156,47 +220,55 @@ func TestSpanSetBatchBoundaries(t *testing.T) { 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) { diff --git a/pkg/storage/spanset/batch.go b/pkg/storage/spanset/batch.go index bdda9ce40c4a..542a04e98990 100644 --- a/pkg/storage/spanset/batch.go +++ b/pkg/storage/spanset/batch.go @@ -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 diff --git a/pkg/storage/spanset/spanset.go b/pkg/storage/spanset/spanset.go index da68a2479ee5..3b0fa33b0426 100644 --- a/pkg/storage/spanset/spanset.go +++ b/pkg/storage/spanset/spanset.go @@ -174,6 +174,10 @@ func (s *SpanSet) AssertAllowed(access SpanAccess, span roachpb.Span) { // keyspan. Timestamps associated with the spans in the spanset are not // considered, only the span boundaries are checked. // +// If the provided span contains only an (exclusive) EndKey and has a nil +// (inclusive) Key then Key is considered to be the key previous to EndKey, +// i.e. [,b) will be considered [b.Prev(),b). +// // TODO(irfansharif): This does not currently work for spans that straddle // across multiple added spans. Specifically a spanset with spans [a-c) and // [b-d) added under read only and read write access modes respectively would @@ -181,22 +185,52 @@ func (s *SpanSet) AssertAllowed(access SpanAccess, span roachpb.Span) { // is also a problem if the added spans were read only and the spanset wasn't // already SortAndDedup-ed. func (s *SpanSet) CheckAllowed(access SpanAccess, span roachpb.Span) error { - return s.checkAllowed(access, span, false /* spanKeyExclusive */) + return s.checkAllowed(access, span, func(_ SpanAccess, _ Span) bool { + return true + }) } -// See CheckAllowed(). The reversed arguments makes the lower bound exclusive -// and the upper bound inclusive, i.e. [a,b) will be considered (a,b]. -func (s *SpanSet) checkAllowed(access SpanAccess, span roachpb.Span, reversed bool) error { +// CheckAllowedAt is like CheckAllowed, except it returns an error if the access +// is not allowed at over the given keyspan at the given timestamp. +func (s *SpanSet) CheckAllowedAt( + access SpanAccess, span roachpb.Span, timestamp hlc.Timestamp, +) error { + return s.checkAllowed(access, span, func(declAccess SpanAccess, declSpan Span) bool { + declTimestamp := declSpan.Timestamp + if declTimestamp.IsEmpty() { + // When the span is declared as non-MVCC (i.e. with an empty + // timestamp), it's equivalent to a read/write mutex where we + // don't consider access timestamps. + return true + } + + switch access { + case SpanReadOnly: + // Read spans acquired at a specific timestamp only allow reads + // at that timestamp and below. Non-MVCC access is not allowed. + return !timestamp.IsEmpty() && timestamp.LessEq(declTimestamp) + case SpanReadWrite: + // Writes under a write span with an associated timestamp at that + // specific timestamp. + return timestamp.Equal(declTimestamp) + default: + panic("unexpected span access") + } + }) +} + +func (s *SpanSet) checkAllowed( + access SpanAccess, span roachpb.Span, check func(SpanAccess, Span) bool, +) error { scope := SpanGlobal - if keys.IsLocal(span.Key) { + if (span.Key != nil && keys.IsLocal(span.Key)) || + (span.EndKey != nil && keys.IsLocal(span.EndKey)) { scope = SpanLocal } for ac := access; ac < NumSpanAccess; ac++ { for _, cur := range s.spans[ac][scope] { - if cur.Contains(span) && - (!reversed || cur.EndKey != nil && !cur.Key.Equal(span.Key)) || - reversed && cur.EndKey.Equal(span.Key) { + if contains(cur.Span, span) && check(ac, cur) { return nil } } @@ -205,55 +239,24 @@ func (s *SpanSet) checkAllowed(access SpanAccess, span roachpb.Span, reversed bo return errors.Errorf("cannot %s undeclared span %s\ndeclared:\n%s", access, span, s) } -// CheckAllowedAt returns an error if the access is not allowed at over the given keyspan -// at the given timestamp. -func (s *SpanSet) CheckAllowedAt( - access SpanAccess, span roachpb.Span, timestamp hlc.Timestamp, -) error { - return s.checkAllowedAt(access, span, timestamp, false /* inclusiveEnd */) -} - -// See CheckAllowedAt. The reversed arguments makes the lower bound exclusive -// and the upper bound inclusive, i.e. [a,b) will be considered (a,b]. -func (s *SpanSet) checkAllowedAt( - access SpanAccess, span roachpb.Span, timestamp hlc.Timestamp, reversed bool, -) error { - scope := SpanGlobal - if keys.IsLocal(span.Key) { - scope = SpanLocal +// contains returns whether s1 contains s2. Unlike Span.Contains, this function +// supports spans 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). +func contains(s1, s2 roachpb.Span) bool { + if s2.Key != nil { + // The common case. + return s1.Contains(s2) } - for ac := access; ac < NumSpanAccess; ac++ { - for _, cur := range s.spans[ac][scope] { - if (cur.Contains(span) && - (!reversed || (cur.EndKey != nil && !cur.Key.Equal(span.Key)))) || - (reversed && cur.EndKey.Equal(span.Key)) { - if cur.Timestamp.IsEmpty() { - // When the span is acquired as non-MVCC (i.e. with an empty - // timestamp), it's equivalent to a read/write mutex where we don't - // consider access timestamps. - return nil - } + // The following is equivalent to: + // s1.Contains(roachpb.Span{Key: s2.EndKey.Prev()}) - if access == SpanReadWrite { - // Writes under a write span with an associated timestamp at that - // specific timestamp. - if timestamp == cur.Timestamp { - return nil - } - } else { - // Read spans acquired at a specific timestamp only allow reads at - // that timestamp and below. Non-MVCC access is not allowed. - if !timestamp.IsEmpty() && (timestamp.Less(cur.Timestamp) || timestamp == cur.Timestamp) { - return nil - } - } - } - } + if s1.EndKey == nil { + return s1.Key.IsPrev(s2.EndKey) } - return errors.Errorf("cannot %s undeclared span %s at %s\ndeclared:\n%s", - access, span, timestamp.String(), s) + return s1.Key.Compare(s2.EndKey) < 0 && s1.EndKey.Compare(s2.EndKey) >= 0 } // Validate returns an error if any spans that have been added to the set diff --git a/pkg/storage/spanset/spanset_test.go b/pkg/storage/spanset/spanset_test.go index 45912d5f4cac..9c61944c5464 100644 --- a/pkg/storage/spanset/spanset_test.go +++ b/pkg/storage/spanset/spanset_test.go @@ -228,23 +228,23 @@ func TestSpanSetCheckAllowedReversed(t *testing.T) { allowed := []roachpb.Span{ // Exactly as declared. - {Key: roachpb.Key("d")}, - {Key: roachpb.Key("q")}, + {EndKey: roachpb.Key("d")}, + {EndKey: roachpb.Key("q")}, } for _, span := range allowed { - if err := bdGkq.checkAllowed(SpanReadOnly, span, true /* spanKeyExclusive */); err != nil { + if err := bdGkq.CheckAllowed(SpanReadOnly, span); err != nil { t.Errorf("expected %s to be allowed, but got error: %+v", span, err) } } disallowed := []roachpb.Span{ // Points outside the declared spans, and on the endpoints. - {Key: roachpb.Key("b")}, - {Key: roachpb.Key("g")}, - {Key: roachpb.Key("k")}, + {EndKey: roachpb.Key("b")}, + {EndKey: roachpb.Key("g")}, + {EndKey: roachpb.Key("k")}, } for _, span := range disallowed { - if err := bdGkq.checkAllowed(SpanReadOnly, span, true /* spanKeyExclusive */); err == nil { + if err := bdGkq.CheckAllowed(SpanReadOnly, span); err == nil { t.Errorf("expected %s to be disallowed", span) } } @@ -261,23 +261,23 @@ func TestSpanSetCheckAllowedAtReversed(t *testing.T) { allowed := []roachpb.Span{ // Exactly as declared. - {Key: roachpb.Key("d")}, - {Key: roachpb.Key("q")}, + {EndKey: roachpb.Key("d")}, + {EndKey: roachpb.Key("q")}, } for _, span := range allowed { - if err := bdGkq.checkAllowedAt(SpanReadOnly, span, ts, true /* spanKeyExclusive */); err != nil { + if err := bdGkq.CheckAllowedAt(SpanReadOnly, span, ts); err != nil { t.Errorf("expected %s to be allowed, but got error: %+v", span, err) } } disallowed := []roachpb.Span{ // Points outside the declared spans, and on the endpoints. - {Key: roachpb.Key("b")}, - {Key: roachpb.Key("g")}, - {Key: roachpb.Key("k")}, + {EndKey: roachpb.Key("b")}, + {EndKey: roachpb.Key("g")}, + {EndKey: roachpb.Key("k")}, } for _, span := range disallowed { - if err := bdGkq.checkAllowedAt(SpanReadOnly, span, ts, true /* spanKeyExclusive */); err == nil { + if err := bdGkq.CheckAllowedAt(SpanReadOnly, span, ts); err == nil { t.Errorf("expected %s to be disallowed", span) } }