backupccl: stop splitting at invalid split keys

[pbardea]

Restore could perform invalid splits during the split and scatter phase.
This commit ensures that splits are always performed at safe keys.

Fixes #67488.

Release note (bug fix): Fix a bug where restores of data with multiple
column families could be split illegally (within a single SQL row). This
could result in temporary data unavailability until the ranges on either
side of the invalid split are merged.

[cockroach-teamcity]

This change is 

[pbardea]

Working on the tests.

I believe that this needs to go back to 20.1.

[jordanlewis]

Glad the fix was so small. Are there tests cases we can add that are more general that prevent us from making this mistake in the future? Why does the AdminSplit command permit invalid SQL split keys at all?

[pbardea]

Are there tests cases we can add that are more general that prevent us from making this mistake in the future?

I think there's defense that we can do here in layers. The first is a regression case that creates a backup with entries split along these boundaries.

I'm still thinking of more general test cases we can add. We would benefit with more backup/restore testing with column families that operate on larger-data sizes (so that there are several entries to work with). I think a combination of updating the default bank payload that we use for the tests to incorporate column families and also reducing the size of the entries produced by backup (metamorphic constant?) could help activate paths that could have caught something like this.

Why does the AdminSplit command permit invalid SQL split keys at all?

I looked into ensuring that AdminSplit validates invalid split keys by using EnsureSafeSplitKey, but EnsureSafeSplitKey already knows quite a bit about the SQL layer which feels very leaky for the KV layer to know so much about decoding SQL rows... That being said, it's already used by storage.MVCCFindSplitKey, which in turn is used by size-based splits, so it feels like that ship has already sailed.

[pbardea]

@adityamaru This change is pretty much the same that David stamped above with an added test that I wanted to get eyes on. It also ignores any errors from EnsureSafeSplitKey since not all keys are "table keys" (e.g. /Table/51 will produce an error from EnsureSafeSplitKey but is fine to split at). We similarly ignore it in SST_Batcher.

I think a change like this is safer to backport, but a follow up will be to try and get this check into AdminSplit.

[pbardea]

TFTR
bors r+

[craig]

Build failed:

• GitHub CI (Cockroach)

[pbardea]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[adityamaru]

blathers backport 21.1