Skip to content

Commit

Permalink
storage: check for replicated locks in CheckSSTConflicts
Browse files Browse the repository at this point in the history 
Informs cockroachdb#111984.
Informs cockroachdb#111893.
Informs cockroachdb#111530.

This commit changes storage.CheckSSTConflicts to also check for
replicated locks, in addition to checking for intents. This is necessary
to prevent AddSSTable from ingesting key-values pairs below replicated
locks, which could violate the isolation expected from these locks if an
ingested key-value is below the commit timestamp of the lock holder.
This was caught in kvnemesis by the validation logic recently introduced
in 7ff1c79.

The change alters the semantics of CheckSSTConflicts slightly for
intents. It now detects conflicts with any intents in the AddSSTable's
key span, instead of just those on overlapping keys. I think this is ok,
as the `checkConflicts = false` path in `EvalAddSSTable` already had
this behavior. This is reflected in the changes to cmd_add_sstable_test.go,
where three cases are no longer inconsistent with blind writes. I also
don't think this is a performance concern (like the comment that's being
deleted was suggesting) because we now have the separated lock table. In
fact, this change allows us to avoid using an `intentInterleavingIter`,
so if anything, it may provide a small performance win.

Release note: None
  • Loading branch information
@nvanbenschoten
nvanbenschoten committed Oct 26, 2023
1 parent cbbcefb commit ff633ba
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 49 deletions.
18 changes: 9 additions & 9 deletions pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,11 +375,11 @@ func TestEvalAddSSTable(t *testing.T) {
sst: kvs{pointKV("b", 3, "sst")},
expectErr: &kvpb.LockConflictError{},
},
"DisallowConflicts ignores intents in span": { // inconsistent with blind writes
"DisallowConflicts returns LockConflictError in span": {
noConflict: true,
data: kvs{pointKV("b", intentTS, "intent")},
sst: kvs{pointKV("a", 3, "sst"), pointKV("c", 3, "sst")},
expect: kvs{pointKV("a", 3, "sst"), pointKV("b", intentTS, "intent"), pointKV("c", 3, "sst")},
expectErr: &kvpb.LockConflictError{},
},
"DisallowConflicts is not idempotent": {
noConflict: true,
Expand Down Expand Up @@ -479,11 +479,11 @@ func TestEvalAddSSTable(t *testing.T) {
sst: kvs{pointKV("b", 3, "sst")},
expectErr: &kvpb.LockConflictError{},
},
"DisallowShadowing ignores intents in span": { // inconsistent with blind writes
noShadow: true,
data: kvs{pointKV("b", intentTS, "intent")},
sst: kvs{pointKV("a", 3, "sst"), pointKV("c", 3, "sst")},
expect: kvs{pointKV("a", 3, "sst"), pointKV("b", intentTS, "intent"), pointKV("c", 3, "sst")},
"DisallowShadowing returns LockConflictError in span": {
noShadow: true,
data: kvs{pointKV("b", intentTS, "intent")},
sst: kvs{pointKV("a", 3, "sst"), pointKV("c", 3, "sst")},
expectErr: &kvpb.LockConflictError{},
},
"DisallowShadowing is idempotent": {
noShadow: true,
Expand Down Expand Up @@ -607,11 +607,11 @@ func TestEvalAddSSTable(t *testing.T) {
sst: kvs{pointKV("b", 3, "sst")},
expectErr: &kvpb.LockConflictError{},
},
"DisallowShadowingBelow ignores intents in span": { // inconsistent with blind writes
"DisallowShadowingBelow returns LockConflictError in span": {
noShadowBelow: 5,
data: kvs{pointKV("b", intentTS, "intent")},
sst: kvs{pointKV("a", 3, "sst"), pointKV("c", 3, "sst")},
expect: kvs{pointKV("a", 3, "sst"), pointKV("b", intentTS, "intent"), pointKV("c", 3, "sst")},
expectErr: &kvpb.LockConflictError{},
},
"DisallowShadowingBelow is not generally idempotent": {
noShadowBelow: 5,
Expand Down
46 changes: 15 additions & 31 deletions pkg/storage/sst.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ func NewMultiMemSSTIterator(ssts [][]byte, verify bool, opts IterOptions) (MVCCI
// engine contains MVCC range keys in the ingested span then this will cause
// MVCC stats to be estimates since we can't adjust stats for masked points.
//
// The given SST and reader cannot contain intents or inline values (i.e. zero
// timestamps), but this is only checked for keys that exist in both sides, for
// performance.
// The given SST and reader cannot contain intents, replicated locks, or inline
// values (i.e. zero timestamps). This is checked across the entire key span,
// from start to end.
//
// The returned MVCC statistics is a delta between the SST-only statistics and
// their effect when applied, which when added to the SST statistics will adjust
Expand Down Expand Up @@ -147,7 +147,6 @@ func CheckSSTConflicts(
// a seek.
const numNextsBeforeSeek = 5
var statsDiff enginepb.MVCCStats
var intents []roachpb.Intent
if usePrefixSeek {
// If we're going to be using a prefix iterator, check for the fast path
// first, where there are no keys in the reader between the sstable's start
Expand All @@ -168,6 +167,13 @@ func CheckSSTConflicts(
}
}

// Check for any overlapping locks, and return them to be resolved.
if locks, err := ScanLocks(ctx, reader, start.Key, end.Key, maxLockConflicts, 0); err != nil {
return enginepb.MVCCStats{}, err
} else if len(locks) > 0 {
return enginepb.MVCCStats{}, &kvpb.LockConflictError{Locks: locks}
}

// Check for any range keys.
//
// TODO(bilal): Expose reader.Properties.NumRangeKeys() here, so we don't
Expand Down Expand Up @@ -233,7 +239,7 @@ func CheckSSTConflicts(
// https://github.com/cockroachdb/cockroach/issues/92254
statsDiff.ContainsEstimates += 2
}
extIter, err := reader.NewMVCCIterator(ctx, MVCCKeyAndIntentsIterKind, IterOptions{
extIter, err := reader.NewMVCCIterator(ctx, MVCCKeyIterKind, IterOptions{
KeyTypes: IterKeyTypePointsAndRanges,
LowerBound: leftPeekBound,
UpperBound: rightPeekBound,
Expand Down Expand Up @@ -278,20 +284,11 @@ func CheckSSTConflicts(
return err
}
if len(mvccMeta.RawBytes) > 0 {
return errors.New("inline values are unsupported")
return errors.AssertionFailedf("inline values are unsupported")
} else if mvccMeta.Txn == nil {
return errors.New("found intent without transaction")
return errors.AssertionFailedf("found intent without transaction")
} else {
// If we encounter a write intent, keep looking for additional intents
// in order to return a large batch for intent resolution. The caller
// will likely resolve the returned intents and retry the call, which
// would be quadratic, so this significantly reduces the overall number
// of scans.
intents = append(intents, roachpb.MakeIntent(mvccMeta.Txn, extIter.UnsafeKey().Key.Clone()))
if int64(len(intents)) >= maxLockConflicts {
return &kvpb.LockConflictError{Locks: roachpb.AsLocks(intents)}
}
return nil
return errors.AssertionFailedf("found intent after ScanLocks call")
}
}
extValueIsTombstone, err := EncodedMVCCValueIsTombstone(extValueRaw)
Expand Down Expand Up @@ -532,17 +529,7 @@ func CheckSSTConflicts(
return enginepb.MVCCStats{}, err
}
} else {
// extIter is at an intent. Save it to the intents list and Next().
var mvccMeta enginepb.MVCCMetadata
if err = extIter.ValueProto(&mvccMeta); err != nil {
return enginepb.MVCCStats{}, err
}
intents = append(intents, roachpb.MakeIntent(mvccMeta.Txn, extIter.UnsafeKey().Key.Clone()))
if int64(len(intents)) >= maxLockConflicts {
return statsDiff, &kvpb.LockConflictError{Locks: roachpb.AsLocks(intents)}
}
extIter.Next()
continue
return enginepb.MVCCStats{}, errors.AssertionFailedf("found intent after ScanLocks call")
}

if sstBottomTombstone.Timestamp.LessEq(extKey.Timestamp) {
Expand Down Expand Up @@ -1229,9 +1216,6 @@ func CheckSSTConflicts(
if sstErr != nil {
return enginepb.MVCCStats{}, sstErr
}
if len(intents) > 0 {
return enginepb.MVCCStats{}, &kvpb.LockConflictError{Locks: roachpb.AsLocks(intents)}
}

return statsDiff, nil
}
Expand Down
29 changes: 20 additions & 9 deletions pkg/storage/sst_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"testing"

"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
Expand All @@ -35,18 +36,20 @@ func TestCheckSSTConflictsMaxLockConflicts(t *testing.T) {

keys := []string{"aa", "bb", "cc", "dd"}
intents := []string{"a", "b", "c"}
locks := []string{"d", "e"}
start, end := "a", "z"

testcases := []struct {
maxLockConflicts int64
expectIntents []string
maxLockConflicts int64
expectLockConflicts []string
}{
{maxLockConflicts: -1, expectIntents: []string{"a"}},
{maxLockConflicts: 0, expectIntents: []string{"a"}},
{maxLockConflicts: 1, expectIntents: []string{"a"}},
{maxLockConflicts: 2, expectIntents: []string{"a", "b"}},
{maxLockConflicts: 3, expectIntents: []string{"a", "b", "c"}},
{maxLockConflicts: 4, expectIntents: []string{"a", "b", "c"}},
{maxLockConflicts: 0, expectLockConflicts: []string{"a", "b", "c", "d", "e"}}, // 0 means no limit
{maxLockConflicts: 1, expectLockConflicts: []string{"a"}},
{maxLockConflicts: 2, expectLockConflicts: []string{"a", "b"}},
{maxLockConflicts: 3, expectLockConflicts: []string{"a", "b", "c"}},
{maxLockConflicts: 4, expectLockConflicts: []string{"a", "b", "c", "d"}},
{maxLockConflicts: 5, expectLockConflicts: []string{"a", "b", "c", "d", "e"}},
{maxLockConflicts: 6, expectLockConflicts: []string{"a", "b", "c", "d", "e"}},
}

// Create SST with keys equal to intents at txn2TS.
Expand Down Expand Up @@ -81,6 +84,14 @@ func TestCheckSSTConflictsMaxLockConflicts(t *testing.T) {
for _, key := range intents {
require.NoError(t, MVCCPut(ctx, batch, roachpb.Key(key), txn1TS, roachpb.MakeValueFromString("intent"), MVCCWriteOptions{Txn: txn1}))
}
// Also write some replicated locks held by txn1.
for i, key := range locks {
str := lock.Shared
if i%2 != 0 {
str = lock.Exclusive
}
require.NoError(t, MVCCAcquireLock(ctx, batch, txn1, str, roachpb.Key(key), nil, 0))
}
require.NoError(t, batch.Commit(true))
batch.Close()
require.NoError(t, engine.Flush())
Expand All @@ -101,7 +112,7 @@ func TestCheckSSTConflictsMaxLockConflicts(t *testing.T) {
for _, i := range lcErr.Locks {
actual = append(actual, string(i.Key))
}
require.Equal(t, tc.expectIntents, actual)
require.Equal(t, tc.expectLockConflicts, actual)
})
}
})
Expand Down

0 comments on commit ff633ba

Please sign in to comment.