keys: Clean up MakeRowSentinelKey and EnsureSafeSplitKey

[bdarnell]

Our mechanisms for avoiding splits in the middle of a row are a bit of a mess (see #16008 (comment)). AdminSplit does not split at the requested key: if the key points to the /Table keyspace then it removes a trailing integer from the key which is assumed to be the column family ID. This is fine when splitting at a key that actually exists (which is true for all size-based splits), but it is difficult to synthesize keys that work correctly in this model (e.g. at table boundaries).

We currently use MakeRowSentinelKey to add a dummy column family ID with a value of 0 (in config.go and restore.go), but this is not really correct. It works OK in config.go where it applies at the table level (and the added zero sorts before all the index IDs that are the normal inhabitants of this level), but in restore.go this zero is intermingled with actual row data, and it sorts higher than any negative values that may be stored in the primary key (assuming an integer PK).

We should fix this by making AdminSplit act on the requested key as given. The key selection process is responsible for choosing a key that is not in the middle of a row. That is, MVCCFindSplitKey and the SQL SPLIT AT command should remove the column family ID from the keys that they use, and splits at the boundaries of tables and indexes should not add them.

Compatibility notes: Splits performed by the split queue are always executed on the same node that selects the keys, so the split queue is safe in mixed-version environments as this change rolls out. The SPLIT AT and RESTORE commands execute splits remotely, which requires some care (or a warning in the release notes that these commands may not be used during a rolling upgrade).

[a6802739]

@bdarnell Maybe I could have a try for this.

[bdarnell]

@a6802739 Go ahead!

[a6802739]

@bdarnell, So we need to cleanup the usage of MakeRowSentinelKey?

I have a confusion here, after we call MakeRowSentinelKey for the splitKey, EnsureSafeSplitKey guarantee remove the family ID, so why should we need to remove the usage of MakeRowSentinelKey ?

[bdarnell]

The problem is that the way we're using EnsureSafeSplitKey, it removes something, but it's not always the family ID. We want to get rid of most uses of both of these functions. I think what we want to do is remove the calls to MakeRowSentinelKey from config.go and restore.go and the call to EnsureSafeSplitKey in AdminSplit. MVCCFindSplitKey should call both MakeRowSentinelKey and EnsureSafeSplitKey to get a valid split key, and the SQL SPLIT AT statement also needs to do call EnsureSafeSplitKey (I think. I'm less familiar with this code).

[a6802739]

@bdarnell So MVCCFindSplitKey should call EnsureSafeSplitKey to get the row key(removed family ID) and use MakeRowSentinelKey to get the RowSentinelKey(add family ID 0 in the tail)?

[bdarnell]

I said earlier that MVCCFindSplitKey should do both, but now I think that was a mistake. MVCCFindSplitKey should only call EnsureSafeSplitKey to remove the family ID from the key on disk.