storage: avoid unsplittable range

If a range cannot be split because a valid split key cannot be found,
set the max size for the range to the range's current size. When a range
is successfully split, reset the max range size to the zone's max range
size. This avoids situations where a range with a single large row
cannot be split.

Fixes cockroachdb#9555

pkg/storage/client_split_test.go

@@ -1955,3 +1955,65 @@ func TestDistributedTxnCleanup(t *testing.T) {
 		}
 	}
 }
+
+func TestUnsplittableRange(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	stopper := stop.NewStopper()
+	defer stopper.Stop()
+
+	store := createTestStoreWithConfig(t, stopper, storage.TestStoreConfig(nil))
+	store.ForceSplitScanAndProcess()
+
+	// Add a single large row to /Table/14.
+	tableKey := keys.MakeTablePrefix(keys.UITableID)
+	rowKey := keys.MakeFamilyKey(append([]byte(nil), tableKey...), 0)
+	value := bytes.Repeat([]byte("x"), 64<<10)
+	if err := store.DB().Put(context.Background(), rowKey, value); err != nil {
+		t.Fatal(err)
+	}
+
+	store.ForceSplitScanAndProcess()
+	testutils.SucceedsSoon(t, func() error {
+		repl := store.LookupReplica(tableKey, nil)
+		if repl.Desc().StartKey.Equal(tableKey) {
+			return nil
+		}
+		return errors.Errorf("waiting for split: %s", repl)
+	})
+
+	repl := store.LookupReplica(tableKey, nil)
+	origMaxBytes := repl.GetMaxBytes()
+	repl.SetMaxBytes(int64(len(value)))
+
+	// Wait for an attempt to split the range which will fail because it contains
+	// a single large value. The max-bytes for the range will be changed, but it
+	// should not have been reset to its original value.
+	store.ForceSplitScanAndProcess()
+	testutils.SucceedsSoon(t, func() error {
+		maxBytes := repl.GetMaxBytes()
+		if maxBytes != int64(len(value)) && maxBytes < origMaxBytes {
+			return nil
+		}
+		return errors.Errorf("expected max-bytes to be changed: %d", repl.GetMaxBytes())
+	})
+
+	// Add another row to the range. Are funky key construction requires this to
+	// be /Table/15.
+	table2Key := keys.MakeTablePrefix(keys.UITableID + 1)
+	row2Key := keys.MakeFamilyKey(table2Key, 0)
+	if err := store.DB().Put(context.Background(), row2Key, value); err != nil {
+		t.Fatal(err)
+	}
+
+	// Wait for the range to be split and verify that max-bytes was reset to the
+	// value in the zone config.
+	store.ForceSplitScanAndProcess()
+	testutils.SucceedsSoon(t, func() error {
+		if origMaxBytes == repl.GetMaxBytes() {
+			return nil
+		}
+		return errors.Errorf("expected max-bytes=%d, but got max-bytes=%d",
+			origMaxBytes, repl.GetMaxBytes())
+	})
+}

pkg/storage/engine/mvcc.go

@@ -2226,10 +2226,13 @@ func MVCCFindSplitKey(
 	bestSplitKey := encStartKey
 	bestSplitDiff := int64(math.MaxInt64)
 	var lastKey roachpb.Key
+	var n int
 
 	if err := engine.Iterate(encStartKey, encEndKey, func(kv MVCCKeyValue) (bool, error) {
-		// Is key within a legal key range?
-		valid := IsValidSplitKey(kv.Key.Key)
+		n++
+		// Is key within a legal key range? Note that we never choose the first key
+		// as the split key.
+		valid := n > 1 && IsValidSplitKey(kv.Key.Key)
 
 		// Determine if this key would make a better split than last "best" key.
 		diff := targetSize - sizeSoFar

pkg/storage/engine/mvcc_test.go

@@ -2990,8 +2990,11 @@ func TestFindSplitKey(t *testing.T) {
 			t.Fatal(err)
 		}
 		ind, _ := strconv.Atoi(string(humanSplitKey))
+		if ind == 0 {
+			t.Fatalf("%d: should never select first key as split key", i)
+		}
 		if diff := td.splitInd - ind; diff > 1 || diff < -1 {
-			t.Fatalf("%d. wanted key #%d+-1, but got %d (diff %d)", i, td.splitInd, ind, diff)
+			t.Fatalf("%d: wanted key #%d+-1, but got %d (diff %d)", i, td.splitInd, ind, diff)
 		}
 	}
 }

pkg/storage/replica_command.go

@@ -2601,7 +2601,7 @@ func (r *Replica) AdminSplit(
 		return roachpb.AdminSplitResponse{}, roachpb.NewErrorf("cannot split range with no key provided")
 	}
 	for retryable := retry.StartWithCtx(ctx, base.DefaultRetryOptions()); retryable.Next(); {
-		reply, pErr := r.adminSplitWithDescriptor(ctx, args, r.Desc())
+		reply, _, pErr := r.adminSplitWithDescriptor(ctx, args, r.Desc())
 		// On seeing a ConditionFailedError, retry the command with the
 		// updated descriptor.
 		if _, ok := pErr.GetDetail().(*roachpb.ConditionFailedError); !ok {
@@ -2631,7 +2631,7 @@ func (r *Replica) AdminSplit(
 // See the comment on splitTrigger for details on the complexities.
 func (r *Replica) adminSplitWithDescriptor(
 	ctx context.Context, args roachpb.AdminSplitRequest, desc *roachpb.RangeDescriptor,
-) (roachpb.AdminSplitResponse, *roachpb.Error) {
+) (_ roachpb.AdminSplitResponse, noSplitKey bool, _ *roachpb.Error) {
 	var reply roachpb.AdminSplitResponse
 
 	// Determine split key if not provided with args. This scan is
@@ -2648,27 +2648,30 @@ func (r *Replica) adminSplitWithDescriptor(
 			targetSize := r.GetMaxBytes() / 2
 			foundSplitKey, err = engine.MVCCFindSplitKey(ctx, snap, desc.RangeID, desc.StartKey, desc.EndKey, targetSize, nil /* logFn */)
 			if err != nil {
-				return reply, roachpb.NewErrorf("unable to determine split key: %s", err)
+				// Swallow the error and tell the caller that no split key could be
+				// found.
+				return reply, true, nil
 			}
 		} else if !r.ContainsKey(foundSplitKey) {
-			return reply, roachpb.NewError(roachpb.NewRangeKeyMismatchError(args.SplitKey, args.SplitKey, desc))
+			return reply, false,
+				roachpb.NewError(roachpb.NewRangeKeyMismatchError(args.SplitKey, args.SplitKey, desc))
 		}
 
 		foundSplitKey, err := keys.EnsureSafeSplitKey(foundSplitKey)
 		if err != nil {
-			return reply, roachpb.NewErrorf("cannot split range at key %s: %v",
+			return reply, true, roachpb.NewErrorf("cannot split range at key %s: %v",
 				args.SplitKey, err)
 		}
 
 		splitKey, err = keys.Addr(foundSplitKey)
 		if err != nil {
-			return reply, roachpb.NewError(err)
+			return reply, true, roachpb.NewError(err)
 		}
 		if !splitKey.Equal(foundSplitKey) {
-			return reply, roachpb.NewErrorf("cannot split range at range-local key %s", splitKey)
+			return reply, true, roachpb.NewErrorf("cannot split range at range-local key %s", splitKey)
 		}
 		if !engine.IsValidSplitKey(foundSplitKey) {
-			return reply, roachpb.NewErrorf("cannot split range at key %s", splitKey)
+			return reply, true, roachpb.NewErrorf("cannot split range at key %s", splitKey)
 		}
 	}
 
@@ -2677,14 +2680,15 @@ func (r *Replica) adminSplitWithDescriptor(
 	// otherwise it will cause infinite retry loop.
 	if desc.StartKey.Equal(splitKey) || desc.EndKey.Equal(splitKey) {
 		log.Event(ctx, "range already split")
-		return reply, nil
+		return reply, true, nil
 	}
 	log.Event(ctx, "found split key")
 
 	// Create right hand side range descriptor with the newly-allocated Range ID.
 	rightDesc, err := r.store.NewRangeDescriptor(splitKey, desc.EndKey, desc.Replicas)
 	if err != nil {
-		return reply, roachpb.NewErrorf("unable to allocate right hand side range descriptor: %s", err)
+		return reply, false,
+			roachpb.NewErrorf("unable to allocate right hand side range descriptor: %s", err)
 	}
 
 	// Init updated version of existing range descriptor.
@@ -2770,11 +2774,11 @@ func (r *Replica) adminSplitWithDescriptor(
 		// ConditionFailedError in the error detail so that the command can be
 		// retried.
 		if _, ok := err.(*roachpb.ConditionFailedError); ok {
-			return reply, roachpb.NewError(err)
+			return reply, false, roachpb.NewError(err)
 		}
-		return reply, roachpb.NewErrorf("split at key %s failed: %s", splitKey, err)
+		return reply, false, roachpb.NewErrorf("split at key %s failed: %s", splitKey, err)
 	}
-	return reply, nil
+	return reply, false, nil
 }
 
 // splitTrigger is called on a successful commit of a transaction

pkg/storage/split_queue.go

@@ -77,13 +77,7 @@ func (sq *splitQueue) shouldQueue(
 
 	// Add priority based on the size of range compared to the max
 	// size for the zone it's in.
-	zone, err := sysCfg.GetZoneConfigForKey(desc.StartKey)
-	if err != nil {
-		log.Error(ctx, err)
-		return
-	}
-
-	if ratio := float64(repl.GetMVCCStats().Total()) / float64(zone.RangeMaxBytes); ratio > 1 {
+	if ratio := float64(repl.GetMVCCStats().Total()) / float64(repl.GetMaxBytes()); ratio > 1 {
 		priority += ratio
 		shouldQ = true
 	}
@@ -96,7 +90,7 @@ func (sq *splitQueue) process(ctx context.Context, r *Replica, sysCfg config.Sys
 	desc := r.Desc()
 	if splitKey := sysCfg.ComputeSplitKey(desc.StartKey, desc.EndKey); splitKey != nil {
 		log.Infof(ctx, "splitting at key %v", splitKey)
-		if _, pErr := r.adminSplitWithDescriptor(
+		if _, _, pErr := r.adminSplitWithDescriptor(
 			ctx,
 			roachpb.AdminSplitRequest{
 				SplitKey: splitKey.AsRawKey(),
@@ -109,19 +103,28 @@ func (sq *splitQueue) process(ctx context.Context, r *Replica, sysCfg config.Sys
 	}
 
 	// Next handle case of splitting due to size.
-	zone, err := sysCfg.GetZoneConfigForKey(desc.StartKey)
-	if err != nil {
-		return err
-	}
 	size := r.GetMVCCStats().Total()
-	if float64(size)/float64(zone.RangeMaxBytes) > 1 {
-		log.Infof(ctx, "splitting size=%d max=%d", size, zone.RangeMaxBytes)
-		if _, pErr := r.adminSplitWithDescriptor(
+	maxBytes := r.GetMaxBytes()
+	if float64(size)/float64(maxBytes) > 1 {
+		log.Infof(ctx, "splitting size=%d max=%d", size, maxBytes)
+		if _, noSplitKey, pErr := r.adminSplitWithDescriptor(
 			ctx,
 			roachpb.AdminSplitRequest{},
 			desc,
 		); pErr != nil {
 			return pErr.GoError()
+		} else if noSplitKey {
+			// If we couldn't find a split key, set the max-bytes for the range to
+			// its current size to prevent future attempts to split the range until
+			// it grows again.
+			r.SetMaxBytes(size)
+		} else {
+			// We successfully split the range, reset max-bytes to the zone setting.
+			zone, err := sysCfg.GetZoneConfigForKey(desc.StartKey)
+			if err != nil {
+				return err
+			}
+			r.SetMaxBytes(zone.RangeMaxBytes)
 		}
 	}
 	return nil

pkg/storage/split_queue_test.go

@@ -53,27 +53,28 @@ func TestSplitQueueShouldQueue(t *testing.T) {
 	testCases := []struct {
 		start, end roachpb.RKey
 		bytes      int64
+		maxBytes   int64
 		shouldQ    bool
 		priority   float64
 	}{
 		// No intersection, no bytes.
-		{roachpb.RKeyMin, roachpb.RKey("/"), 0, false, 0},
+		{roachpb.RKeyMin, roachpb.RKey("/"), 0, 64 << 20, false, 0},
 		// Intersection in zone, no bytes.
-		{keys.MakeTablePrefix(2001), roachpb.RKeyMax, 0, true, 1},
+		{keys.MakeTablePrefix(2001), roachpb.RKeyMax, 0, 64 << 20, true, 1},
 		// Already split at largest ID.
-		{keys.MakeTablePrefix(2002), roachpb.RKeyMax, 0, false, 0},
+		{keys.MakeTablePrefix(2002), roachpb.RKeyMax, 0, 32 << 20, false, 0},
 		// Multiple intersections, no bytes.
-		{roachpb.RKeyMin, roachpb.RKeyMax, 0, true, 1},
+		{roachpb.RKeyMin, roachpb.RKeyMax, 0, 64 << 20, true, 1},
 		// No intersection, max bytes.
-		{roachpb.RKeyMin, roachpb.RKey("/"), 64 << 20, false, 0},
+		{roachpb.RKeyMin, roachpb.RKey("/"), 64 << 20, 64 << 20, false, 0},
 		// No intersection, max bytes+1.
-		{roachpb.RKeyMin, roachpb.RKey("/"), 64<<20 + 1, true, 1},
+		{roachpb.RKeyMin, roachpb.RKey("/"), 64<<20 + 1, 64 << 20, true, 1},
 		// No intersection, max bytes * 2.
-		{roachpb.RKeyMin, roachpb.RKey("/"), 64 << 21, true, 2},
+		{roachpb.RKeyMin, roachpb.RKey("/"), 64 << 21, 64 << 20, true, 2},
 		// Intersection, max bytes +1.
-		{keys.MakeTablePrefix(2000), roachpb.RKeyMax, 32<<20 + 1, true, 2},
+		{keys.MakeTablePrefix(2000), roachpb.RKeyMax, 32<<20 + 1, 32 << 20, true, 2},
 		// Split needed at table boundary, but no zone config.
-		{keys.MakeTablePrefix(2001), roachpb.RKeyMax, 32<<20 + 1, true, 1},
+		{keys.MakeTablePrefix(2001), roachpb.RKeyMax, 32<<20 + 1, 64 << 20, true, 1},
 	}
 
 	splitQ := newSplitQueue(tc.store, nil, tc.gossip)
@@ -94,6 +95,7 @@ func TestSplitQueueShouldQueue(t *testing.T) {
 				t.Fatal(err)
 			}
 			tc.repl.mu.state.Stats = ms
+			tc.repl.mu.maxBytes = test.maxBytes
 		}()
 
 		copy := *tc.repl.Desc()