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

util/must: add Handle for convenient failure handling #107790

Closed
wants to merge 3 commits into from
Closed

util/must: add Handle for convenient failure handling #107790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a83ae4
util/must: add `Handle` for convenient failure handling
erikgrinaker Jul 25, 2023
64a558d
util/must: add `DisableFatalAssertions` for tests
erikgrinaker Jul 30, 2023
e6b1b49
storage: use `must` for MVCC iterator assertions
erikgrinaker Jul 28, 2023
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
2 changes: 2 additions & 0 deletions pkg/storage/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ go_library(
"//pkg/util/log",
"//pkg/util/log/eventpb",
"//pkg/util/mon",
"//pkg/util/must",
"//pkg/util/protoutil",
"//pkg/util/syncutil",
"//pkg/util/sysutil",
Expand Down Expand Up @@ -175,6 +176,7 @@ go_test(
"//pkg/util/leaktest",
"//pkg/util/log",
"//pkg/util/mon",
"//pkg/util/must",
"//pkg/util/protoutil",
"//pkg/util/randutil",
"//pkg/util/shuffle",
Expand Down
268 changes: 113 additions & 155 deletions pkg/storage/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/iterutil"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
"github.com/cockroachdb/cockroach/pkg/util/must"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/cockroachdb/cockroach/pkg/util/uuid"
"github.com/cockroachdb/errors"
Expand Down Expand Up @@ -1713,185 +1714,142 @@ func iterateOnReader(
}

// assertSimpleMVCCIteratorInvariants asserts invariants in the
// SimpleMVCCIterator interface that should hold for all implementations,
// returning errors.AssertionFailedf for any violations. The iterator
// must be valid.
// SimpleMVCCIterator interface that should hold for all implementations. The
// iterator must be valid.
func assertSimpleMVCCIteratorInvariants(iter SimpleMVCCIterator) error {
key := iter.UnsafeKey()

// Keys can't be empty.
if len(key.Key) == 0 {
return errors.AssertionFailedf("valid iterator returned empty key")
}

// Can't be positioned in the lock table.
if bytes.HasPrefix(key.Key, keys.LocalRangeLockTablePrefix) {
return errors.AssertionFailedf("MVCC iterator positioned in lock table at %s", key)
}

// Any valid position must have either a point and/or range key.
hasPoint, hasRange := iter.HasPointAndRange()
if !hasPoint && !hasRange {
// NB: MVCCIncrementalIterator can return hasPoint=false,hasRange=false
// following a NextIgnoringTime() call. We explicitly allow this here.
if incrIter, ok := iter.(*MVCCIncrementalIterator); !ok || !incrIter.ignoringTime {
return errors.AssertionFailedf("valid iterator without point/range keys at %s", key)
}
}

// Range key assertions.
if hasRange {
// Must have bounds. The MVCCRangeKey.Validate() call below will make
// further bounds assertions.
bounds := iter.RangeBounds()
if len(bounds.Key) == 0 && len(bounds.EndKey) == 0 {
return errors.AssertionFailedf("hasRange=true but empty range bounds at %s", key)
}

// Iterator position must be within range key bounds.
if !bounds.ContainsKey(key.Key) {
return errors.AssertionFailedf("iterator position %s outside range bounds %s", key, bounds)
}

// Bounds must match range key stack.
rangeKeys := iter.RangeKeys()
if !rangeKeys.Bounds.Equal(bounds) {
return errors.AssertionFailedf("range bounds %s does not match range key %s",
bounds, rangeKeys.Bounds)
// nolint:errcheck
return must.Handle(context.Background(), func(ctx context.Context) {
key := iter.UnsafeKey()

// Keys can't be empty.
must.NotEmpty(ctx, key.Key, "empty key")

// Can't be positioned in the lock table.
must.NotPrefixBytes(ctx, key.Key, keys.LocalRangeLockTablePrefix, "MVCC iter in lock table")

// Any valid position must have either a point and/or range key.
hasPoint, hasRange := iter.HasPointAndRange()
if !hasPoint && !hasRange {
// NB: MVCCIncrementalIterator can return hasPoint=false,hasRange=false
// following a NextIgnoringTime() call. We explicitly allow this here.
if incrIter, ok := iter.(*MVCCIncrementalIterator); !ok || !incrIter.ignoringTime {
must.Fail(ctx, "iterator without point/range keys at %s", key)
}
}

// Must have range keys.
if rangeKeys.IsEmpty() {
return errors.AssertionFailedf("hasRange=true but no range key versions at %s", key)
}
// Range key assertions.
if hasRange {
// Must have bounds. The MVCCRangeKey.Validate() call below will make
// further bounds assertions.
bounds := iter.RangeBounds()
must.True(ctx, len(bounds.Key) > 0 || len(bounds.EndKey) > 0,
"hasRange but empty range bounds at %s", key)

// Iterator position must be within range key bounds.
must.True(ctx, bounds.ContainsKey(key.Key),
"iterator position %s outside range key bounds %s", key, bounds)

// Bounds must match range key stack.
rangeKeys := iter.RangeKeys()
must.True(ctx, rangeKeys.Bounds.Equal(bounds),
"range key bounds %s do not match range key %s", bounds, rangeKeys.Bounds)

// Must have range keys.
must.NotEmpty(ctx, rangeKeys.Versions, "hasRange but no range key versions at %s", key)

for i, v := range rangeKeys.Versions {
// Range key must be valid.
rangeKey := rangeKeys.AsRangeKey(v)
must.NoError(ctx, rangeKey.Validate(), "invalid range key at %s", key)

// Range keys must be in descending timestamp order.
if i > 0 {
must.True(ctx, v.Timestamp.Less(rangeKeys.Versions[i-1].Timestamp),
"range key %s not below version %s", rangeKey, rangeKeys.Versions[i-1].Timestamp)
}

for i, v := range rangeKeys.Versions {
// Range key must be valid.
rangeKey := rangeKeys.AsRangeKey(v)
if err := rangeKey.Validate(); err != nil {
return errors.NewAssertionErrorWithWrappedErrf(err, "invalid range key at %s", key)
}
// Range keys must be in descending timestamp order.
if i > 0 && !v.Timestamp.Less(rangeKeys.Versions[i-1].Timestamp) {
return errors.AssertionFailedf("range key %s not below version %s",
rangeKey, rangeKeys.Versions[i-1].Timestamp)
}
// Range keys must currently be tombstones.
if value, err := DecodeMVCCValue(v.Value); err != nil {
return errors.NewAssertionErrorWithWrappedErrf(err, "invalid range key value at %s",
rangeKey)
} else if !value.IsTombstone() {
return errors.AssertionFailedf("non-tombstone range key %s with value %x",
// Range keys must currently be tombstones.
value, err := DecodeMVCCValue(v.Value)
must.NoError(ctx, err, "invalid range key value at %s", rangeKey)
must.True(ctx, value.IsTombstone(), "non-tombstone range key %s with value %x",
rangeKey, value.Value.RawBytes)
}
}

}
if hasPoint {
value, err := iter.UnsafeValue()
if err != nil {
return err
}
valueLen := iter.ValueLen()
if len(value) != valueLen {
return errors.AssertionFailedf("length of UnsafeValue %d != ValueLen %d", len(value), valueLen)
}
if key.IsValue() {
valueLen2, isTombstone, err := iter.MVCCValueLenAndIsTombstone()
if err == nil {
if len(value) != valueLen2 {
return errors.AssertionFailedf("length of UnsafeValue %d != MVCCValueLenAndIsTombstone %d",
len(value), valueLen2)
}
if v, err := DecodeMVCCValue(value); err == nil {
if isTombstone != v.IsTombstone() {
return errors.AssertionFailedf("isTombstone from MVCCValueLenAndIsTombstone %t != MVCCValue.IsTombstone %t",
isTombstone, v.IsTombstone())
if hasPoint {
value, err := iter.UnsafeValue()
must.NoError(ctx, err, "failed to get value for %s", key)
must.Len(ctx, value, iter.ValueLen(), "UnsafeValue != ValueLen at %s", key)
if key.IsValue() {
valueLen, isTombstone, err := iter.MVCCValueLenAndIsTombstone()
if err == nil {
must.Len(ctx, value, valueLen, "UnsafeValue != MVCCValueLenAndIsTombstone at %s", key)
if v, err := DecodeMVCCValue(value); err == nil {
must.Equal(ctx, v.IsTombstone(), isTombstone,
"IsTombstone != MVCCValueLenAndIsTombstone at %s", key)
// Else err != nil. SimpleMVCCIterator is not responsile for data
// corruption since it is possible that the implementation of
// MVCCValueLenAndIsTombstone is fetching information from a
// different part of the store than where the value is stored.
}
// Else err != nil. SimpleMVCCIterator is not responsile for data
// corruption since it is possible that the implementation of
// MVCCValueLenAndIsTombstone is fetching information from a
// different part of the store than where the value is stored.
}
// Else err != nil. Ignore, since SimpleMVCCIterator is not to be held
// responsible for data corruption or tests writing non-MVCCValues.
}
// Else err != nil. Ignore, since SimpleMVCCIterator is not to be held
// responsible for data corruption or tests writing non-MVCCValues.
}
}

return nil
})
}

// assertMVCCIteratorInvariants asserts invariants in the MVCCIterator interface
// that should hold for all implementations, returning errors.AssertionFailedf
// for any violations. It calls through to assertSimpleMVCCIteratorInvariants().
// The iterator must be valid.
// that should hold for all implementations. It calls through to
// assertSimpleMVCCIteratorInvariants(). The iterator must be valid.
func assertMVCCIteratorInvariants(iter MVCCIterator) error {
// Assert SimpleMVCCIterator invariants.
if err := assertSimpleMVCCIteratorInvariants(iter); err != nil {
return err
}

key := iter.UnsafeKey().Clone()

// UnsafeRawMVCCKey must match Key.
if r, err := DecodeMVCCKey(iter.UnsafeRawMVCCKey()); err != nil {
return errors.NewAssertionErrorWithWrappedErrf(
err, "failed to decode UnsafeRawMVCCKey at %s",
key,
)
} else if !r.Equal(key) {
return errors.AssertionFailedf("UnsafeRawMVCCKey %s does not match Key %s", r, key)
}

// UnsafeRawKey must either be an MVCC key matching Key, or a lock table key
// that refers to it.
if engineKey, ok := DecodeEngineKey(iter.UnsafeRawKey()); !ok {
return errors.AssertionFailedf("failed to decode UnsafeRawKey as engine key at %s", key)
} else if engineKey.IsMVCCKey() {
if k, err := engineKey.ToMVCCKey(); err != nil {
return errors.NewAssertionErrorWithWrappedErrf(err, "invalid UnsafeRawKey at %s", key)
} else if !k.Equal(key) {
return errors.AssertionFailedf("UnsafeRawKey %s does not match Key %s", k, key)
}
} else if engineKey.IsLockTableKey() {
if k, err := engineKey.ToLockTableKey(); err != nil {
return errors.NewAssertionErrorWithWrappedErrf(err, "invalid UnsafeRawKey at %s", key)
} else if !k.Key.Equal(key.Key) {
return errors.AssertionFailedf("UnsafeRawKey lock table key %s does not match Key %s", k, key)
} else if !key.Timestamp.IsEmpty() {
return errors.AssertionFailedf(
"UnsafeRawKey lock table key %s for Key %s with non-zero timestamp", k, key,
)
// nolint:errcheck
return must.Handle(context.Background(), func(ctx context.Context) {
key := iter.UnsafeKey().Clone()

// UnsafeRawMVCCKey must match Key.
raw, err := DecodeMVCCKey(iter.UnsafeRawMVCCKey())
must.NoError(ctx, err, "failed to decode raw MVCC key at %s", key)
must.True(ctx, raw.Equal(key), "UnsafeRawMVCCKey %s != Key %s", raw, key)

// UnsafeRawKey must either be an MVCC key matching Key, or a lock table key
// that refers to it.
engineKey, ok := DecodeEngineKey(iter.UnsafeRawKey())
must.True(ctx, ok, "failed to decode engine key at %s", key)
if engineKey.IsMVCCKey() {
mvccKey, err := engineKey.ToMVCCKey()
must.NoError(ctx, err, "MVCC key conversion failed at %s", key)
must.True(ctx, mvccKey.Equal(key), "UnsafeRawKey %s != Key %s", mvccKey, key)
} else if engineKey.IsLockTableKey() {
ltKey, err := engineKey.ToLockTableKey()
must.NoError(ctx, err, "failed to decode lock table key at %s", key)
must.EqualBytes(ctx, ltKey.Key, key.Key, "UnsafeRawKey != Key", ltKey.Key, key.Key)
must.Zero(ctx, key.Timestamp, "lock table key with timestamp at %s", key)
} else {
must.Fail(ctx, "unknown engine key type for %s", engineKey)
}
} else {
return errors.AssertionFailedf("unknown type for engine key %s", engineKey)
}

// Value must equal UnsafeValue.
u, err := iter.UnsafeValue()
if err != nil {
return err
}
v, err := iter.Value()
if err != nil {
return err
}
if !bytes.Equal(v, u) {
return errors.AssertionFailedf("Value %x does not match UnsafeValue %x at %s", v, u, key)
}

// For prefix iterators, any range keys must be point-sized. We've already
// asserted that the range key covers the iterator position.
if iter.IsPrefix() {
if _, hasRange := iter.HasPointAndRange(); hasRange {
if bounds := iter.RangeBounds(); !bounds.EndKey.Equal(bounds.Key.Next()) {
return errors.AssertionFailedf("prefix iterator with wide range key %s", bounds)
// Value must equal UnsafeValue.
u, err := iter.UnsafeValue()
must.NoError(ctx, err, "failed to fetch value")
v, err := iter.Value()
must.NoError(ctx, err, "failed to fetch value")
must.EqualBytes(ctx, v, u, "Value != UnsafeValue at %s", key)

// For prefix iterators, any range keys must be point-sized. We've already
// asserted that the range key covers the iterator position.
if iter.IsPrefix() {
if _, hasRange := iter.HasPointAndRange(); hasRange {
b := iter.RangeBounds()
must.EqualBytes(ctx, b.EndKey, b.Key.Next(), "prefix iterator with wide range key")
}
}
}

return nil
})
}

// ScanConflictingIntentsForDroppingLatchesEarly scans intents using only the
Expand Down
Loading