cockroachdb / cockroachdb/cockroach

kvserver: make initial closed timestamp on splits consistent

Open
#148,972 2 comments 0 reactions 0 assignees View on GitHub
A-kv-replication C-bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.