kv: stop decoding synthetic timestamp bit from mvcc keys

Informs #101938.

This commit removes logic in mvcc key decoding routines to decode
synthetic timestamps. We retain the ability to decode keys with the
synthetic timestamp bit set, but we simply ignore its presence.

As discussed in the previous commit, the role of these synthetic
timestamp markers was eliminated in #80706 by the local_timestamp field
in the mvcc value header, which was first present in v22.2. v23.2 does
not require compatibility with v22.2, so it can rely on the fact that
any txn that has a synthetic timestamp (because it writes in the future)
will also write local timestamps into each of its values.

Release note: None

pkg/storage/engine_key.go

@@ -31,7 +31,7 @@ import (
 // should consider changing all the legacy code).
 //
 // The version can have the following lengths in addition to 0 length.
-// - Timestamp of MVCC keys: 8 or 12 bytes.
+// - Timestamp of MVCC keys: 8, 12, or 13 bytes.
 // - Lock table key: 17 bytes.
 type EngineKey struct {
 	Key     roachpb.Key
@@ -153,13 +153,11 @@ func (k EngineKey) ToMVCCKey() (MVCCKey, error) {
 		// No-op.
 	case engineKeyVersionWallTimeLen:
 		key.Timestamp.WallTime = int64(binary.BigEndian.Uint64(k.Version[0:8]))
-	case engineKeyVersionWallAndLogicalTimeLen:
+	case engineKeyVersionWallAndLogicalTimeLen, engineKeyVersionWallLogicalAndSyntheticTimeLen:
 		key.Timestamp.WallTime = int64(binary.BigEndian.Uint64(k.Version[0:8]))
 		key.Timestamp.Logical = int32(binary.BigEndian.Uint32(k.Version[8:12]))
-	case engineKeyVersionWallLogicalAndSyntheticTimeLen:
-		key.Timestamp.WallTime = int64(binary.BigEndian.Uint64(k.Version[0:8]))
-		key.Timestamp.Logical = int32(binary.BigEndian.Uint32(k.Version[8:12]))
-		key.Timestamp.Synthetic = k.Version[12] != 0
+		// NOTE: byte 13 used to store the timestamp's synthetic bit, but this is no
+		// longer consulted and can be ignored during decoding.
 	default:
 		return MVCCKey{}, errors.Errorf("version is not an encoded timestamp %x", k.Version)
 	}

pkg/storage/engine_key_test.go

@@ -132,12 +132,14 @@ func TestMVCCAndEngineKeyEncodeDecode(t *testing.T) {
 
 // TestMVCCAndEngineKeyDecodeSyntheticTimestamp tests decoding an MVCC key with
 // a synthetic timestamp. The synthetic timestamp bit is now ignored during key
-// encoding, but synthetic timestamps may still be present in the wild, so they
-// must be decoded.
+// encoding and decoding, but synthetic timestamps may still be present in the
+// wild, so they must not confuse decoding.
 func TestMVCCAndEngineKeyDecodeSyntheticTimestamp(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
 	key := MVCCKey{Key: roachpb.Key("bar"), Timestamp: hlc.Timestamp{WallTime: 99, Logical: 45, Synthetic: true}}
+	keyNoSynthetic := key
+	keyNoSynthetic.Timestamp.Synthetic = false
 
 	// encodedStr was computed from key using a previous version of the code that
 	// that included synthetic timestamps in the MVCC key encoding.
@@ -153,7 +155,7 @@ func TestMVCCAndEngineKeyDecodeSyntheticTimestamp(t *testing.T) {
 	require.NoError(t, eKeyDecoded.Validate())
 	keyDecoded, err := eKeyDecoded.ToMVCCKey()
 	require.NoError(t, err)
-	require.Equal(t, key, keyDecoded)
+	require.Equal(t, keyNoSynthetic, keyDecoded)
 }
 
 func TestEngineKeyValidate(t *testing.T) {

pkg/storage/enginepb/decode.go

@@ -59,13 +59,11 @@ func DecodeKey(encodedKey []byte) ([]byte, hlc.Timestamp, error) {
 		// No-op.
 	case 8:
 		timestamp.WallTime = int64(binary.BigEndian.Uint64(encodedTS[0:8]))
-	case 12:
+	case 12, 13:
 		timestamp.WallTime = int64(binary.BigEndian.Uint64(encodedTS[0:8]))
 		timestamp.Logical = int32(binary.BigEndian.Uint32(encodedTS[8:12]))
-	case 13:
-		timestamp.WallTime = int64(binary.BigEndian.Uint64(encodedTS[0:8]))
-		timestamp.Logical = int32(binary.BigEndian.Uint32(encodedTS[8:12]))
-		timestamp.Synthetic = encodedTS[12] != 0
+		// NOTE: byte 13 used to store the timestamp's synthetic bit, but this is no
+		// longer consulted and can be ignored during decoding.
 	default:
 		return nil, hlc.Timestamp{}, errors.Errorf(
 			"invalid encoded mvcc key: %x bad timestamp %x", encodedKey, encodedTS)

pkg/storage/mvcc_key.go

@@ -176,6 +176,11 @@ func (k MVCCKey) Len() int {
 //
 // The sentinel byte can be used to detect a key without a timestamp, since
 // timeLength will never be 0 (it includes itself in the length).
+//
+// The timeSynthetic form is no longer written by the current version of the
+// code, but can be encountered in the wild until we migrate it away. Until
+// then, decoding routines must be prepared to handle it, but can ignore the
+// synthetic bit.
 func EncodeMVCCKey(key MVCCKey) []byte {
 	keyLen := encodedMVCCKeyLength(key)
 	buf := make([]byte, keyLen)
@@ -348,14 +353,11 @@ func decodeMVCCTimestamp(encodedTS []byte) (hlc.Timestamp, error) {
 		// No-op.
 	case 8:
 		ts.WallTime = int64(binary.BigEndian.Uint64(encodedTS[0:8]))
-	case 12:
-		ts.WallTime = int64(binary.BigEndian.Uint64(encodedTS[0:8]))
-		ts.Logical = int32(binary.BigEndian.Uint32(encodedTS[8:12]))
-	case 13:
+	case 12, 13:
 		ts.WallTime = int64(binary.BigEndian.Uint64(encodedTS[0:8]))
 		ts.Logical = int32(binary.BigEndian.Uint32(encodedTS[8:12]))
-		// TODO(nvanbenschoten): stop writing Synthetic timestamps in v23.1.
-		ts.Synthetic = encodedTS[12] != 0
+		// NOTE: byte 13 used to store the timestamp's synthetic bit, but this is no
+		// longer consulted and can be ignored during decoding.
 	default:
 		return hlc.Timestamp{}, errors.Errorf("bad timestamp %x", encodedTS)
 	}

pkg/storage/mvcc_key_test.go

@@ -259,23 +259,28 @@ func TestDecodeUnnormalizedMVCCKey(t *testing.T) {
 			equalToNormal: false,
 		},
 		"synthetic": {
-			encoded:       "666f6f00000000000000000000000000010e",
-			expected:      MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 0, Logical: 0, Synthetic: true}},
-			equalToNormal: true,
+			encoded: "666f6f00000000000000000000000000010e",
+			// Synthetic bit set to true when decoded by older versions of the code.
+			expected: MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 0, Logical: 0}},
+			// See comment above on "zero walltime and logical".
+			equalToNormal: false,
 		},
 		"walltime and synthetic": {
-			encoded:       "666f6f0016cf10bc0505574100000000010e",
-			expected:      MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 1643550788737652545, Logical: 0, Synthetic: true}},
+			encoded: "666f6f0016cf10bc0505574100000000010e",
+			// Synthetic bit set to true when decoded by older versions of the code.
+			expected:      MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 1643550788737652545, Logical: 0}},
 			equalToNormal: true,
 		},
 		"logical and synthetic": {
-			encoded:       "666f6f0000000000000000000000ffff010e",
-			expected:      MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 0, Logical: 65535, Synthetic: true}},
+			encoded: "666f6f0000000000000000000000ffff010e",
+			// Synthetic bit set to true when decoded by older versions of the code.
+			expected:      MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 0, Logical: 65535}},
 			equalToNormal: true,
 		},
 		"walltime, logical, and synthetic": {
-			encoded:       "666f6f0016cf10bc050557410000ffff010e",
-			expected:      MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 1643550788737652545, Logical: 65535, Synthetic: true}},
+			encoded: "666f6f0016cf10bc050557410000ffff010e",
+			// Synthetic bit set to true when decoded by older versions of the code.
+			expected:      MVCCKey{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 1643550788737652545, Logical: 65535}},
 			equalToNormal: true,
 		},
 	}
@@ -382,7 +387,6 @@ func BenchmarkDecodeMVCCKey(b *testing.B) {
 		"empty":            {},
 		"walltime":         {WallTime: 1643550788737652545},
 		"walltime+logical": {WallTime: 1643550788737652545, Logical: 4096},
-		"all":              {WallTime: 1643550788737652545, Logical: 4096, Synthetic: true},
 	}
 	var mvccKey MVCCKey
 	var err error

pkg/storage/mvcc_value.go

@@ -106,17 +106,6 @@ func (v MVCCValue) LocalTimestampNeeded(keyTS hlc.Timestamp) bool {
 // provided key version timestamp and returned.
 func (v MVCCValue) GetLocalTimestamp(keyTS hlc.Timestamp) hlc.ClockTimestamp {
 	if v.LocalTimestamp.IsEmpty() {
-		if keyTS.Synthetic {
-			// A synthetic version timestamp means that the version timestamp is
-			// disconnected from real time and did not come from an HLC clock on the
-			// leaseholder that wrote the value or from somewhere else in the system.
-			// As a result, the version timestamp cannot be cast to a clock timestamp,
-			// so we return min_clock_timestamp instead. The effect of this is that
-			// observed timestamps can not be used to avoid uncertainty retries for
-			// values without a local timestamp and with a synthetic version
-			// timestamp.
-			return hlc.MinClockTimestamp
-		}
 		return hlc.ClockTimestamp(keyTS)
 	}
 	return v.LocalTimestamp

pkg/storage/mvcc_value_test.go

@@ -59,18 +59,16 @@ func TestMVCCValueGetLocalTimestamp(t *testing.T) {
 	ts0 := hlc.Timestamp{Logical: 0}
 	ts1 := hlc.Timestamp{Logical: 1}
 	ts2 := hlc.Timestamp{Logical: 2}
-	ts2S := hlc.Timestamp{Logical: 2, Synthetic: true}
 
 	testcases := map[string]struct {
 		localTs   hlc.Timestamp
 		versionTs hlc.Timestamp
 		expect    hlc.Timestamp
 	}{
-		"no local timestamp":                    {ts0, ts2, ts2},
-		"no local timestamp, version synthetic": {ts0, ts2S, hlc.MinTimestamp},
-		"smaller local timestamp":               {ts1, ts2, ts1},
-		"equal local timestamp":                 {ts2, ts2, ts2},
-		"larger local timestamp":                {ts2, ts1, ts2},
+		"no local timestamp":      {ts0, ts2, ts2},
+		"smaller local timestamp": {ts1, ts2, ts1},
+		"equal local timestamp":   {ts2, ts2, ts2},
+		"larger local timestamp":  {ts2, ts1, ts2},
 	}
 	for name, tc := range testcases {
 		t.Run(name, func(t *testing.T) {