spanset: fix and simplify mergeSpans #157105

pav-kv · 2025-11-09T01:10:34Z

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.

Epic: none Release note: none

cockroach-teamcity · 2025-11-09T01:11:02Z

This change is

pav-kv · 2025-11-09T14:36:32Z

pkg/kv/kvserver/spanset/merge.go

+		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


So this func goes an extra mile to merge [a,b) + [b] = [a,b] (see below). However, it doesn't handle cases like two consecutive point keys:

[a] + [a.Next()] = [a,a.Next().Next())

I wonder if we should do it for completeness. Today:

[a,b) + [b] + [b.Next()] = [a,b.Next().Next()) [b] + [b.Next()] = [b] + [b.Next()]

I have a commit that fixes it, but I'll send it separately after this PR merges. As is, the PR does not change the behaviour of mergeSpans.

Epic: none Release note: none

It was passed by pointer to avoid allocation when converted to sort.Interface, but it's no longer needed with slices.SortFunc. Epic: none Release note: none

Epic: none Release note: none

stevendanna

Overall I like this cleanup.

For the commits with "simplify" I've mostly tried to just verify that I was able to follow the logic without thinking too hard and I think this accomplishes that.

One thing I've seen us do elsewhere is list out the cases in a block comment above where it is a bit easier to spell them out with labels:

// CASE 1: [a] + [b, any)    = [a], [b, any)
// CASE 2: [a] + [a, any)    = [a, any)
// CASE 3: [a, c) + [d, any) = [a,c], [d,any)
// CASE 4: [a, c) + [c] = [a, c.Next())
// CASE 5: [a, c) + [c, d) = [a, d)
// CASE 6: [a, c) + [b, d) = [a, d)
// CASE 7: [a, c) + [b-bb) = [a, c)

And then inline refer to the case labels.

Not recommending this as I think your inline comments made it straightforward to follow. Just noting it for your consideration in case you prefer it.

pkg/kv/kvserver/spanset/merge_test.go

stevendanna · 2025-11-10T11:28:02Z

pkg/kv/kvserver/spanset/spanset.go

+// TODO(pav-kv): does this make CheckAllowed falsely fail in some cases? Maybe
+// it's fine: importantly, it should not falsely succeed.


Do you have an example of what you are considering here?

Something similar to the concern under TODO(irfansharif) below. Say there are 2 spans [a-b)@10 [b-c)@10 that haven't merged due to mergeSpans best-effortness. If there is a read of span [a-c), CheckAllowed can fail due to not finding any whole span that includes [a-c), even though it's technically allowed here.

I'll address this as part of TODO(pav-kv) next to mergeSpans saying that we can sort by (timestamp, key) and achieve "canonicalization" of this set. Seemingly, the callers don't rely on this being sorted by key (and I also confirmed that by running all kvserver tests locally with this change).

pkg/roachpb/merge_spans_test.go

Epic: none Release note: none

All branches in this loop check timestamp at the end. This was unnecessarily verbose, error-prone, and led to a bug fixed in the previous commit. Check the timestamp at the beginning once instead. Epic: none Release note: none

Epic: none Release note: none

pav-kv

TFTR!

@stevendanna the CASE comments look nice, I've adopted the "notation" from it in the inlined comments.

pkg/kv/kvserver/spanset/merge_test.go

pkg/roachpb/merge_spans_test.go

pav-kv · 2025-11-10T14:05:09Z

pkg/kv/kvserver/spanset/spanset.go

+// TODO(pav-kv): does this make CheckAllowed falsely fail in some cases? Maybe
+// it's fine: importantly, it should not falsely succeed.


Something similar to the concern under TODO(irfansharif) below. Say there are 2 spans [a-b)@10 [b-c)@10 that haven't merged due to mergeSpans best-effortness. If there is a read of span [a-c), CheckAllowed can fail due to not finding any whole span that includes [a-c), even though it's technically allowed here.

I'll address this as part of TODO(pav-kv) next to mergeSpans saying that we can sort by (timestamp, key) and achieve "canonicalization" of this set. Seemingly, the callers don't rely on this being sorted by key (and I also confirmed that by running all kvserver tests locally with this change).

pav-kv · 2025-11-10T14:51:16Z

bors r=stevendanna

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. Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

craig · 2025-11-10T14:58:42Z

Build failed:

examples_orms

pav-kv · 2025-11-10T19:31:35Z

bors retry

craig · 2025-11-10T20:33:21Z

Build succeeded:

spanset: rm unused method

483faa2

Epic: none Release note: none

pav-kv force-pushed the spanset-tweaks branch from 41578e4 to 3ded0a0 Compare November 9, 2025 14:05

pav-kv changed the title ~~spanset: simplify mergeSpans~~ spanset: fix and simplify mergeSpans Nov 9, 2025

pav-kv commented Nov 9, 2025

View reviewed changes

pav-kv force-pushed the spanset-tweaks branch 3 times, most recently from b73d78c to 38d2e14 Compare November 10, 2025 10:29

roachpb: use SortFunc in MergeSpans

5cb3ebd

Epic: none Release note: none

pav-kv force-pushed the spanset-tweaks branch from 38d2e14 to 28ea5af Compare November 10, 2025 10:40

roachpb: pass slice to MergeSpans by value

4f02123

It was passed by pointer to avoid allocation when converted to sort.Interface, but it's no longer needed with slices.SortFunc. Epic: none Release note: none

pav-kv force-pushed the spanset-tweaks branch from 28ea5af to 8dc1395 Compare November 10, 2025 10:44

pav-kv added 3 commits November 10, 2025 10:47

spanset: use SortFunc in mergeSpans

d1137f9

Epic: none Release note: none

spanset: pass slice into mergeSpans by value

05e2f2b

Epic: none Release note: none

spanset: remove unused bool return value

f9e764e

Epic: none Release note: none

pav-kv force-pushed the spanset-tweaks branch 2 times, most recently from 8f9764e to cb4c5d0 Compare November 10, 2025 10:49

pav-kv marked this pull request as ready for review November 10, 2025 10:49

pav-kv requested review from a team as code owners November 10, 2025 10:49

pav-kv requested review from jbowens, msbutler, stevendanna and tbg and removed request for a team November 10, 2025 10:49

stevendanna approved these changes Nov 10, 2025

View reviewed changes

pav-kv added 3 commits November 10, 2025 14:02

spanset: test mergeSpans

289226e

Epic: none Release note: none

spanset: fix bug in mergeSpans

7329637

Epic: none Release note: none

spanset: do timestamp check first

1ed2fa1

All branches in this loop check timestamp at the end. This was unnecessarily verbose, error-prone, and led to a bug fixed in the previous commit. Check the timestamp at the beginning once instead. Epic: none Release note: none

pav-kv added 3 commits November 10, 2025 14:17

roachpb: simplify MergeSpans

f886d2b

Epic: none Release note: none

spanset: simplify mergeSpans

9b3327a

Epic: none Release note: none

spanset: comment mergeSpans

d45b333

Epic: none Release note: none

pav-kv force-pushed the spanset-tweaks branch from cb4c5d0 to d45b333 Compare November 10, 2025 14:17

pav-kv commented Nov 10, 2025

View reviewed changes

craig bot merged commit 6471b37 into cockroachdb:master Nov 10, 2025
24 checks passed

celeste-cockroachdb bot added the target-release-26.1.0 label Nov 10, 2025

pav-kv deleted the spanset-tweaks branch November 10, 2025 20:41

celeste-cockroachdb bot added v26.1.0-prerelease and removed target-release-26.1.0 labels Dec 4, 2025

		// TODO(pav-kv): does this make CheckAllowed falsely fail in some cases? Maybe
		// it's fine: importantly, it should not falsely succeed.

spanset: fix and simplify mergeSpans #157105

spanset: fix and simplify mergeSpans #157105

Uh oh!

Conversation

pav-kv commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 9, 2025

Uh oh!

pav-kv Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pav-kv Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

stevendanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stevendanna Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

pav-kv Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pav-kv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pav-kv Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

pav-kv commented Nov 10, 2025

Uh oh!

craig bot commented Nov 10, 2025

Uh oh!

pav-kv commented Nov 10, 2025

Uh oh!

craig bot commented Nov 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pav-kv commented Nov 9, 2025 •

edited

Loading

pav-kv Nov 9, 2025 •

edited

Loading

pav-kv left a comment •

edited

Loading