keys: EnsureSafeSplitKey is too strict

[dt]

EnsureSafeSplitKey, and the GetRowPrefixLength function that implements it, today typically insists that a key have what appears to be a valid column family ID as its suffix, so that it can decode its length then use that to truncate the family ID from the row key and return it. It has special cases for keys that consist of only a table ID, or only a table and index ID, which it also considers to be have a valid "RowPrefixLength" of their entire length.

However consider a table with a primary key like (region string, id int). A key like /Table/101/1/"us-east" is very much a valid split key for this table -- probably a very good one at that -- but it does not end in a valid column family ID+length, but also does not consist of only a table ID or table and index ID, so it causes EnsureSafeSplitKey to return an error.

Should EnsureSafeSplitKey instead assume that any key which does not appear to end in a column family ID is a safe split key?

Doing so would more correctly handle prefixes like this, particularly when handling compound primary keys (though like the current implementation, it would still suffer from cases of mistaken identity where an indexed integer value is mistaken for a column family ID). However, if someone were to append bytes to a column family key - by calling Next or PrefixEnd -- the resulting key, which is not a safe split keys it still falls between two column family keys -- would no longer appear to be a column family key, and thus would be erroneously considered to be a safe split key. Thus switching the default assumption of EnsureSafeSplitKey, to be that a key is a safe split key unless it appears to end with a column family ID, would require being sure that they keys passed to it are not derived by calling Next or PrefixEnd on real keys.

A more ambitious option might be to fix the underlying problem of being unable to tell a when a key contains a column family or not, by switching the column family ID to be encoded with a unique wire tag instead of Int. However unless all KVs in all clusters were actively rewritten in a migration to use the new tag, we would still need to fall back to the current guessing approach, so this may not be worth the churn.

Jira issue: CRDB-31122

[dt]

It would be good to better understand the potential impact of this pessimistic approach in EnsureSafeSplitKey, both in the case where it errors and in the case where it mistakes an indexed integer in a user column for a family ID and performs truncation using it.

One callsite noted is the DistSQLPhysicalPlanner, where an error, or dramatic change of prefix, could change how spans are partitioned (potentially pilling work on a single processor that should have been distributed?).

[dt]

I sketched out a potential alternative encoding tag for column family IDs over in #112702

[michae2]

(See also #43094.)