cockroachkvs: use bytes.Compare when comparing key version

This patch uses bytes.Compare to compare key versions instead of
ComparePointSuffixes; the latter calls on
`normalizeEngineSuffixForCompare` - which could end up strip additional
bytes from the `suffix` (comparing only a prefix of the suffix) if the
last byte of the version happens to match one of the sentinel lengths
defined.

Fixes: #158463

Author: annrpom

cockroachkvs/cockroachkvs.go

@@ -789,7 +789,17 @@ func (ks *cockroachKeySeeker) IsLowerBound(k []byte, syntheticSuffix []byte) boo
 	// is either (a) unversioned (and sorts before all other suffixes) or (b) is
 	// a non-MVCC key with an untyped version.
 	if firstRowWall == 0 && firstLogical == 0 {
-		return ComparePointSuffixes(ks.untypedVersions.At(0), version) >= 0
+		firstRowUntypedVer := ks.untypedVersions.At(0)
+		// Empty suffixes sort before non-empty suffixes.
+		switch {
+		case len(version) == 0:
+			// This includes the case where both versions are empty.
+			return true
+		case len(firstRowUntypedVer) == 0:
+			return false
+		default:
+			return bytes.Compare(version, firstRowUntypedVer) >= 0
+		}
 	}
 
 	var wallTime uint64
@@ -825,7 +835,7 @@ func (ks *cockroachKeySeeker) IsLowerBound(k []byte, syntheticSuffix []byte) boo
 			buf[12] = 13
 			firstRowSuffix = buf[:13]
 		}
-		return ComparePointSuffixes(firstRowSuffix, version) >= 0
+		return bytes.Compare(version, firstRowSuffix[:len(firstRowSuffix)-1]) >= 0
 	}
 
 	// NB: The sign comparison is inverted because suffixes are sorted such that

cockroachkvs/testdata/key_schema_key_seeker

@@ -241,3 +241,62 @@ IsLowerBound(moo @ 3000000000,1, "") = true
 IsLowerBound(moo @ 3000000000,0, "") = true
 IsLowerBound(moo @ 0000000000,1, "") = true
 IsLowerBound(moo @ 02073a83c45688420eaf97824255790f1e12, "") = true
+
+# Regression test for cockroach/#158463. This test uses a lock table key (a
+# non-MVCC key with an untyped version) as the first key in the block.
+# Previously, we would incorrectly use the key version as a full suffix and
+# call ComparePointSuffixes with it.
+
+define-block
+moo @ 02073a83c45688420eaf97824255790f1e
+moo @ 3000000002,1
+moo @ 3000000001,1
+moo @ 0000000001,0
+----
+Parse(moo @ 02073a83c45688420eaf97824255790f1e) = hex:6d6f6f0002073a83c45688420eaf97824255790f1e12
+Parse(moo @ 3000000002,1) = hex:6d6f6f0000000000b2d05e02000000010d
+Parse(moo @ 3000000001,1) = hex:6d6f6f0000000000b2d05e01000000010d
+Parse(moo @ 0000000001,0) = hex:6d6f6f00000000000000000109
+
+is-lower-bound
+moo @ 03073a83c45688420eaf97824255790f1e
+moo @ 9000000000,2
+moo @ 8000000000,2
+moo @ 8000000000,1
+moo @ 8000000000,0
+moo @ 7000000000,9
+moo @ 3000000000,2
+moo @ 3000000000,1
+moo @ 3000000000,0
+moo @ 0000000000,1
+----
+IsLowerBound(moo @ 03073a83c45688420eaf97824255790f1e, "") = true
+IsLowerBound(moo @ 9000000000,2, "") = false
+IsLowerBound(moo @ 8000000000,2, "") = false
+IsLowerBound(moo @ 8000000000,1, "") = false
+IsLowerBound(moo @ 8000000000,0, "") = false
+IsLowerBound(moo @ 7000000000,9, "") = false
+IsLowerBound(moo @ 3000000000,2, "") = false
+IsLowerBound(moo @ 3000000000,1, "") = false
+IsLowerBound(moo @ 3000000000,0, "") = false
+IsLowerBound(moo @ 0000000000,1, "") = false
+
+# Test case for key with non-standard version length.
+
+define-block
+moo @ 3000000000,1
+moo @ 3000000002,1
+moo @ 3000000001,1
+----
+Parse(moo @ 3000000000,1) = hex:6d6f6f0000000000b2d05e00000000010d
+Parse(moo @ 3000000002,1) = hex:6d6f6f0000000000b2d05e02000000010d
+Parse(moo @ 3000000001,1) = hex:6d6f6f0000000000b2d05e01000000010d
+
+is-lower-bound
+moo @ 0102030405
+moo @ ffffffffffffffffffffffffffffffff
+moo @ 00
+----
+IsLowerBound(moo @ 0102030405, "") = true
+IsLowerBound(moo @ ffffffffffffffffffffffffffffffff, "") = true
+IsLowerBound(moo @ 00, "") = false