sql, storage: tolerate existing splits in AdminSplit and SPLIT AT

This error is not useful; indeed many callers go through hoops to ignore it.
Extending SPLIT AT to handle multiple split points makes this error even more
annoying. Removing this error in favor of a silent no-op (based on discussion
in cockroachdb#14146).

The backup test is changed to read the meta descriptor instead on relying on
the old behavior to verify splits.

pkg/ccl/sqlccl/backup_test.go

@@ -870,12 +870,39 @@ func TestPresplitRanges(t *testing.T) {
 			if err := presplitRanges(ctx, *kvDB, splitPoints); err != nil {
 				t.Error(err)
 			}
-			for _, splitPoint := range splitPoints {
-				// presplitRanges makes the keys into row sentinels, so we have
-				// to match that behavior.
-				splitKey := keys.MakeRowSentinelKey(splitPoint)
-				if err := kvDB.AdminSplit(ctx, splitKey); !testutils.IsError(err, "already split") {
-					t.Errorf("missing split %s: %+v", splitKey, err)
+
+			// Verify that the splits exist.
+			// Note that presplitRanges adds the row sentinel to make a valid table
+			// key, but AdminSplit internally removes it (via EnsureSafeSplitKey). So
+			// we expect splits that match the splitPoints exactly.
+			for _, splitKey := range splitPoints {
+				// Scan the meta range for splitKey.
+				rk, err := keys.Addr(splitKey)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				startKey := keys.RangeMetaKey(rk)
+				endKey := keys.Meta2Prefix.PrefixEnd()
+
+				kvs, err := kvDB.Scan(context.Background(), startKey, endKey, 1)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if len(kvs) != 1 {
+					t.Fatalf("expected 1 KV, got %v", kvs)
+				}
+				desc := &roachpb.RangeDescriptor{}
+				if err := kvs[0].ValueProto(desc); err != nil {
+					t.Fatal(err)
+				}
+				if !desc.EndKey.Equal(rk) {
+					t.Errorf(
+						"missing split %s: range %s to %s",
+						keys.PrettyPrint(splitKey),
+						keys.PrettyPrint(desc.StartKey.AsRawKey()),
+						keys.PrettyPrint(desc.EndKey.AsRawKey()),
+					)
 				}
 			}
 		})

pkg/ccl/sqlccl/restore.go

@@ -9,8 +9,6 @@
 package sqlccl
 
 import (
-	"strings"
-
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 	"golang.org/x/sync/errgroup"
@@ -470,9 +468,7 @@ func presplitRanges(baseCtx context.Context, db client.DB, input []roachpb.Key)
 		splitKey := append([]byte(nil), splitPoints[splitIdx]...)
 		splitKey = keys.MakeRowSentinelKey(splitKey)
 		if err := db.AdminSplit(ctx, splitKey); err != nil {
-			if !strings.Contains(err.Error(), "range is already split at key") {
-				return err
-			}
+			return err
 		}
 
 		splitPointsLeft, splitPointsRight := splitPoints[:splitIdx], splitPoints[splitIdx+1:]

pkg/cmd/zerosum/main.go

@@ -226,9 +226,6 @@ func (z *zeroSum) monkey(tableID uint32, d time.Duration) {
 		switch r.Intn(2) {
 		case 0:
 			if err := z.Split(z.RandNode(r.Intn), key); err != nil {
-				if strings.Contains(err.Error(), "range is already split at key") {
-					continue
-				}
 				z.maybeLogError(err)
 			} else {
 				atomic.AddUint64(&z.stats.splits, 1)

pkg/kv/split_test.go

@@ -221,7 +221,7 @@ func TestRangeSplitsWithWritePressure(t *testing.T) {
 }
 
 // TestRangeSplitsWithSameKeyTwice check that second range split
-// on the same splitKey should not cause infinite retry loop.
+// on the same splitKey succeeds.
 func TestRangeSplitsWithSameKeyTwice(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	s, _ := createTestDB(t)
@@ -233,13 +233,7 @@ func TestRangeSplitsWithSameKeyTwice(t *testing.T) {
 		t.Fatal(err)
 	}
 	log.Infof(context.Background(), "split at key %q first time complete", splitKey)
-	ch := make(chan error)
-	go func() {
-		// should return error other than infinite loop
-		ch <- s.DB.AdminSplit(context.TODO(), splitKey)
-	}()
-
-	if err := <-ch; err == nil {
-		t.Error("range split on same splitKey should fail")
+	if err := s.DB.AdminSplit(context.TODO(), splitKey); err != nil {
+		t.Fatal(err)
 	}
 }

pkg/sql/split_at_test.go

@@ -55,8 +55,8 @@ func TestSplitAt(t *testing.T) {
 			in: "ALTER TABLE d.t SPLIT AT (2, 'b')",
 		},
 		{
-			in:    "ALTER TABLE d.t SPLIT AT (2, 'b')",
-			error: "range is already split",
+			// Splitting at an existing split is a silent no-op.
+			in: "ALTER TABLE d.t SPLIT AT (2, 'b')",
 		},
 		{
 			in:    "ALTER TABLE d.t SPLIT AT ('c', 3)",

pkg/storage/client_split_test.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"math/rand"
 	"reflect"
-	"strings"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -313,15 +312,10 @@ func TestStoreRangeSplitConcurrent(t *testing.T) {
 	for i := 0; i < concurrentCount; i++ {
 		pErr := <-errCh
 		if pErr != nil {
-			// There are only three expected errors from concurrent splits:
-			// conflicting range descriptors if the splits are initiated
-			// concurrently, the range is already split at the specified key or the
-			// split key is outside of the bounds for the range.
-			expected := strings.Join([]string{
-				"range is already split at key",
-				"key range .* outside of bounds of range",
-			}, "|")
-			if !testutils.IsError(pErr.GoError(), expected) {
+			// The only expected error from concurrent splits is the split key being
+			// outside the bounds for the range. Note that conflicting range
+			// descriptor errors are retried internally.
+			if !testutils.IsError(pErr.GoError(), "key range .* outside of bounds of range") {
 				t.Fatalf("unexpected error: %v", pErr)
 			}
 			failureCount++

pkg/storage/replica_command.go

@@ -2572,7 +2572,8 @@ func (r *Replica) adminSplitWithDescriptor(
 	// roachpb.NewRangeKeyMismatchError if splitKey equals to desc.EndKey,
 	// otherwise it will cause infinite retry loop.
 	if desc.StartKey.Equal(splitKey) || desc.EndKey.Equal(splitKey) {
-		return reply, roachpb.NewErrorf("range is already split at key %s", splitKey)
+		log.Event(ctx, "range already split")
+		return reply, nil
 	}
 	log.Event(ctx, "found split key")
 

pkg/storage/replicate_queue_test.go

@@ -62,10 +62,6 @@ func TestReplicateQueueRebalance(t *testing.T) {
 		tableID := keys.MaxReservedDescID + i + 1
 		splitKey := keys.MakeRowSentinelKey(keys.MakeTablePrefix(uint32(tableID)))
 		if _, _, err := tc.SplitRange(splitKey); err != nil {
-			// TODO(peter): Remove when #11592 is fixed.
-			if testutils.IsError(err, "range is already split at key") {
-				continue
-			}
 			t.Fatal(err)
 		}
 	}