backport-19.2: storage: call EnsureSafeSplitKey during load-based splits

pkg/sql/row/fetcher.go

@@ -587,6 +587,11 @@ func (rf *Fetcher) NextKey(ctx context.Context) (rowDone bool, err error) {
 			// No more keys in the scan. We need to transition
 			// rf.rowReadyTable to rf.currentTable for the last
 			// row.
+			//
+			// NB: this assumes that the KV layer will never split a range
+			// between column families, which is a brittle assumption.
+			// See:
+			// https://github.com/cockroachdb/cockroach/pull/42056
 			rf.rowReadyTable = rf.currentTable
 			return true, nil
 		}

pkg/storage/split/decider.go

@@ -15,6 +15,7 @@ package split
 import (
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
@@ -139,7 +140,36 @@ func (d *Decider) MaybeSplitKey(now time.Time) roachpb.Key {
 	d.mu.Lock()
 	d.recordLocked(now, 0, nil)
 	if d.mu.splitFinder != nil && d.mu.splitFinder.Ready(now) {
-		key = d.mu.splitFinder.Key()
+		// We've found a key to split at. This key might be in the middle of a
+		// SQL row. If we fail to rectify that, we'll cause SQL crashes:
+		//
+		// https://github.com/cockroachdb/cockroach/pull/42056
+		//
+		// While the behavior at the SQL level is arguably bad and should be
+		// fixed, splitting between column families is also never a good idea
+		// for performance in general. So, if the split key is, say
+		//
+		//   /Table/51/52/53/54/55/9/1
+		//
+		// then we want to split instead at
+		//
+		//   /Table/51/52/53/54/55
+		//
+		// (see TestDeciderCallsEnsureSafeSplitKey).
+		//
+		// The key found here isn't guaranteed to be a valid SQL column family
+		// key. This is because the keys are sampled from StartKey of requests
+		// hitting this replica. Ranged operations may well wish to exclude the
+		// start point by calling .Next() or may span multiple ranges, and so
+		// such a key may end up being passed to EnsureSafeSplitKey here.
+		//
+		// 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
+		}
 	}
 	d.mu.Unlock()
 

pkg/storage/split/decider_test.go

@@ -15,9 +15,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestDecider(t *testing.T) {
@@ -170,3 +173,36 @@ func TestDecider(t *testing.T) {
 	assert.Nil(t, d.MaybeSplitKey(ms(tick)))
 	assert.Nil(t, d.mu.splitFinder)
 }
+
+func TestDeciderCallsEnsureSafeSplitKey(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), keys.MakeFamilyKey(baseKey, 1)...)} }
+	c1 := func() roachpb.Span { return roachpb.Span{Key: append([]byte(nil), keys.MakeFamilyKey(baseKey, 9)...)} }
+
+	expK, err := keys.EnsureSafeSplitKey(c1().Key)
+	require.NoError(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, expK, k)
+}