batcheval: add latch key protecting range key stats update

[aliher1911]

Previously GC needed to get a read latch with max timestamp to
ensure that range tombstones are not modified during GC. This
is causing all writers to get stuck in queue while GC is validating
request and removing range tombstone.
This commit adds a dedicated latch key
LocalRangeRangeTombstoneStatsUpdateLockSuffix to address the problem.
All range tombstone writers obtain this read latch on top of the write
latches for the ranges they are interested to update.
GC on the other hand will obtain write latch on that key. This
approach allows point writers to proceed during GC, but will block new
range tombstones from being written. Non conflicting writes of range
tombstones could still proceed since their write latch ranges don't
overlap.

Release justification: this is a safe change as range tombstone
behaviour is not enabled yet and the change is needed to address
potential performance regressions.

Release note: None

Fixes #84576
Fixes #86551

[cockroach-teamcity]

This change is 

[aliher1911]

Other commands needs latches as well. TBD.

[tbg]

Looks good mod Erik's comments.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @erikgrinaker)

---

pkg/keys/keys.go line 1061 at r1 (raw file):

// RangeTombstoneStatsUpdateKey returns a system-local key for last used GC threshold on the
// user keyspace. Reads and writes <= this timestamp will not be served.
func (b RangeIDPrefixBuf) RangeTombstoneStatsUpdateKey() roachpb.Key {

Inconsistent name and comment.

---

pkg/kv/kvserver/batcheval/cmd_delete_range.go line 63 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

We only need to set this when UseRangeTombstone is set.

We'll need this for all other operations that can affect range keys too, i.e. ClearRange, RevertRange, and AddSSTable. The last one is a bit unfortunate -- we could maybe look at the given SST stats, but I'm not sure we want to rely on them for correctness.

Why is it unfortunate to do this for AddSSTable? Shouldn't matter much, since ranges undergoing GC are unlikely to also be receiving SSTs, no?

[erikgrinaker]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @tbg)

---

pkg/kv/kvserver/batcheval/cmd_delete_range.go line 63 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Why is it unfortunate to do this for AddSSTable? Shouldn't matter much, since ranges undergoing GC are unlikely to also be receiving SSTs, no?

It's just a bit coarse to essentially block all AddSSTableactivity during MVCC range tombstone GC. This is for the general MVCC range tombstone GC, not for the fast path used by schema GC, so this could happen across live keyspace. E.g. if someone cancels an import, then retries it, and the GC triggers during the import. Not a huge deal though, should be fine (at least for now).

[erikgrinaker]

I think we'll have to do this for EndTxn too, for splits/merges, where we peek around the split/merge point and adjust range key stats -- since GC is now latchless and the stats updates are non-commutative.

[erikgrinaker]

MVCCRangeKeyGCKey()

[erikgrinaker]

Let's add a helper for this, similarly to e.g. keys.RaftTruncatedStateKey(rangeID).

[erikgrinaker]

We discussed earlier that GC takes out a read latch on the range descriptor, but how exactly does that serialize with splits/merges? May well be right, I'd just like to be sure we're covered.

[aliher1911]

I think you are right here. We are not getting enough latches to cover GC range ops.

[erikgrinaker]

While we're here, could you update all the comments around rangeTombstonePeekBounds() calls to say that even though we're possibly taking out latches beyond the range end bound, the latches are enforced per-range, so we won't contend with other ranges -- and the peek bounds will be tightened to the range bounds during evaluation.

[aliher1911]

bors r=erikgrinaker

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)