storage: sync Raft log writes

[petermattis]

Add COCKROACH_SYNC_RAFT_LOG (default true) to control whether writes to
the Raft log are written synchronously to disk. There was a previous
assumption that this was being done, but it turns out that RocksDB does
not sync the WAL by default on writes and we weren't explicitly enabling
it. A write-only workload shows a 25% slowdown for enabling syncing of
Raft log writes, but not doing so is a lie. We can probably recover this
by moving the Raft log out of RocksDB, but doing so is a significant
chunk of work.

---

This change is 

Jira issue: CRDB-14754

Jira issue: CRDB-14755

[bdarnell]

---

Reviewed 13 of 13 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/storage/replica.go, line 2707 at r1 (raw file):

		}
	}
	writer.Close()

Why this addition?

---

pkg/storage/replica.go, line 2708 at r1 (raw file):

	}
	writer.Close()
	if err := batch.Commit(syncRaftLog); err != nil {

It's probably worth a comment that we're intentionally including the HardState under the syncRaftLog umbrella (the HardState requires the same durability guarantees as the log)

---

pkg/storage/engine/engine.go, line 129 at r1 (raw file):

	// calling Repr() on a batch. Using this method is equivalent to constructing
	// and committing a batch whose Repr() equals repr. If sync is true, the
	// batch is synchronously written to disk.

Add "If this Writer is a Batch, sync has no effect" (right? Should we make it an error to specify sync=true when it won't actually go all the way to disk?)

It doesn't look like we ever pass sync=true to ApplyBatchRepr.

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/storage/replica.go, line 2707 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Why this addition?

Just noticed in passing. It should have no effect.

---

pkg/storage/replica.go, line 2708 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

It's probably worth a comment that we're intentionally including the HardState under the syncRaftLog umbrella (the HardState requires the same durability guarantees as the log)

Done.

---

pkg/storage/engine/engine.go, line 129 at r1 (raw file):

If this Writer is a Batch, sync has no effect

Done.

Should we make it an error to specify sync=true when it won't actually go all the way to disk?

Sure. Done.

It doesn't look like we ever pass sync=true to ApplyBatchRepr.

We do in rocksDBBatch.Commit.

---

Comments from Reviewable