cockroachdb / cockroachdb/cockroach
kvserver: make initial closed timestamp on splits consistent
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
When applying splits, we [initialize](https://github.com/cockroachdb/cockroach/blob/94013343b92f6121b7eb19d6d462ae4c497fd398/pkg/kv/kvserver/store_split.go#L148-L159) the "raft" closed timestamp from the pre-split `Replica.GetCurrentClosedTimestamp()` which is the max of the "raft" CTS and the [side-transport](https://github.com/cockroachdb/cockroach/blob/33e009d762585f330945d5238c12c2b5c8b72fe0/pkg/kv/kvserver/replica_follower_read.go#L167) CTS.
It seems that there is a risk for different replicas to be initialized at different CTs, and a subsequent replica inconsistency, or CT regression the like of #70894 and #136971 (whichever fires first). It's unclear whether or why the CT is consistent. Particularly the comment saying `initClosedTS ... is expected to be in advance of r.GetCurrentClosedTimestamp() since it's coming hot off a Raft command` appears unproven.
Ideas/options:
1. Make the comment more convincing if the code is correct.
2. Assert that `initClosedTS` is non-nil and don't `forward(GetCurrentClosedTimestamp)`. Conjecture: the latter is always no-op.
3. ~We could consider writing the initial RHS closed timestamp into the batch at split evaluation time~ instead of computing it on each replica at apply time. E.g. as part of [WriteInitialReplicaState](https://github.com/cockroachdb/cockroach/blob/33e009d762585f330945d5238c12c2b5c8b72fe0/pkg/kv/kvserver/batcheval/cmd_end_transaction.go#L1478) which currently leaves it blank.
Jira issue: CRDB-51901
Contributor guide
Assessment
This issue has not been assessed yet.