Merge #157105 #157147

craig[bot] · pav-kv · craig[bot] · commit 6471b37f612f · 2025-11-10T19:51:42.000Z
157105: spanset: fix and simplify mergeSpans r=stevendanna a=pav-kv The `mergeSpans` function used for `SortAndDedup` calls on `SpanSet` is not tested and not clear about the semantics when the input slice of `Span@Timestamp` has keys that are present at multiple timestamps. In the latter case, `mergeSpans` does not output a minimal / "canonical" set of spans. This does not look good because the result of this call is fed to the latch manager, i.e. this function is on the critical path for CRDB correctness / performance. Adding a test revealed a bug in this function that could result in placing a wider latch than was requested. This PR fixes this bug, adds tests, clarifies the semantics of and refactors `mergeSpans` (and the `roachpb.MergeSpans` that it is mostly a copy of) into a readable state. Epic: none Release note (bug fix): a bug in key span merging could result in placing wider latches than needed for a request, which could impose unnecessary contention. The effect of this bug hasn't been observed in the wild, and possibly never happened. 157147: roachtest: deflake splits/load/ycsb/e r=pav-kv a=pav-kv Resolves #156923 Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
diff --git a/pkg/backup/backup_job.go b/pkg/backup/backup_job.go
@@ -1037,7 +1037,7 @@ func spansForAllTableIndexes(
 	// Attempt to merge any contiguous spans generated from the tables and revs.
 	// No need to check if the spans are distinct, since some of the merged
 	// indexes may overlap between different revisions of the same descriptor.
-	mergedSpans, _ := roachpb.MergeSpans(&spans)
+	mergedSpans, _ := roachpb.MergeSpans(spans)
 
 	knobs := execCfg.BackupRestoreTestingKnobs
 	if knobs != nil && knobs.CaptureResolvedTableDescSpans != nil {
diff --git a/pkg/backup/restore_span_covering_test.go b/pkg/backup/restore_span_covering_test.go
@@ -797,7 +797,7 @@ func runTestRestoreEntryCoverForSpanAndFileCounts(
 			completedSpans = append(completedSpans, sp)
 		}
 
-		merged, _ := roachpb.MergeSpans(&completedSpans)
+		merged, _ := roachpb.MergeSpans(completedSpans)
 		return merged
 	}
 
diff --git a/pkg/backup/utils_test.go b/pkg/backup/utils_test.go
@@ -239,7 +239,7 @@ func getSpansFromManifest(ctx context.Context, t *testing.T, backupPath string)
 	for _, file := range backupManifest.Files {
 		spans = append(spans, file.Span)
 	}
-	mergedSpans, _ := roachpb.MergeSpans(&spans)
+	mergedSpans, _ := roachpb.MergeSpans(spans)
 	return mergedSpans
 }
 
diff --git a/pkg/cmd/roachtest/tests/split.go b/pkg/cmd/roachtest/tests/split.go
@@ -413,7 +413,7 @@ func registerLoadSplits(r registry.Registry) {
 				// YCSB/E has a zipfian distribution with 95% scans (limit 1k) and 5%
 				// inserts.
 				minimumRanges:     5,
-				maximumRanges:     32,
+				maximumRanges:     33,
 				initialRangeCount: 2,
 				load: ycsbSplitLoad{
 					workload:     "e",
diff --git a/pkg/kv/kvclient/kvcoord/condensable_span_set.go b/pkg/kv/kvclient/kvcoord/condensable_span_set.go
@@ -63,7 +63,7 @@ func (s *condensableSpanSet) insert(spans ...roachpb.Span) {
 // The method has the side effect of sorting the set.
 func (s *condensableSpanSet) mergeAndSort() {
 	oldLen := len(s.s)
-	s.s, _ = roachpb.MergeSpans(&s.s)
+	s.s, _ = roachpb.MergeSpans(s.s)
 	// Recompute the size if anything has changed.
 	if oldLen != len(s.s) {
 		s.bytes = 0
@@ -222,7 +222,7 @@ func (s *condensableSpanSet) estimateSize(spans []roachpb.Span, mergeThresholdBy
 	// Try harder - merge (a copy of) the existing spans with the new spans.
 	spans = append(spans, s.s...)
 	lenBeforeMerge := len(spans)
-	spans, _ = roachpb.MergeSpans(&spans)
+	spans, _ = roachpb.MergeSpans(spans)
 	if len(spans) == lenBeforeMerge {
 		// Nothing changed -i.e. we failed to merge any spans.
 		return estimate
diff --git a/pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go b/pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go
@@ -404,7 +404,7 @@ func mergeIntoSpans(s []roachpb.Span, ws []roachpb.SequencedWrite) ([]roachpb.Sp
 	for i, w := range ws {
 		m[len(s)+i] = roachpb.Span{Key: w.Key}
 	}
-	return roachpb.MergeSpans(&m)
+	return roachpb.MergeSpans(m)
 }
 
 // isTxnCommitImplicit determines whether the transaction has satisfied the
diff --git a/pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go b/pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go
@@ -485,7 +485,7 @@ func (tp *txnPipeliner) attachLocksToEndTxn(
 	}
 
 	// Sort both sets and condense the lock spans.
-	et.LockSpans, _ = roachpb.MergeSpans(&et.LockSpans)
+	et.LockSpans, _ = roachpb.MergeSpans(et.LockSpans)
 	sort.Sort(roachpb.SequencedWriteBySeq(et.InFlightWrites))
 
 	if log.V(3) {
diff --git a/pkg/kv/kvserver/batcheval/cmd_recover_txn.go b/pkg/kv/kvserver/batcheval/cmd_recover_txn.go
@@ -204,7 +204,7 @@ func RecoverTxn(
 		sp := roachpb.Span{Key: w.Key}
 		reply.RecoveredTxn.LockSpans = append(reply.RecoveredTxn.LockSpans, sp)
 	}
-	reply.RecoveredTxn.LockSpans, _ = roachpb.MergeSpans(&reply.RecoveredTxn.LockSpans)
+	reply.RecoveredTxn.LockSpans, _ = roachpb.MergeSpans(reply.RecoveredTxn.LockSpans)
 	reply.RecoveredTxn.InFlightWrites = nil
 
 	// Recover the transaction based on whether or not all of its writes
diff --git a/pkg/kv/kvserver/lockspanset/lockspanset.go b/pkg/kv/kvserver/lockspanset/lockspanset.go
@@ -42,7 +42,7 @@ func (l *LockSpanSet) Add(str lock.Strength, span roachpb.Span) {
 // SortAndDeDup sorts the spans in the LockSpanSet and removes any duplicates.
 func (l *LockSpanSet) SortAndDeDup() {
 	for st := range l.spans {
-		l.spans[st], _ /* distinct */ = roachpb.MergeSpans(&l.spans[st])
+		l.spans[st], _ /* distinct */ = roachpb.MergeSpans(l.spans[st])
 	}
 }
 
diff --git a/pkg/kv/kvserver/replica_batch_updates.go b/pkg/kv/kvserver/replica_batch_updates.go
@@ -92,7 +92,7 @@ func maybeStripInFlightWrites(ba *kvpb.BatchRequest) (*kvpb.BatchRequest, error)
 				et.LockSpans[len(origET.LockSpans)+i] = roachpb.Span{Key: w.Key}
 			}
 			// See below for why we set Header.DistinctSpans here.
-			et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(&et.LockSpans)
+			et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(et.LockSpans)
 			return ba, nil
 		}
 	}
@@ -164,7 +164,7 @@ func maybeStripInFlightWrites(ba *kvpb.BatchRequest) (*kvpb.BatchRequest, error)
 		// batch overlap with each other. This will have (rare) false negatives
 		// when the in-flight writes overlap with existing lock spans, but never
 		// false positives.
-		et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(&et.LockSpans)
+		et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(et.LockSpans)
 	}
 	return ba, nil
 }
diff --git a/pkg/kv/kvserver/spanset/BUILD.bazel b/pkg/kv/kvserver/spanset/BUILD.bazel
@@ -29,6 +29,7 @@ go_test(
     size = "small",
     srcs = [
         "batch_test.go",
+        "merge_test.go",
         "spanset_test.go",
     ],
     embed = [":spanset"],
diff --git a/pkg/kv/kvserver/spanset/merge.go b/pkg/kv/kvserver/spanset/merge.go
@@ -5,114 +5,74 @@
 
 package spanset
 
-import "sort"
+import "slices"
 
-type sortedSpans []Span
-
-func (s *sortedSpans) Less(i, j int) bool {
-	// Sort first on the start key and second on the end key. Note that we're
-	// relying on EndKey = nil (and len(EndKey) == 0) sorting before other
-	// EndKeys.
-	c := (*s)[i].Key.Compare((*s)[j].Key)
-	if c != 0 {
-		return c < 0
-	}
-	return (*s)[i].EndKey.Compare((*s)[j].EndKey) < 0
-}
-
-func (s *sortedSpans) Swap(i, j int) {
-	(*s)[i], (*s)[j] = (*s)[j], (*s)[i]
-}
-
-func (s *sortedSpans) Len() int {
-	return len(*s)
-}
-
-// mergeSpans sorts the given spans and merges ones with overlapping
-// spans and equal access timestamps. The implementation is a copy of
-// roachpb.MergeSpans.
+// mergeSpans sorts the given spans and merges ones with overlapping spans and
+// equal access timestamps. The implementation is a modified roachpb.MergeSpans.
+//
+// The input and output spans represent the same subset of {keys}x{timestamps}
+// space. The resulting spans are ordered by key. It is minimal (impossible to
+// "merge" further without losing precision) only if there are no keys present
+// under multiple timestamps.
+//
+// When there are keys present under multiple timestamps, the resulting merged
+// set is not guaranteed to be minimal. For example, [{a-b}@10, b@20, {b-c}@10]
+// is returned as is, even though we could merge it as [{a-c}@10, b@20]. This is
+// due to an arbitrary choice of sorting it by key.
+//
+// TODO(pav-kv): we could sort by (timestamp, key), which makes it possible to
+// merge spans with the same timestamp and guarantee minimality. After which, we
+// can optionally sort by key again. This is still more expensive than one sort,
+// so consider if the users are fine with the set sorted by (timestamp, key).
+//
+// The priority for this function is to be simple and free from assumptions
+// about how to "resolve" the situation of having the same key under multiple
+// timestamps. E.g. it does not assume that higher/lower timestamp "wins", or
+// what the semantics of an empty timestamp are. Considerations like that are
+// left to the consumer of this span set, e.g. the latch manager.
 //
-// Returns true iff all of the spans are distinct.
 // The input spans are not safe for re-use.
-func mergeSpans(latches *[]Span) ([]Span, bool) {
-	if len(*latches) == 0 {
-		return *latches, true
+func mergeSpans(latches []Span) []Span {
+	if len(latches) == 0 {
+		return latches
 	}
 
-	sort.Sort((*sortedSpans)(latches))
+	// Sort first on the start key and second on the end key. Note that we're
+	// relying on empty EndKey sorting before other EndKeys.
+	slices.SortFunc(latches, func(s1, s2 Span) int {
+		if c := s1.Key.Compare(s2.Key); c != 0 {
+			return c
+		}
+		return s1.EndKey.Compare(s2.EndKey)
+	})
 
 	// We build up the resulting slice of merged spans in place. This is safe
 	// because "r" grows by at most 1 element on each iteration, staying abreast
 	// or behind the iteration over "latches".
-	r := (*latches)[:1]
-	distinct := true
+	r := latches[:1]
 
-	for _, cur := range (*latches)[1:] {
+	for _, cur := range latches[1:] {
 		prev := &r[len(r)-1]
-		if len(cur.EndKey) == 0 && len(prev.EndKey) == 0 {
-			if cur.Key.Compare(prev.Key) != 0 {
-				// [a, nil] merge [b, nil]
-				r = append(r, cur)
-			} else {
-				// [a, nil] merge [a, nil]
-				if cur.Timestamp != prev.Timestamp {
-					r = append(r, cur)
-				}
-				distinct = false
+		// Can only merge spans at the same timestamp.
+		if cur.Timestamp != prev.Timestamp {
+			r = append(r, cur)
+		} else if len(prev.EndKey) == 0 { // prev is a point key
+			if !cur.Key.Equal(prev.Key) { // cur.Key > prev.Key
+				r = append(r, cur) // [a] + [b,any) = [a], [b,any)
+			} else { // [a] + [a,any) = [a,any)
+				prev.EndKey = cur.EndKey // cur.EndKey >= prev.EndKey
 			}
-			continue
-		}
-		if len(prev.EndKey) == 0 {
-			if cur.Key.Compare(prev.Key) == 0 {
-				// [a, nil] merge [a, b]
-				if cur.Timestamp != prev.Timestamp {
-					r = append(r, cur)
-				} else {
-					prev.EndKey = cur.EndKey
-				}
-				distinct = false
-			} else {
-				// [a, nil] merge [b, c]
-				r = append(r, cur)
-			}
-			continue
-		}
-
-		if c := prev.EndKey.Compare(cur.Key); c >= 0 {
-			if cur.EndKey != nil {
-				if prev.EndKey.Compare(cur.EndKey) < 0 {
-					// [a, c] merge [b, d]
-					if cur.Timestamp != prev.Timestamp {
-						r = append(r, cur)
-					} else {
-						prev.EndKey = cur.EndKey
-					}
-					if c > 0 {
-						distinct = false
-					}
-				} else {
-					// [a, c] merge [b, c]
-					if cur.Timestamp != prev.Timestamp {
-						r = append(r, cur)
-					}
-					distinct = false
-				}
-			} else if c == 0 {
-				// [a, b] merge [b, nil]
-				if cur.Timestamp != prev.Timestamp {
-					r = append(r, cur)
-				}
-				prev.EndKey = cur.Key.Next()
-			} else {
-				// [a, c] merge [b, nil]
-				if cur.Timestamp != prev.Timestamp {
-					r = append(r, cur)
-				}
-				distinct = false
+		} else if c := cur.Key.Compare(prev.EndKey); c > 0 {
+			r = append(r, cur) // [a,b) + [c,any) = [a,b), [c,any)
+		} else if c == 0 {
+			if len(cur.EndKey) == 0 { // cur is a point key
+				prev.EndKey = cur.Key.Next() // [a,b) + [b] = [a,b.Next())
+			} else if cur.EndKey.Compare(prev.EndKey) > 0 {
+				prev.EndKey = cur.EndKey // [a,b) + [b,c) = [a,c)
 			}
-			continue
+		} else if len(cur.EndKey) != 0 && cur.EndKey.Compare(prev.EndKey) > 0 {
+			prev.EndKey = cur.EndKey // [a,c) + [b,d) = [a,d)
 		}
-		r = append(r, cur)
 	}
-	return r, distinct
+	return r
 }
diff --git a/pkg/kv/kvserver/spanset/merge_test.go b/pkg/kv/kvserver/spanset/merge_test.go
@@ -0,0 +1,88 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package spanset
+
+import (
+	"strconv"
+	"strings"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMergeSpans(t *testing.T) {
+	for _, tc := range []struct {
+		spans    string
+		expected string
+	}{
+		{"", ""},
+		{"a", "a"},
+		{"a,b", "a,b"},
+		{"b,a", "a,b"},
+		{"a,a", "a"},
+		{"a-b", "a-b"},
+		{"a-b,b-c", "a-c"},
+		{"a-c,a-b", "a-c"},
+		{"a,b-c", "a,b-c"},
+		{"a,a-c", "a-c"},
+		{"a-c,b", "a-c"},
+		{"a-c,c", "a-c\x00"},
+		{"a-c,b-bb", "a-c"},
+		{"a-c,b-c", "a-c"},
+		{"a-c,b-d", "a-d"},
+
+		{"a-b,b-b\x00", "a-b\x00"},
+		{"b,b\x00", "b,b\x00"},     // TODO(pav-kv): should merge here too
+		{"b,b\x00-c", "b,b\x00-c"}, // TODO(pav-kv): should merge here too
+
+		{"a@10", "a@10"},
+		{"a@10,b@10", "a@10,b@10"},
+		{"a@10,b@20", "a@10,b@20"},
+		{"b@20,a@10", "a@10,b@20"},
+		{"a@10,a-b@10", "a-b@10"},
+		{"a@10,a-b@20", "a@10,a-b@20"},
+		{"a-b@20,b@20", "a-b\x00@20"},
+		{"a-b@20,b@30", "a-b@20,b@30"},
+		{"a-c@20,m-n@10,c-o@10,o-z@20", "a-c@20,c-o@10,o-z@20"},
+	} {
+		t.Run("", func(t *testing.T) {
+			spans := mergeSpans(makeSpans(t, tc.spans))
+			expected := makeSpans(t, tc.expected)
+			require.Equal(t, expected, spans, tc.spans)
+		})
+	}
+}
+
+func makeSpan(t *testing.T, s string) Span {
+	var ts hlc.Timestamp
+	if idx := strings.IndexByte(s, '@'); idx != -1 {
+		i, err := strconv.ParseInt(s[idx+1:], 10, 63)
+		require.NoError(t, err)
+		ts = hlc.Timestamp{WallTime: i}
+		s = s[:idx]
+	}
+	parts := strings.Split(s, "-")
+	if len(parts) != 2 {
+		return Span{Span: roachpb.Span{Key: roachpb.Key(s)}, Timestamp: ts}
+	}
+	return Span{
+		Span: roachpb.Span{
+			Key:    roachpb.Key(parts[0]),
+			EndKey: roachpb.Key(parts[1]),
+		},
+		Timestamp: ts,
+	}
+}
+
+func makeSpans(t *testing.T, s string) []Span {
+	var spans []Span
+	for _, p := range strings.Split(s, ",") {
+		spans = append(spans, makeSpan(t, p))
+	}
+	return spans
+}
diff --git a/pkg/kv/kvserver/spanset/spanset.go b/pkg/kv/kvserver/spanset/spanset.go
diff --git a/pkg/roachpb/merge_spans.go b/pkg/roachpb/merge_spans.go
diff --git a/pkg/roachpb/merge_spans_test.go b/pkg/roachpb/merge_spans_test.go
diff --git a/pkg/storage/metamorphic/operations.go b/pkg/storage/metamorphic/operations.go

Original file line number	Diff line number	Diff line change
`@@ -797,7 +797,7 @@ func runTestRestoreEntryCoverForSpanAndFileCounts(`
`797`	`797`	`completedSpans = append(completedSpans, sp)`
`798`	`798`	`}`
`799`	`799`
`800`		`- merged, _ := roachpb.MergeSpans(&completedSpans)`
	`800`	`+ merged, _ := roachpb.MergeSpans(completedSpans)`
`801`	`801`	`return merged`
`802`	`802`	`}`
`803`	`803`
Original file line number	Diff line number	Diff line change
`@@ -239,7 +239,7 @@ func getSpansFromManifest(ctx context.Context, t *testing.T, backupPath string)`
`239`	`239`	`for _, file := range backupManifest.Files {`
`240`	`240`	`spans = append(spans, file.Span)`
`241`	`241`	`}`
`242`		`- mergedSpans, _ := roachpb.MergeSpans(&spans)`
	`242`	`+ mergedSpans, _ := roachpb.MergeSpans(spans)`
`243`	`243`	`return mergedSpans`
`244`	`244`	`}`
`245`	`245`
Original file line number	Diff line number	Diff line change
`@@ -404,7 +404,7 @@ func mergeIntoSpans(s []roachpb.Span, ws []roachpb.SequencedWrite) ([]roachpb.Sp`
`404`	`404`	`for i, w := range ws {`
`405`	`405`	`m[len(s)+i] = roachpb.Span{Key: w.Key}`
`406`	`406`	`}`
`407`		`- return roachpb.MergeSpans(&m)`
	`407`	`+ return roachpb.MergeSpans(m)`
`408`	`408`	`}`
`409`	`409`
`410`	`410`	`// isTxnCommitImplicit determines whether the transaction has satisfied the`
Original file line number	Diff line number	Diff line change
`@@ -485,7 +485,7 @@ func (tp *txnPipeliner) attachLocksToEndTxn(`
`485`	`485`	`}`
`486`	`486`
`487`	`487`	`// Sort both sets and condense the lock spans.`
`488`		`- et.LockSpans, _ = roachpb.MergeSpans(&et.LockSpans)`
	`488`	`+ et.LockSpans, _ = roachpb.MergeSpans(et.LockSpans)`
`489`	`489`	`sort.Sort(roachpb.SequencedWriteBySeq(et.InFlightWrites))`
`490`	`490`
`491`	`491`	`if log.V(3) {`
Original file line number	Diff line number	Diff line change
`@@ -204,7 +204,7 @@ func RecoverTxn(`
`204`	`204`	`sp := roachpb.Span{Key: w.Key}`
`205`	`205`	`reply.RecoveredTxn.LockSpans = append(reply.RecoveredTxn.LockSpans, sp)`
`206`	`206`	`}`
`207`		`- reply.RecoveredTxn.LockSpans, _ = roachpb.MergeSpans(&reply.RecoveredTxn.LockSpans)`
	`207`	`+ reply.RecoveredTxn.LockSpans, _ = roachpb.MergeSpans(reply.RecoveredTxn.LockSpans)`
`208`	`208`	`reply.RecoveredTxn.InFlightWrites = nil`
`209`	`209`
`210`	`210`	`// Recover the transaction based on whether or not all of its writes`
Original file line number	Diff line number	Diff line change
`@@ -42,7 +42,7 @@ func (l *LockSpanSet) Add(str lock.Strength, span roachpb.Span) {`
`42`	`42`	`// SortAndDeDup sorts the spans in the LockSpanSet and removes any duplicates.`
`43`	`43`	`func (l *LockSpanSet) SortAndDeDup() {`
`44`	`44`	`for st := range l.spans {`
`45`		`- l.spans[st], _ /* distinct */ = roachpb.MergeSpans(&l.spans[st])`
	`45`	`+ l.spans[st], _ /* distinct */ = roachpb.MergeSpans(l.spans[st])`
`46`	`46`	`}`
`47`	`47`	`}`
`48`	`48`
Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ func maybeStripInFlightWrites(ba kvpb.BatchRequest) (kvpb.BatchRequest, error)`
`92`	`92`	`et.LockSpans[len(origET.LockSpans)+i] = roachpb.Span{Key: w.Key}`
`93`	`93`	`}`
`94`	`94`	`// See below for why we set Header.DistinctSpans here.`
`95`		`- et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(&et.LockSpans)`
	`95`	`+ et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(et.LockSpans)`
`96`	`96`	`return ba, nil`
`97`	`97`	`}`
`98`	`98`	`}`
`@@ -164,7 +164,7 @@ func maybeStripInFlightWrites(ba kvpb.BatchRequest) (kvpb.BatchRequest, error)`
`164`	`164`	`// batch overlap with each other. This will have (rare) false negatives`
`165`	`165`	`// when the in-flight writes overlap with existing lock spans, but never`
`166`	`166`	`// false positives.`
`167`		`- et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(&et.LockSpans)`
	`167`	`+ et.LockSpans, ba.Header.DistinctSpans = roachpb.MergeSpans(et.LockSpans)`
`168`	`168`	`}`
`169`	`169`	`return ba, nil`
`170`	`170`	`}`