rditer,kvserver: simplify and unify replica span handling

[tbg]

From 40573d3 Mon Sep 17 00:00:00 2001
From: Tobias Grieger tobias.b.grieger@gmail.com
Date: Fri, 6 Jan 2023 05:42:50 +0100
Subject: [PATCH] rditer,kvserver: simplify and unify replica span handling

A Replica comprises a number of spans:

• RangeID-keyed replicated data, such as the GCThresholdKey,
  confusingly named "local RangeID keys" since they all have a /Local/
  (\x02) prefix. They have a r infix.
• RangeID-keyed unreplicated data, such as the HardState,
• The system-internal key-addressable "parallel plane" (such as the
  RangeDescriptorKey), confusingly named "local keys" (again, \x02
  prefix, but u infix).
• Two lock spans: one for the "local" keyspace (e.g. for an intent on
  a RangeDescriptor), one for the "global" keyspace (e.g. regular
  row intents), in both cases the span is local and uses a k infix.
• The actual span of the range (i.e. user data).

Code that needs some or all of these spans gets them from various
methods provided by the rditer package. Other than being generally a
messy affair, with a separate raft log, RangeID-based unreplicated keys
will mostly¹ live on the logstore engine, while RangeID-based
replicated keys will be on the state machine engine and so the current
approach of targeting either only the replicated or the combined
(replicated and unreplicated) part of the local RangeID-based span
no longer works. It was also never particularly obvious which spans
one would select with that bool, and I wound up confused more than
once.

The above means we need to stop thinking about RangeID-based keys as
belonging to "one span" - the two RangeID-keyed spans are very different
in nature and storage.

This commit introduces a unified way to select the different parts of the
keyspace. It then goes ahead and uses this new primitive for all span
uses external to the rditer package. (rditer itself has a few methods
that need exotic combinations of spans, and it gets to do those more
ad-hoc for now but this could be improved later).

In particular, we can now rework clearRangeData and give it an
analogoous way to explicitly select which spans to clear. Replica
removal is a central work item in CRDB-220, since it comes up all
over the place: snapshots (in particular in combination with replica
subsumption), splits, replication changes, replicaGC.

After this PR, we can improve splits by not mutating the raft state of
the right-hand side replica in cases where it is known to be newer than
the split. The old API forced us to remove the raft state as well, and
we had to awkwardly lock the right-hand side and write the raft state
back to make things work. We can now simply opt out of mutating that
raft state (via ClearUnreplicatedByRangeID=false). This is left for
a separate PR.

This PR makes some additional improvements:

• improve rditer tests via echotest, also test lock table keys
• improved commentary, esp. around ¹.

Release note: None
Epic: CRDB-220

Footnotes

1. https://github.com/cockroachdb/cockroach/issues/95055 ↩ ↩²

[cockroach-teamcity]

This change is 

[tbg]

I suggest starting the review with two windows, one looking at old diff for makeRangeKeySpans and comparing it to Select in new file select.go. The logic essentially transitioned over.

The remainder of the changes in rditer remove custom implementations and use Select instead:

• MakeReplicatedKeySpansExceptRangeID was removed, since we could just use Select directly.
• MakeRangeIDLocalKeySpan is no longer there because we now have two methods to make spans for the replicated and unreplicated parts, respectively: makeRangeID{Unre,Re}plicatedSpan.

In kvserver, the main change affects clearRangeData, which wants to use the new API:

• clearRangeDataOptions is introduced and basically mirrors the options to Select
• all callers are updated to use it.

[pav-kv]

Dumping some comments for today.

[pav-kv]

Commented the fixups. Looking good. I'll make a final pass on the whole PR after this little round later today.

[tbg]

Could I trouble you for a last review? I just fixed a pretty severe (but at least caught in 100 places across CI) problem, see the new commits.

[tbg]

Friendly ping.

[tbg]

TFTR! I skipped the few nits you had to get this PR merged, but I did make the non-nit adjustment.
bors r=pavelkalinnikov

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[tbg]

   	Error:      	"change replicas of r54 failed: descriptor changed: [expected] r54:/T{able/53-enant/10} [(n1,s1):1, (n2,s2):2, (n5,s5):3, (n3,s3):5NON_VOTER, (n4,s4):6NON_VOTER, next=7, gen=9] != [actual] r54:/T{able/53-enant/10} [(n1,s1):1, (n2,s2):2, (n5,s5):3NON_VOTER, (n3,s3):5, next=7, gen=12]" does not contain "ok"

#95262

bors r=pavelkalinnikov

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)