backupccl: allow rewriting spans keys that may not decode as keys

[dt]

backupccl: allow rewriting spans keys that may not decode as keys

When rewriting the spans to pre-split and distribute work in RESTORE, we
use the key rewriter to rewrite span boundaries. This usually works
well, but in a few cases, valid span boundaries may no longer be
entirely made of valid value encodings. For example, .PrefixEnd alters
the trailing bytes to increment them, but this will break decoding of
byte string that assumes a very specific trailing terminator.

PeekLength, and key decoding in general, can fail when reading the last
value from a key that is coming from a span. Keys in spans are often
altered e.g. by calling Next() or PrefixEnd() to ensure a given span is
inclusive or for other reasons, but the manipulations sometimes change
the encoded bytes, meaning they can no longer successfully decode as
back to the original values. This is OK when span boundaries mostly are
only required to be even divisions of keyspace, but when we try to go
back to interpreting them as keys, it can fall apart. Partitioning a
table (and applying zone configs) eagerly creates splits at the defined
partition boundaries, using PrefixEnd for their ends, resulting in such
spans.

Fortunately, the only common span manipulations are to the trailing byte
of a key (e.g. incrementing or appending a null) so for our needs here,
if we fail to decode because of one fo those manipulations, we can
assume that we are at the end of the key as far as fields where a table
ID which needs to be replaced can appear and consider the rewrite of
this key as being completed successfully.

Finally unlike key rewrites of actual row-data, span rewrites do not
need to be perfect: spans are only rewritten for use in pre-splitting
and work distribution, so even if it turned out that this assumption was
incorrect, it could cause a performance degradation but does not pose a
correctness risk.

Release note (bug fix): fix an issue that prevented restoring some
backups if they included tables that were partitioned by columns of a
certain types while also interleaved by child tables.

[cockroach-teamcity]

This change is 

[dt]

I think this is the third time that trying to decode spans as keys has confused RESTORE. I wonder if given that spans are only really needed for pre-splitting / sharing work evenly during RESTORE, we should allow RESTORE to continue even if they fail to rewrite so that you can at least still restore, though maybe slower. Though I fear that if we did that, it'd probably rot.

[danhhz]

I don't have all this loaded up anymore, but is it possible to undo a PrefixEnd? If you hit an encoding error, you could attempt to undo a PrefixEnd and do it again. Maybe you'd also have to try to undo a Next

[dt]

@danhhz Yep, that was exactly my first thought too and I started with a patch using that approach (there's an UndoPrefixEnd in encoding). However working though that approach, I ended up changing my mind as I went, switching to this approach instead.

Part of it is that 1) you can't always undo PrefixEnd since can throw away bytes after it carries its increment and 2) I couldn't convince myself that operating on the post-undo key was always correct and then 3) we can't even be sure a key was actually PrefixEnd'ed to start with. On top of those, there could be other things done to spans now or in the future that break this, or other encodings (which could also be added without this code and tests noticing). Given all those (in particular 1), I ended up concluding we'd still want to handle a non-decodable suffix, so I was back where I started.

All of that got thinking that baking assumptions into this code about spans and key encoding was making this brittle, and going further down that path seemed likely to be difficult to maintain. I tried to think about what we actually need from this code: a corse partitioning of the target keyspace for pre-splitting. Span bounds at least have a prefix of valid key encoding, since they serve to partition the key space of valid key byte strings, and if we can rewrite that prefix, we should end up with enough of a partitioning of the target keyspace.

[dt]

based on offline discussion with @danhhz I think we concluded this is good to go for now -- longer term we might want a more general solution to avoid trying to decode spans entirely in the interest of making this less brittle, but for now this change should fix the known bug.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)