From 0ebacb741d4919f7573a0c24f0adc2e20f13c457 Mon Sep 17 00:00:00 2001
From: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Date: Tue, 10 Dec 2019 18:01:35 +0100
Subject: [PATCH] decider: ignore EnsureSafeSplitKey on error

Fixes #42903.

Load based splitting would previously not split when EnsureSafeSplitKey
returns an error. However such an error only meant that the key in
question wasn't even a SQL key, which is the case most of the time,
at least in simple workloads such as kv (the SQL layer will send
single-element scans, but without specifying the column family).

No release note since not released.

Release note: None
---
 pkg/storage/split/decider.go      | 14 ++++++++----
 pkg/storage/split/decider_test.go | 38 +++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/pkg/storage/split/decider.go b/pkg/storage/split/decider.go
index 4d49015f08dc..6e2000bf10a6 100644
--- a/pkg/storage/split/decider.go
+++ b/pkg/storage/split/decider.go
@@ -169,10 +169,16 @@ func (d *Decider) MaybeSplitKey(now time.Time) roachpb.Key {
 		//
 		// We take the risk that the result may sometimes not be a good split
 		// point (or even in this range).
-		var err error
-		key, err = keys.EnsureSafeSplitKey(d.mu.splitFinder.Key())
-		if err != nil {
-			key = nil
+		//
+		// Note that we ignore EnsureSafeSplitKey when it returns an error since
+		// that error only tells us that this key couldn't possibly be a SQL
+		// key. This is more common than one might think since SQL issues plenty
+		// of scans over all column families, meaning that we'll frequently find
+		// a key that has no column family suffix and thus errors out in
+		// EnsureSafeSplitKey.
+		key = d.mu.splitFinder.Key()
+		if safeKey, err := keys.EnsureSafeSplitKey(key); err == nil {
+			key = safeKey
 		}
 	}
 	d.mu.Unlock()
diff --git a/pkg/storage/split/decider_test.go b/pkg/storage/split/decider_test.go
index 03134d2568f5..435c5c354d19 100644
--- a/pkg/storage/split/decider_test.go
+++ b/pkg/storage/split/decider_test.go
@@ -15,6 +15,7 @@
 package split
 
 import (
+	"math"
 	"math/rand"
 	"testing"
 	"time"
@@ -210,3 +211,40 @@ func TestDeciderCallsEnsureSafeSplitKey(t *testing.T) {
 
 	require.Equal(t, expK, k)
 }
+
+func TestDeciderIgnoresEnsureSafeSplitKeyOnError(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	intn := rand.New(rand.NewSource(11)).Intn
+
+	var d Decider
+	Init(&d, intn, func() float64 { return 1.0 })
+
+	baseKey := keys.MakeTablePrefix(51)
+	for i := 0; i < 4; i++ {
+		baseKey = encoding.EncodeUvarintAscending(baseKey, uint64(52+i))
+	}
+	c0 := func() roachpb.Span {
+		return roachpb.Span{Key: append([]byte(nil), encoding.EncodeUvarintAscending(baseKey, math.MaxInt32+1)...)}
+	}
+	c1 := func() roachpb.Span {
+		return roachpb.Span{Key: append([]byte(nil), encoding.EncodeUvarintAscending(baseKey, math.MaxInt32+2)...)}
+	}
+
+	_, err := keys.EnsureSafeSplitKey(c1().Key)
+	require.Error(t, err)
+
+	var k roachpb.Key
+	var now time.Time
+	for i := 0; i < 2*int(minSplitSuggestionInterval/time.Second); i++ {
+		now = now.Add(500 * time.Millisecond)
+		d.Record(now, 1, c0)
+		now = now.Add(500 * time.Millisecond)
+		d.Record(now, 1, c1)
+		k = d.MaybeSplitKey(now)
+		if len(k) != 0 {
+			break
+		}
+	}
+
+	require.Equal(t, c1().Key, k)
+}