cockroachdb / cockroachdb/cockroach

kvserver: AddSSTable crash recovery

Open
#141,122 5 comments 0 reactions 0 assignees View on GitHub
A-kv branch-master C-bug T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

### Issue

Most raft commands are applied to the state machine [atomically with](https://github.com/cockroachdb/cockroach/blob/2606875e8b0a8b9b0b75738d429a451b249f0dfb/pkg/kv/kvserver/replica_app_batch.go#L598-L604) bumping the `RaftAppliedIndex` (and [other](https://github.com/cockroachdb/cockroach/blob/2606875e8b0a8b9b0b75738d429a451b249f0dfb/pkg/kv/kvserver/stateloader/stateloader.go#L238-L242) things like closed timestamp). Except the `AddSSTable` commands: they are applied as Pebble [ingestion](https://github.com/cockroachdb/cockroach/blob/2606875e8b0a8b9b0b75738d429a451b249f0dfb/pkg/kv/kvserver/replica_proposal.go#L690-L748) first, and then separately there is a write batch.

Because of this non-atomicity, it’s possible that we ingest, then crash/restart, and end up with the SST applied but `RaftAppliedIndex` being stale (by more than one, since typical command applications don’t sync). After restart, the `Replica` is in an incorrect state (with "future" writes applied), until all commands are replayed up to this `AddSSTable`.

### Potential Fixes

Options:

1. Support atomic ingestion+batch in Pebble, and make `AddSSTable` application atomic. Considered complex.
2. Recover partially applied `AddSSTable` commands on restart. E.g. on applying `AddSSTable`, write/sync a batch saying that we’re about to apply a non-atomic command, and only then ingest the SST and write another batch indicating a finished command. On restart, detect a partially applied `AddSSTable` and finish it before the `Replica` is initialized.
3. Same as option 2, but no sync on the first batch. Requires support from Pebble (below).
4. There is another [approach](https://github.com/cockroachdb/cockroach/blob/546e170520fd6f5e61308371bf7eef1f6de84d6a/pkg/storage/replicas_storage.go#L507-L510) which requires broader “standalone” raft log replays on restarts. It wouldn’t incur an additional sync on `AddSSTable`, but is more complex.
5. Convert the `AddSSTable` command application into a multi-SST ingestion, i.e. convert the write batch (with `RaftAppliedIndex` etc) into an SST, and atomically ingest both.
6. Introduce a “flush barrier” on splits/merges. Say, when a range is split/merged, we write a no-op range key into Pebble, over the range’s whole “user” key space. Now, any AddSSTable into this range will cause a flush (because it overlaps this barrier), but at most once (only if it arrives before a flush). The next one will no longer overlap this barrier. If there are no AddSSTables (typical case), there is no extra sync/flush.
7. **(preferred)** The RSE project will introduce WAG [#149604], recording replica lifecycle events to the log engine before applying the state machine mutation; which can be replayed at restart. One of these instructions is "apply to index" for a range. It will allow nicely fixing this bug.

### Background / Details

Solution 2 requires an extra sync because Pebble currently doesn't guarantee "durability order" between an ingestion and a write batch that do not intersect by keys. Consider a scenario where we make a write, ingest, make another write; and, before we were able to sync the WAL, there is a crash. On restart, the two writes are dropped but the ingestion is applied.

It would be possible to avoid this sync (solution 3) if Pebble made ingestions behave exactly like a write batch w.r.t. durability: post-restart, there is a prefix of all writes/ingestions that is durable.

More details / discussion in a [thread](https://cockroachlabs.slack.com/archives/CAC6K3SLU/p1739209164941419).

Jira issue: CRDB-47622

Contributor guide

Open the contributing guide

Research direction

Start with the AddSSTable application path in pkg/kv/kvserver/replica_proposal.go and the atomic state-machine updates in replica_app_batch.go, then review stateloader.go and the WAG proposal in issue #149604. Define the recovery behavior for a crash between SST ingestion and the RaftAppliedIndex update, and verify that restart no longer leaves future writes applied with a stale index.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.