Merge #74674

74674: kvserver: don't use `ClearRange` point deletes with estimated MVCC stats r=tbg a=erikgrinaker

`ClearRange` avoids dropping a Pebble range tombstone if the amount of
data that's deleted is small (<=512 KB), instead dropping point
deletions. It uses MVCC statistics to determine this. However, when
clearing an entire range, it will rely on the existing range MVCC stats
rather than computing them.

These range statistics can be highly inaccurate -- in some cases so
inaccurate that they even become negative. This in turn can cause
`ClearRange` to submit a huge write batch, which gets rejected by Raft
with `command too large`.

This patch avoids dropping point deletes if the statistics are estimated
(which is only the case when clearing an entire range). Alternatively,
it could do a full stats recomputation in this case, but entire range
deletions seem likely to be large and/or rare enough that dropping a
range tombstone is fine.

Resolves #74686.

Release note (bug fix): Fixed a bug where deleting data via schema
changes (e.g. when dropping an index or table) could fail with a
"command too large" error.

Co-authored-by: Erik Grinaker <[email protected]>

pkg/kv/kvserver/batcheval/cmd_clear_range.go

@@ -97,8 +97,15 @@ func ClearRange(
 	// If the total size of data to be cleared is less than
 	// clearRangeBytesThreshold, clear the individual values with an iterator,
 	// instead of using a range tombstone (inefficient for small ranges).
-	if total := statsDelta.Total(); total < ClearRangeBytesThreshold {
-		log.VEventf(ctx, 2, "delta=%d < threshold=%d; using non-range clear", total, ClearRangeBytesThreshold)
+	//
+	// However, don't do this if the stats contain estimates -- this can only
+	// happen when we're clearing an entire range and we're using the existing
+	// range stats. We've seen cases where these estimates are wildly inaccurate
+	// (even negative), and it's better to drop an unnecessary range tombstone
+	// than to submit a huge write batch that'll get rejected by Raft.
+	if statsDelta.ContainsEstimates == 0 && statsDelta.Total() < ClearRangeBytesThreshold {
+		log.VEventf(ctx, 2, "delta=%d < threshold=%d; using non-range clear",
+			statsDelta.Total(), ClearRangeBytesThreshold)
 		iter := readWriter.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{
 			LowerBound: from,
 			UpperBound: to,
@@ -154,7 +161,7 @@ func computeStatsDelta(
 		}
 		// If we took the fast path but race is enabled, assert stats were correctly computed.
 		if fast {
-			delta.ContainsEstimates = computed.ContainsEstimates
+			computed.ContainsEstimates = delta.ContainsEstimates // retained for tests under race
 			if !delta.Equal(computed) {
 				log.Fatalf(ctx, "fast-path MVCCStats computation gave wrong result: diff(fast, computed) = %s",
 					pretty.Diff(delta, computed))

pkg/kv/kvserver/batcheval/cmd_clear_range_test.go

@@ -65,6 +65,7 @@ func TestCmdClearRangeBytesThreshold(t *testing.T) {
 	overFull := ClearRangeBytesThreshold/len(valueStr) + 1
 	tests := []struct {
 		keyCount           int
+		estimatedStats     bool
 		expClearIterCount  int
 		expClearRangeCount int
 	}{
@@ -85,6 +86,13 @@ func TestCmdClearRangeBytesThreshold(t *testing.T) {
 			expClearIterCount:  0,
 			expClearRangeCount: 1,
 		},
+		// Estimated stats always use ClearRange.
+		{
+			keyCount:           1,
+			estimatedStats:     true,
+			expClearIterCount:  0,
+			expClearRangeCount: 1,
+		},
 	}
 
 	for _, test := range tests {
@@ -100,6 +108,9 @@ func TestCmdClearRangeBytesThreshold(t *testing.T) {
 					t.Fatal(err)
 				}
 			}
+			if test.estimatedStats {
+				stats.ContainsEstimates++
+			}
 
 			batch := &wrappedBatch{Batch: eng.NewBatch()}
 			defer batch.Close()
@@ -126,10 +137,12 @@ func TestCmdClearRangeBytesThreshold(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			// Verify cArgs.Stats is equal to the stats we wrote.
+			// Verify cArgs.Stats is equal to the stats we wrote, ignoring some values.
 			newStats := stats
-			newStats.SysBytes, newStats.SysCount, newStats.AbortSpanBytes = 0, 0, 0          // ignore these values
-			cArgs.Stats.SysBytes, cArgs.Stats.SysCount, cArgs.Stats.AbortSpanBytes = 0, 0, 0 // these too, as GC threshold is updated
+			newStats.ContainsEstimates, cArgs.Stats.ContainsEstimates = 0, 0
+			newStats.SysBytes, cArgs.Stats.SysBytes = 0, 0
+			newStats.SysCount, cArgs.Stats.SysCount = 0, 0
+			newStats.AbortSpanBytes, cArgs.Stats.AbortSpanBytes = 0, 0
 			newStats.Add(*cArgs.Stats)
 			newStats.AgeTo(0) // pin at LastUpdateNanos==0
 			if !newStats.Equal(enginepb.MVCCStats{}) {