db: permit flushable-ingest when ElevateWriteStallThresholdForFailover is true

That is, we ignore the length of the flushableList which normally decides
whether a flushable-ingest is permitted.

Fixes #4944

Author: sumeerbhola

db.go

@@ -2630,15 +2630,6 @@ func (d *DB) maybeInduceWriteStall(b *Batch) {
 		for i := range d.mu.mem.queue {
 			size += d.mu.mem.queue[i].totalBytes()
 		}
-		// If ElevateWriteStallThresholdForFailover is true, we give an
-		// unlimited memory budget for memtables. This is simpler than trying to
-		// configure an explicit value, given that memory resources can vary.
-		// When using WAL failover in CockroachDB, an OOM risk is worth
-		// tolerating for workloads that have a strict latency SLO. Also, an
-		// unlimited budget here does not mean that the disk stall in the
-		// primary will go unnoticed until the OOM -- CockroachDB is monitoring
-		// disk stalls, and we expect it to fail the node after ~60s if the
-		// primary is stalled.
 		if size >= uint64(d.opts.MemTableStopWritesThreshold)*d.opts.MemTableSize &&
 			!d.mu.log.manager.ElevateWriteStallThresholdForFailover() {
 			// We have filled up the current memtable, but already queued memtables

ingest.go

@@ -1592,7 +1592,19 @@ func (d *DB) ingest(ctx context.Context, args ingestArgs) (IngestOperationStats,
 		// files.
 		hasRemoteFiles := len(shared) > 0 || len(external) > 0
 		canIngestFlushable := d.FormatMajorVersion() >= FormatFlushableIngest &&
-			(len(d.mu.mem.queue) < d.opts.MemTableStopWritesThreshold) &&
+			// We require that either the queue of flushables is below the
+			// stop-writes threshold (note that this is typically a conservative
+			// check, since not every element of this queue will contribute the full
+			// memtable memory size that could result in a write stall), or WAL
+			// failover is permitting an unlimited queue without causing a write
+			// stall. The latter condition is important to avoid delays in
+			// visibility of concurrent writes that happen to get a sequence number
+			// after this ingest and then must wait for this ingest that is itself
+			// waiting on a large flush. See
+			// https://github.com/cockroachdb/pebble/issues/4944 for an illustration
+			// of this problem.
+			(len(d.mu.mem.queue) < d.opts.MemTableStopWritesThreshold ||
+				d.mu.log.manager.ElevateWriteStallThresholdForFailover()) &&
 			!d.opts.Experimental.DisableIngestAsFlushable() && !hasRemoteFiles &&
 			(!args.ExciseSpan.Valid() || d.FormatMajorVersion() >= FormatFlushableIngestExcises)
 

testdata/flushable_ingest

@@ -27,9 +27,44 @@ set d 1
 blockFlush
 ----
 
+# One WAL, corresponding to the mutable memtable containing the set.
+ls
+----
+000002.log
+LOCK
+MANIFEST-000001
+OPTIONS-000003
+ext
+ext1
+ext2
+ext3
+marker.format-version.000011.024
+marker.manifest.000001.MANIFEST-000001
+
+# Ingest can complete despite the flush being blocked.
 ingest ext1 ext2 ext3
 ----
 
+lsm
+----
+
+# Two more WALs, one for the flushable ingest, and one for the mutable
+# memtable.
+ls
+----
+000002.log
+000004.sst
+000005.sst
+000006.sst
+000007.log
+000008.log
+LOCK
+MANIFEST-000001
+OPTIONS-000003
+ext
+marker.format-version.000011.024
+marker.manifest.000001.MANIFEST-000001
+
 allowFlush
 ----
 
@@ -45,9 +80,10 @@ a:1
 b:1
 d:1
 
-# We expect 1 WAL for an immutable memtable, 1 file for the ingested ssts,
-# one for the mutable memtable. We also expect 3 ssts corresponding to the
-# ingested files.
+# We expect 1 WAL for an immutable memtable, 1 WAL for the ingested ssts, one
+# for the mutable memtable. We also expect 3 ssts corresponding to the
+# ingested files. The assumption here is that this command runs before the
+# flush completes.
 ls
 ----
 000002.log
@@ -163,6 +199,20 @@ blockFlush
 ingest ext4
 ----
 
+ls
+----
+000002.log
+000004.sst
+000005.log
+000006.log
+LOCK
+MANIFEST-000001
+OPTIONS-000003
+ext
+ext5
+marker.format-version.000011.024
+marker.manifest.000001.MANIFEST-000001
+
 allowFlush
 ----
 

wal/wal.go

@@ -349,6 +349,15 @@ type Manager interface {
 	// ElevateWriteStallThresholdForFailover returns true if the caller should
 	// use a high write stall threshold because the WALs are being written to
 	// the secondary dir.
+	//
+	// In practice, if this value is true, we give an unlimited memory budget
+	// for memtables. This is simpler than trying to configure an explicit
+	// value, given that memory resources can vary. When using WAL failover in
+	// CockroachDB, an OOM risk is worth tolerating for workloads that have a
+	// strict latency SLO. Also, an unlimited budget here does not mean that the
+	// disk stall in the primary will go unnoticed until the OOM -- CockroachDB
+	// is monitoring disk stalls, and we expect it to fail the node after ~60s
+	// if the primary is stalled.
 	ElevateWriteStallThresholdForFailover() bool
 	// Stats returns the latest Stats.
 	Stats() Stats