diff --git a/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go b/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go index d7219fb34467..65b687ca19e6 100644 --- a/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go @@ -808,6 +808,56 @@ func TestEvalAddSSTable(t *testing.T) { sst: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8")}, expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d")}, }, + "DisallowConflicts allows sst range keys 2": { + noConflict: true, + reqTS: 10, + data: kvs{pointKV("a", 6, "d"), pointKV("bbb", 6, ""), pointKV("c", 6, "d"), pointKV("d", 6, "d"), pointKV("dd", 6, "")}, + sst: kvs{pointKV("aa", 9, ""), pointKV("b", 9, "dee"), rangeKV("bb", "e", 9, ""), pointKV("cc", 10, "")}, + expect: kvs{pointKV("a", 6, "d"), pointKV("aa", 9, ""), pointKV("b", 9, "dee"), rangeKV("bb", "e", 9, ""), pointKV("bbb", 6, ""), pointKV("c", 6, "d"), pointKV("cc", 10, ""), pointKV("d", 6, "d"), pointKV("dd", 6, "")}, + }, + "DisallowConflicts does not skip sst range keys ahead of first one": { + noConflict: true, + reqTS: 10, + toReqTS: 8, + data: kvs{pointKV("a", 6, "d"), pointKV("e", 5, "d")}, + sst: kvs{rangeKV("b", "c", 8, ""), rangeKV("cc", "f", 8, "")}, + expect: kvs{pointKV("a", 6, "d"), rangeKV("b", "c", 10, ""), rangeKV("cc", "f", 10, ""), pointKV("e", 5, "d")}, + }, + "DisallowConflicts correctly accounts for complex fragment cases": { + noConflict: true, + reqTS: 10, + data: kvs{rangeKV("b", "c", 7, ""), rangeKV("b", "c", 6, ""), rangeKV("c", "f", 6, ""), rangeKV("f", "g", 7, ""), rangeKV("f", "g", 6, "")}, + sst: kvs{rangeKV("a", "d", 8, ""), rangeKV("e", "g", 8, "")}, + expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 7, ""), rangeKV("b", "c", 6, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 6, ""), rangeKV("d", "e", 6, ""), rangeKV("e", "f", 8, ""), rangeKV("e", "f", 6, ""), rangeKV("f", "g", 8, ""), rangeKV("f", "g", 7, ""), rangeKV("f", "g", 6, "")}, + }, + "DisallowConflicts correctly accounts for complex fragment cases 2": { + noConflict: true, + reqTS: 10, + data: kvs{rangeKV("a", "g", 6, ""), pointKV("bb", 5, "bar")}, + sst: kvs{rangeKV("b", "c", 8, ""), pointKV("cc", 8, "foo"), rangeKV("e", "f", 8, "")}, + expect: kvs{rangeKV("a", "b", 6, ""), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 6, ""), pointKV("bb", 5, "bar"), rangeKV("c", "e", 6, ""), pointKV("cc", 8, "foo"), rangeKV("e", "f", 8, ""), rangeKV("e", "f", 6, ""), rangeKV("f", "g", 6, "")}, + }, + "DisallowConflicts correctly accounts for complex fragment cases 3": { + noConflict: true, + reqTS: 10, + data: kvs{rangeKV("c", "d", 6, ""), pointKV("e", 6, ""), pointKV("e", 5, "foo"), pointKV("f", 6, ""), pointKV("f", 5, ""), pointKV("g", 6, ""), rangeKV("j", "k", 5, "")}, + sst: kvs{rangeKV("a", "l", 8, "")}, + expect: kvs{rangeKV("a", "c", 8, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 6, ""), rangeKV("d", "j", 8, ""), pointKV("e", 6, ""), pointKV("e", 5, "foo"), pointKV("f", 6, ""), pointKV("f", 5, ""), pointKV("g", 6, ""), rangeKV("j", "k", 8, ""), rangeKV("j", "k", 5, ""), rangeKV("k", "l", 8, "")}, + }, + "DisallowConflicts correctly accounts for complex fragment cases 4": { + noConflict: true, + reqTS: 10, + data: kvs{rangeKV("c", "d", 6, ""), rangeKV("j", "k", 5, "")}, + sst: kvs{rangeKV("a", "l", 8, "")}, + expect: kvs{rangeKV("a", "c", 8, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 6, ""), rangeKV("d", "j", 8, ""), rangeKV("j", "k", 8, ""), rangeKV("j", "k", 5, ""), rangeKV("k", "l", 8, "")}, + }, + "DisallowConflicts accounts for point key already deleted in engine": { + noConflict: true, + reqTS: 10, + data: kvs{rangeKV("b", "d", 6, ""), pointKV("c", 5, "foo")}, + sst: kvs{rangeKV("a", "d", 8, "")}, + expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("b", "d", 8, ""), rangeKV("b", "d", 6, ""), pointKV("c", 5, "foo")}, + }, "DisallowConflicts allows fragmented sst range keys": { noConflict: true, data: kvs{pointKV("a", 6, "d")}, diff --git a/pkg/storage/sst.go b/pkg/storage/sst.go index 9b7e0df9d1a2..e09f87939e98 100644 --- a/pkg/storage/sst.go +++ b/pkg/storage/sst.go @@ -217,7 +217,7 @@ func CheckSSTConflicts( // // TODO(bilal): Close this gap in GCBytesAge calculation, see: // https://github.com/cockroachdb/cockroach/issues/92254 - statsDiff.ContainsEstimates++ + statsDiff.ContainsEstimates += 2 } extIter := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, @@ -704,24 +704,6 @@ func CheckSSTConflicts( sstPrevRangeKeys = sstRangeKeys.Clone() } if extRangeKeysChanged { - if !extPrevRangeKeys.IsEmpty() && extPrevRangeKeys.Bounds.EndKey.Compare(extRangeKeys.Bounds.Key) < 0 && - sstHasRange && sstRangeKeys.Bounds.Key.Compare(extRangeKeys.Bounds.Key) < 0 && - sstRangeKeys.Bounds.Overlaps(extRangeKeys.Bounds) { - // We're adding a fragment between two non-abutting engine range keys. - startKey := extPrevRangeKeys.Bounds.EndKey - if sstRangeKeys.Bounds.Key.Compare(startKey) > 0 { - startKey = sstRangeKeys.Bounds.Key - } - endKey := extRangeKeys.Bounds.Key - newRangeKeyStack := MVCCRangeKeyStack{ - Versions: sstRangeKeys.Versions, - Bounds: roachpb.Span{Key: startKey, EndKey: endKey}, - } - statsDiff.Add(updateStatsOnRangeKeyPut(newRangeKeyStack)) - if extPrevRangeKeys.CanMergeRight(newRangeKeyStack) { - statsDiff.Add(updateStatsOnRangeKeyMerge(startKey, newRangeKeyStack.Versions)) - } - } // Note that we exclude sstRangeKeysChanged below, as this case only // accounts for additional ext range keys that this SST range key stack // could be adding versions to. The very first ext range key stack that @@ -877,7 +859,7 @@ func CheckSSTConflicts( if err := compareForCollision(sstKey, extKey, sstValueRaw, extValueRaw); err != nil { return enginepb.MVCCStats{}, err } - } else if extValueDeletedByRange { + } else if sstHasPoint && extValueDeletedByRange { // Don't double-count the current key. var deletedAt hlc.Timestamp if _, isTombstone, err := extIter.MVCCValueLenAndIsTombstone(); err != nil { @@ -1053,10 +1035,19 @@ func CheckSSTConflicts( if sstErr != nil { return enginepb.MVCCStats{}, sstErr } - if sstOK && !sstIter.UnsafeKey().Key.Equal(oldKey.Key) { - // sstIter stepped one key ahead. Re-seek extIter at this key. - extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key}) + if sstOK && !sstIter.UnsafeKey().Key.Equal(oldKey.Key) && extIter.UnsafeKey().Key.Compare(sstIter.UnsafeKey().Key) < 0 { + // sstIter stepped one key ahead. Re-seek both iterators at the next + // ext key. + extIter.NextKey() extOK, extErr = extIter.Valid() + if extOK { + sstIter.SeekGE(MVCCKey{Key: extIter.UnsafeKey().Key}) + } + sstOK, sstErr = sstIter.Valid() + if sstOK { + extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key}) + extOK, extErr = extIter.Valid() + } } // If sstIter found a point key at the same MVCC Key, we still need // to check for conflicts against it. @@ -1068,24 +1059,28 @@ func CheckSSTConflicts( // this logic does not guarantee forward progress in those cases. sstIter.Next() sstOK, sstErr = sstIter.Valid() - extIter.Next() - steppedExtIter = true - extOK, extErr = extIter.Valid() sstChangedKeys := !sstOK || !sstIter.UnsafeKey().Key.Equal(oldKey.Key) - extChangedKeys := !extOK || !extIter.UnsafeKey().Key.Equal(oldKey.Key) - if sstChangedKeys && !extChangedKeys { - sstIter.SeekGE(MVCCKey{Key: extIter.UnsafeKey().Key}) - sstOK, sstErr = sstIter.Valid() - } - // Re-seek the ext iterator if the ext iterator changed keys and: - // 1) the SST iterator did not change keys, and we need to bring the ext - // iterator back. - // 2) the ext iterator became invalid - // 3) both iterators changed keys and the sst iterator's key is further - // ahead. - if sstOK && extChangedKeys && (!sstChangedKeys || !extOK || extIter.UnsafeKey().Key.Compare(sstIter.UnsafeKey().Key) < 0) { + if sstOK && sstChangedKeys { extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key}) extOK, extErr = extIter.Valid() + } else { + extIter.Next() + steppedExtIter = true + extOK, extErr = extIter.Valid() + extChangedKeys := !extOK || !extIter.UnsafeKey().Key.Equal(oldKey.Key) + if sstChangedKeys && !extChangedKeys { + sstIter.SeekGE(MVCCKey{Key: extIter.UnsafeKey().Key}) + sstOK, sstErr = sstIter.Valid() + } + // Re-seek the ext iterator if the ext iterator changed keys and: + // 1) the SST iterator did not change keys, and we need to bring the ext + // iterator back. + // 2) the ext iterator became invalid + // 3) both iterators changed keys. + if sstOK && extChangedKeys { + extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key}) + extOK, extErr = extIter.Valid() + } } // If both iterators are invalid, we are now done. If one both iterators // are at point keys under the same MVCC key, then we can check for @@ -1107,6 +1102,7 @@ func CheckSSTConflicts( // This SeekGE is purely to maintain the extIter > sstIter invariant // as in most cases it'll be a no-op. extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key}) + extOK, extErr = extIter.Valid() } } }