This change is 

This looks good, but the tests are all failing. I think you also need to remove the call to MakeRowSentinelKey in keys/constants.go:247: SystemConfigSplitKey = MakeRowSentinelKey(TableDataMin)

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

pkg/ccl/sqlccl/restore.go, line 536 at r1 (raw file):

		// Pick the index such that it's 0 if len(splitPoints) == 1.
		splitIdx := len(splitPoints) / 2
		// AdminSplit requires that the key be a valid table key, which means

Now you can remove this comment too. splitKey is just splitPoints[splitIdx] and the same value can be used for both arguments to AdminSplit.

pkg/storage/replica_command.go, line 2642 at r1 (raw file):

		}

		// EnsureSafeSplitKey could have changed the key, so we must

Remove this section too.

pkg/storage/engine/mvcc.go, line 2313 at r1 (raw file):

	}

	// The key has been removed family ID now.

"The family ID has been removed from this key, making it a valid split point."

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

pkg/storage/engine/mvcc.go, line 2308 at r1 (raw file):

	}

	splitKey, err := keys.EnsureSafeSplitKey(bestSplitKey.Key)

I think this new code is the same as return keys.EnsureSafeSplitKey(bestSplitKey.Key).

Comments from Reviewable

@bdarnell I found a lot of error logs like

E170706 19:41:38.251592 49696 storage/queue.go:657  [split,s1,r4/1:/{System/tse-Max}] unable to split [n1,s1,r4/1:/{System/tse-
Max}] at key "/Table/0": storage/replica_command.go:2658: cannot split range at key /Table/0

if we remove MakeRowSentinelKey for SystemConfigSplitKey, will the split be invalid?

And all the test just panic: test timed out after 4m0s.

I think this line in IsValidSplitKey needs to be changed to use > instead of >=. We want to allow splits at either end of the NoSplitSpans, just not in the middle.

@bdarnell, I found it will report had 4 ranges at startup, expected 10 for tests in test_server_shim.go, I don't know what does this mean.

Need some help here. :)

The remaining issues are test-only problems. Two tests were using MakeRowSentinelKey where it is no longer necessary, and an assertion in sql/main_test.go was incorrectly reporting errors in many tests because it was using keys.Addr when it should not. I've added a commit that should fix these.

I had to rebase to pick up some build changes, and in the meantime another test was added that needed to be updated. This is ready to go now.

Could you expand the commit message to (1) include the issue reference and (2) describe the actual change?

Reviewed 1 of 5 files at r1, 12 of 12 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

pkg/ccl/sqlccl/restore.go, line 536 at r2 (raw file):

		// Pick the index such that it's 0 if len(splitPoints) == 1.
		splitIdx := len(splitPoints) / 2
		splitKey := append([]byte(nil), splitPoints[splitIdx]...)

why is this copy needed?

pkg/keys/constants.go, line 247 at r2 (raw file):

	// SystemConfigSplitKey is the key to split at immediately prior to the
	// system config span. NB: Split keys need to be valid column keys.
	SystemConfigSplitKey = []byte(TableDataMin)

why the different type?

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

	rowKey := roachpb.Key(encoding.EncodeVarintAscending(append([]byte(nil), tableKey...), 1))
	rowKey = encoding.EncodeStringAscending(encoding.EncodeVarintAscending(rowKey, 1), "a")
	col1Key, _ := keys.EnsureSafeSplitKey(keys.MakeFamilyKey(append([]byte(nil), rowKey...), 1))

why ignore these errors?

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

func TestEnsureSafeSplitKeyOutOfBounds(t *testing.T) {
	defer leaktest.AfterTest(t)()
	t.Skip()

did you mean to leave this in?

Comments from Reviewable

Review status: 10 of 14 files reviewed at latest revision, 7 unresolved discussions.

pkg/ccl/sqlccl/restore.go, line 536 at r2 (raw file):

I don't think it is any more. Deleted.

pkg/keys/constants.go, line 247 at r2 (raw file):

MakeRowSentinelKey returns a []byte, so this is the same as it was before this PR. It looks like it should be either a roachpb.Key or roachpb.RKey, but it's used inconsistently today. Added a TODO here and on MakeRowSentinelKey to change to a more precise type.

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

No good reason; added checks.

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

No, I meant to remove this whole test but forgot to push my last commit.

Comments from Reviewable

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

Comments from Reviewable

LGTM modulo commit message changes.

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

Comments from Reviewable

Rewrote commit message and added another commit which removes MakeRowSentinelKey entirely.

mod typo in first commit message:

MVCCFIndSplitKey

Should be MVCCFindSplitKey

Reviewed 15 of 15 files at r4.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

pkg/keys/keys.go, line 571 at r3 (raw file):

// SentinelFamilyID indicates that MakeFamilyKey should make a sentinel key.
const SentinelFamilyID = 0

since there are at least two comments that reference special behaviour associated with this value, perhaps keep this constant is worthwhile, if only to have a place to anchor comments.

pkg/sql/sqlbase/rowfetcher.go, line 399 at r4 (raw file):

	prettyKey = prettyKeyPrefix

	// If this is the row sentinel (in the legacy pre-family format),

isn't this also the case for column family zero, which is encoded without this value?

pkg/sql/sqlbase/table.go, line 1351 at r4 (raw file):

	}

	// Index keys are considered "sentinel" keys in that they do not have a

this comment is somewhat incongruent now

Comments from Reviewable

Review status: 10 of 25 files reviewed at latest revision, 10 unresolved discussions.

pkg/keys/keys.go, line 571 at r3 (raw file):

But the two places that referred to it were doing so for two separate reasons: column zero was a special sentinel in the pre-family encoding. With column families it's just a normal family with a small optimization in its key encoding.

pkg/sql/sqlbase/rowfetcher.go, line 399 at r4 (raw file):

No, family zero has values (including the entire table by default). The family-aware format doesn't go through processValueSingle (it uses processValueTuple instead).

Comments from Reviewable