kv: combine local and global uncertainty limit into Interval struct

[nvanbenschoten]

This commit combines the server-side "local" and "global" uncertainty limits into an uncertainty.Interval struct. It also centralizes the computation of this interval to an uncertainty.ComputeInterval function.

As a result, MVCC no longer reaches inside of a Transaction object to construct its implied uncertainty interval. Instead, an uncertainty interval is supplied to MVCC directly.

This refactor is made in preparation for a follow-on commit that will address #58459 (giving non-transactional requests uncertainty intervals), which in turn will prepare to simplify HLC handling throughout the system.

The refactor also cleanly addresses a goal I've had to make the local uncertainty limit a ClockTimestamp, which helps clarify its role in the system.

[cockroach-teamcity]

This change is 

[tbg]

Reviewed 17 of 17 files at r1, 26 of 26 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @irfansharif, and @nvanbenschoten)

---

pkg/kv/kvserver/uncertainty/compute.go, line 56 at r2 (raw file):

//    is guaranteed to be greater than any write which occurred on the
//    right-hand side.
// TODO(nvanbenschoten): fix this bug with range merges.

Is there an issue for this?

---

pkg/kv/kvserver/uncertainty/interval.go, line 50 at r2 (raw file):

// uncertain to a reader with a ReadTimestamp below the value's and with
// the specified uncertainty interval.
func (in Interval) IsUncertain(valueTs hlc.Timestamp) bool {

So now Interval{}.IsUncertain(ts) is always false, meaning we have some special casing for non-txnal requests right now? Might be worth a note.

[sumeerbhola]

for the storage package changes

Reviewed 3 of 17 files at r1, 12 of 26 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @irfansharif, @nvanbenschoten, and @tbg)

---

pkg/kv/kvserver/uncertainty/compute.go, line 56 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Is there an issue for this?

I don't fully understand this example, given that observed timestamps are for a node. Presumably n1 is not the leaseholder for r2 before the merge, and "acquires" the lease as a side-effect of the merge since it is r1's raft group that is continuing post merge?

[nvanbenschoten]

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei, @irfansharif, @sumeerbhola, and @tbg)

---

pkg/kv/kvserver/uncertainty/compute.go, line 56 at r2 (raw file):
I have a branch that addresses this, but I'm not happy enough with it to commit to a fast follow PR. So +1 on this getting an issue. I opened #73292 to track this.

Presumably n1 is not the leaseholder for r2 before the merge, and "acquires" the lease as a side-effect of the merge since it is r1's raft group that is continuing post merge?

Right, n1 is not the leaseholder for r2.

What you said is one way to view what's broken here. Merging a right-hand neighbor into a LHS range does not force the LHS range to acquire a new lease. So r1's lease can extend from before the merge to beyond it. This breaks the existing logic that forwards the local limit to the lease start time.

Or are you saying that you would expect that the replica of r2 on n1 would acquire the lease before the merge could complete? This is not the case. We don't require collocated leaseholders before a merge can complete. But even if we did, that would not be enough to address this issue. We would still need some way to ignore observed timestamps acquired from n1 before the lease transfer occurred.

---

pkg/kv/kvserver/uncertainty/interval.go, line 50 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

So now Interval{}.IsUncertain(ts) is always false, meaning we have some special casing for non-txnal requests right now? Might be worth a note.

I don't quite understand. A follow on PR will be adding uncertainty intervals to non-txnal requests, but for now, no value is uncertain to a non-txnal request. There isn't special casing for this. Well, there is for efficiency purposes (see pebbleMVCCScanner.checkUncertainty), but there doesn't need to be.

Any chance you read this as IsNotUncertain or IsCertain and that's where the confusion is coming from?

Also, I added test cases for this.

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei, @irfansharif, @nvanbenschoten, @sumeerbhola, and @tbg)

---

pkg/kv/kvserver/uncertainty/interval.go, line 50 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I don't quite understand. A follow on PR will be adding uncertainty intervals to non-txnal requests, but for now, no value is uncertain to a non-txnal request. There isn't special casing for this. Well, there is for efficiency purposes (see pebbleMVCCScanner.checkUncertainty), but there doesn't need to be.

Any chance you read this as IsNotUncertain or IsCertain and that's where the confusion is coming from?

Also, I added test cases for this.

👍

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei, @irfansharif, @nvanbenschoten, and @tbg)

---

pkg/kv/kvserver/uncertainty/compute.go, line 56 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I have a branch that addresses this, but I'm not happy enough with it to commit to a fast follow PR. So +1 on this getting an issue. I opened #73292 to track this.

Presumably n1 is not the leaseholder for r2 before the merge, and "acquires" the lease as a side-effect of the merge since it is r1's raft group that is continuing post merge?

Right, n1 is not the leaseholder for r2.

What you said is one way to view what's broken here. Merging a right-hand neighbor into a LHS range does not force the LHS range to acquire a new lease. So r1's lease can extend from before the merge to beyond it. This breaks the existing logic that forwards the local limit to the lease start time.

Or are you saying that you would expect that the replica of r2 on n1 would acquire the lease before the merge could complete? This is not the case. We don't require collocated leaseholders before a merge can complete. But even if we did, that would not be enough to address this issue. We would still need some way to ignore observed timestamps acquired from n1 before the lease transfer occurred.

Thanks for clarifying the details -- this matches what I was guessing.

[nvanbenschoten]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)