storage: avoid unsplittable range

[petermattis]

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 #9555

[cockroach-teamcity]

This change is 

[petermattis]

This needs a test. I'll get to it tomorrow.

[spencerkimball]

I'm worried about the following situation: we have a large row because there are many versions being written in the past 24 hours for a key. Versions continue to be written second by second, each of which triggers the maybeAdd call to the split queue. Just setting replica.mu.maxBytes to the current size means that we will continue to get log spam and do the work of scanning through the range on every write to the range, all with no hope of splitting the range.

[petermattis]

I'm worried about the following situation: we have a large row because there are many versions being written in the past 24 hours for a key. Versions continue to be written second by second, each of which triggers the maybeAdd call to the split queue. Just setting replica.mu.maxBytes to the current size means that we will continue to get log spam and do the work of scanning through the range on every write to the range, all with no hope of splitting the range.

The scenario you describe is likely to create a range that is much larger than 64MB. I'm pretty sure we'll have other more pressing problems than the log spam if we have such a large range.

[petermattis]

Test added which revealed a problem in MVCCFindSplitKey. Consider a range with a max-size of 64KB. Adding a single 64KB row will cause the range to be narrowed to include just that row. Now add another 64KB row to the range and we'll attempt to split again. The target size we pass to MVCCFindSplitKey will be ~32KB which will cause that method to select the first key in the range as the split key but we can't split on that key because that is already the key we're split on. I adjusted MVCCFindSplitKey to never return the first key as the split key. I still need to enhance TestFindSplitKey to verify this behavior.

[petermattis]

The enhancement to TestFindSplitKey was trivial. This is ready for a look.

[bdarnell]

Reviewed 3 of 6 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/storage/client_split_test.go, line 2001 at r2 (raw file):

	})

	// Add another row to the range. Are funky key construction requires this to

s/Are/Our/

The funkiness is just in this test, right? It would be more natural if we used two actual row keys instead of writing the giant row to the table prefix with no PK values (especially since the table prefix keys are already special for splits)

---

pkg/storage/replica_command.go, line 2634 at r2 (raw file):

func (r *Replica) adminSplitWithDescriptor(
	ctx context.Context, args roachpb.AdminSplitRequest, desc *roachpb.RangeDescriptor,
) (_ roachpb.AdminSplitResponse, noSplitKey bool, _ *roachpb.Error) {

Negating this return value feels unnatural to me.

---

pkg/storage/replica_command.go, line 2652 at r2 (raw file):

				ctx, snap, desc.RangeID, desc.StartKey, desc.EndKey, targetSize)
			if err != nil {
				// Swallow the error and tell the caller that no split key could be

Swallowing all errors here seems like a bad idea; I think it would be better to either make "no valid splits" a separate error type or make it not an error at all (return nil, nil, since nil will never be a valid split key)

---

pkg/storage/split_queue.go, line 120 at r2 (raw file):

			// its current size to prevent future attempts to split the range until
			// it grows again.
			r.SetMaxBytes(size)

I think that adding some margin here makes sense so we don't re-enqueue the range on every subsequent write.

---

Comments from Reviewable

[petermattis]

Review status: 1 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/storage/client_split_test.go, line 2001 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

s/Are/Our/

The funkiness is just in this test, right? It would be more natural if we used two actual row keys instead of writing the giant row to the table prefix with no PK values (especially since the table prefix keys are already special for splits)

Yes, the funkiness is just in this test. Played with this some more and was able to fix it so that we use 3 rows in the same table.

---

pkg/storage/replica_command.go, line 2634 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Negating this return value feels unnatural to me.

Inverted.

---

pkg/storage/replica_command.go, line 2652 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Swallowing all errors here seems like a bad idea; I think it would be better to either make "no valid splits" a separate error type or make it not an error at all (return nil, nil, since nil will never be a valid split key)

Done. I've made no valid splits not an error and instead return

---

pkg/storage/split_queue.go, line 120 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think that adding some margin here makes sense so we don't re-enqueue the range on every subsequent write.

I'm not against doing that, but I don't think it will matter in practice. Changed this to size * 2 which required changing the test to write 2 new rows.

---

Comments from Reviewable

[bdarnell]

---

Reviewed 2 of 5 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

---

pkg/storage/client_split_test.go, line 1986 at r4 (raw file):

	repl := store.LookupReplica(tableKey, nil)
	fmt.Printf("replica %s\n", repl)

Remove this.

---

Comments from Reviewable

[petermattis]

Review status: 2 of 6 files reviewed at latest revision, 5 unresolved discussions.

---

pkg/storage/client_split_test.go, line 1986 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Remove this.

Done.

---

Comments from Reviewable