record: allow unbounded queued blocks in LogWriter

[jbowens]

Previously, a LogWriter would allocate up to 16 blocks of 32 KiB for buffering WAL writes. If all 16 blocks had been allocated and no free blocks were available, a batch writing to the WAL would queue until the flushing goroutine freed blocks. In testing of write-heavy workloads, especially with larger value sizes, we've seen queueing at the LogWriter. This queueing blocks the commit pipeline, preventing any batches from committing regardless of priority and whether they require waiting for fsync.

This commit modifies LogWriter to allow the queueing of an unbounded number of blocks. In practice, for the current WAL, the memtable size serves as an upper bound. With a 64 MiB memtable, at most 64 MiB / 32 KiB = 2,048 blocks may queue. This is not an unreasonable of additional memory overhead for a write-heavy workload.

Beyond improving throughput for write-heavy workloads, removing this hard bound improves tolerance of momentary fsync stalls.

Informs cockroachdb/cockroach#88699.

[cockroach-teamcity]

This change is 

[jbowens]

We might also consider adding additional memory accounting for these blocks. I don't think it's urgent, given the 64 MiB bound. But if in the future we allow pipelining WALs and permit buffering writes across the WAL rotation (which I think would help tail latencies considerably), I think we'll need some kind of accounting.

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @sumeerbhola)

[sumeerbhola]

Is it worth adding some commentary to #2540?

Can you add a test that does allocate a large number of blocks, so we can be sure that (a) it is correct, (b) the slices are behaving reasonably in that we are not incurring slice allocations where we don't expect to.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @jbowens)

---

batch.go line 323 at r2 (raw file):

	// WALQueueWaitDuration is the wait time for allocating memory blocks in the
	// LogWriter (due to the LogWriter not writing fast enough).
	WALQueueWaitDuration time.Duration

will this come back once we add some memory bound?
I am wondering whether we want to keep this for now given that we have the plumbing for it in CockroachDB already.

---

record/log_writer.go line 569 at r2 (raw file):

	w.free.Lock()
	if len(w.free.blocks) == 0 {
		w.free.blocks = append(w.free.blocks, &block{})

should we consider allocating the free blocks from a sync.Pool, given the well-defined lifecycle and the larger number of blocks a logWriter may allocate? That is, two levels of free pool management -- what we have here, and then the sync.Pool for &block{} and return them all to the sync.Pool in LogWriter.Close.

[jbowens]

Added a comment to #2540 and a unit test + benchmark.

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @sumeerbhola)

---

batch.go line 323 at r2 (raw file):

Previously, sumeerbhola wrote…

will this come back once we add some memory bound?
I am wondering whether we want to keep this for now given that we have the plumbing for it in CockroachDB already.

Yeah, I started to wonder whether we ultimately should only have MemtableStopWritesThreshold which will also bound on the queued WAL blocks. But I'll keep this around so we can make a decision then.

---

record/log_writer.go line 569 at r2 (raw file):

Previously, sumeerbhola wrote…

should we consider allocating the free blocks from a sync.Pool, given the well-defined lifecycle and the larger number of blocks a logWriter may allocate? That is, two levels of free pool management -- what we have here, and then the sync.Pool for &block{} and return them all to the sync.Pool in LogWriter.Close.

Done.

[sumeerbhola]

Reviewed 4 of 4 files at r3, 1 of 1 files at r4, 1 of 1 files at r6.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @jbowens)

[jbowens]

TFTR!