Skip to content

Commit

Permalink
Merge #105523
Browse files Browse the repository at this point in the history 
105523: kv: stop encoding or decoding synthetic timestamp bit in/from mvcc keys r=sumeerbhola a=nvanbenschoten

Informs #101938.

This first commit removes logic in mvcc key encoding routines that handle synthetic timestamps. As a result, we no longer write keys with synthetic timestamps, though we retain the ability to decode them.

The second 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 described in #72121 (comment) and later in 24c56df (see "Future improvements"), the introduction of the mvcc value header and the optional, per-version local timestamp paved the way for the removal of synthetic timestamps. MVCC keys no longer need to carry the synthetic bit in order for reads from GLOBAL tables to behave properly. As a result, we no longer need to write it.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
  • Loading branch information
@nvanbenschoten
craig[bot] and nvanbenschoten committed Jul 10, 2023
2 parents 3ca7382 + 3773994 commit a61471c
Show file tree
Hide file tree
Showing 10 changed files with 81 additions and 3,134 deletions.
1 change: 0 additions & 1 deletion pkg/storage/batch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,6 @@ func TestDecodeKey(t *testing.T) {
{Key: []byte("foo")},
{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 1}},
{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 1, Logical: 1}},
{Key: []byte("foo"), Timestamp: hlc.Timestamp{WallTime: 1, Logical: 1, Synthetic: true}},
}
for _, test := range tests {
t.Run(test.String(), func(t *testing.T) {
Expand Down
10 changes: 4 additions & 6 deletions pkg/storage/engine_key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down
31 changes: 29 additions & 2 deletions pkg/storage/engine_key_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
package storage

import (
"encoding/hex"
"fmt"
"math/rand"
"testing"
Expand Down Expand Up @@ -88,7 +89,6 @@ func TestMVCCAndEngineKeyEncodeDecode(t *testing.T) {
{key: MVCCKey{Key: roachpb.Key("a")}},
{key: MVCCKey{Key: roachpb.Key("glue"), Timestamp: hlc.Timestamp{WallTime: 89999}}},
{key: MVCCKey{Key: roachpb.Key("foo"), Timestamp: hlc.Timestamp{WallTime: 99, Logical: 45}}},
{key: MVCCKey{Key: roachpb.Key("bar"), Timestamp: hlc.Timestamp{WallTime: 99, Logical: 45, Synthetic: true}}},
}
for _, test := range testCases {
t.Run("", func(t *testing.T) {
Expand Down Expand Up @@ -130,6 +130,34 @@ 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 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.
encodedStr := "6261720000000000000000630000002d010e"
encoded, err := hex.DecodeString(encodedStr)
require.NoError(t, err)

// Decode to demonstrate that the synthetic timestamp can be decoded.
eKeyDecoded, ok := DecodeEngineKey(encoded)
require.True(t, ok)
require.False(t, eKeyDecoded.IsLockTableKey())
require.True(t, eKeyDecoded.IsMVCCKey())
require.NoError(t, eKeyDecoded.Validate())
keyDecoded, err := eKeyDecoded.ToMVCCKey()
require.NoError(t, err)
require.Equal(t, keyNoSynthetic, keyDecoded)
}

func TestEngineKeyValidate(t *testing.T) {
defer leaktest.AfterTest(t)()
uuid1 := uuid.Must(uuid.FromString("6ba7b810-9dad-11d1-80b4-00c04fd430c8"))
Expand All @@ -141,7 +169,6 @@ func TestEngineKeyValidate(t *testing.T) {
{key: MVCCKey{Key: roachpb.Key("a")}},
{key: MVCCKey{Key: roachpb.Key("glue"), Timestamp: hlc.Timestamp{WallTime: 89999}}},
{key: MVCCKey{Key: roachpb.Key("foo"), Timestamp: hlc.Timestamp{WallTime: 99, Logical: 45}}},
{key: MVCCKey{Key: roachpb.Key("bar"), Timestamp: hlc.Timestamp{WallTime: 99, Logical: 45, Synthetic: true}}},

// Valid LockTableKeys.
{
Expand Down
8 changes: 3 additions & 5 deletions pkg/storage/enginepb/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
25 changes: 10 additions & 15 deletions pkg/storage/mvcc_key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -280,11 +285,8 @@ func EncodeMVCCTimestampToBuf(buf []byte, ts hlc.Timestamp) []byte {
// buffer must have the correct size, and the timestamp must not be empty.
func encodeMVCCTimestampToBuf(buf []byte, ts hlc.Timestamp) {
binary.BigEndian.PutUint64(buf, uint64(ts.WallTime))
if ts.Logical != 0 || ts.Synthetic {
if ts.Logical != 0 {
binary.BigEndian.PutUint32(buf[mvccEncodedTimeWallLen:], uint32(ts.Logical))
if ts.Synthetic {
buf[mvccEncodedTimeWallLen+mvccEncodedTimeLogicalLen] = 1
}
}
}

Expand All @@ -296,12 +298,8 @@ func encodedMVCCKeyLength(key MVCCKey) int {
keyLen := len(key.Key) + mvccEncodedTimeSentinelLen
if !key.Timestamp.IsEmpty() {
keyLen += mvccEncodedTimeWallLen + mvccEncodedTimeLengthLen
if key.Timestamp.Logical != 0 || key.Timestamp.Synthetic {
if key.Timestamp.Logical != 0 {
keyLen += mvccEncodedTimeLogicalLen
if key.Timestamp.Synthetic {
// TODO(nvanbenschoten): stop writing Synthetic timestamps in v23.1.
keyLen += mvccEncodedTimeSyntheticLen
}
}
}
return keyLen
Expand Down Expand Up @@ -355,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)
}
Expand Down
54 changes: 31 additions & 23 deletions pkg/storage/mvcc_key_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ func TestMVCCKeyCompare(t *testing.T) {
b0 := MVCCKey{roachpb.Key("b"), hlc.Timestamp{Logical: 0}}
b1 := MVCCKey{roachpb.Key("b"), hlc.Timestamp{Logical: 1}}
b2 := MVCCKey{roachpb.Key("b"), hlc.Timestamp{Logical: 2}}
b2S := MVCCKey{roachpb.Key("b"), hlc.Timestamp{Logical: 2, Synthetic: true}}

testcases := map[string]struct {
a MVCCKey
Expand All @@ -90,7 +89,6 @@ func TestMVCCKeyCompare(t *testing.T) {
"empty time lt set": {b0, b1, -1}, // empty MVCC timestamps sort before non-empty
"set time gt empty": {b1, b0, 1}, // empty MVCC timestamps sort before non-empty
"key time precedence": {a1, b2, -1}, // a before b, but 2 before 1; key takes precedence
"synthetic equal": {b2, b2S, 0}, // synthetic bit does not affect ordering
}
for name, tc := range testcases {
t.Run(name, func(t *testing.T) {
Expand Down Expand Up @@ -153,10 +151,6 @@ func (k randMVCCKey) Generate(r *rand.Rand, size int) reflect.Value {
k.Key = []byte([...]string{"a", "b", "c"}[r.Intn(3)])
k.Timestamp.WallTime = r.Int63n(5)
k.Timestamp.Logical = r.Int31n(5)
if !k.Timestamp.IsEmpty() {
// NB: the zero timestamp cannot be synthetic.
k.Timestamp.Synthetic = r.Intn(2) != 0
}
return reflect.ValueOf(k)
}

Expand All @@ -168,16 +162,12 @@ func TestEncodeDecodeMVCCKeyAndTimestampWithLength(t *testing.T) {
ts hlc.Timestamp
encoded string // hexadecimal
}{
"empty": {"", hlc.Timestamp{}, "00"},
"only key": {"foo", hlc.Timestamp{}, "666f6f00"},
"no key": {"", hlc.Timestamp{WallTime: 1643550788737652545}, "0016cf10bc0505574109"},
"walltime": {"foo", hlc.Timestamp{WallTime: 1643550788737652545}, "666f6f0016cf10bc0505574109"},
"logical": {"foo", hlc.Timestamp{Logical: 65535}, "666f6f0000000000000000000000ffff0d"},
"synthetic": {"foo", hlc.Timestamp{Synthetic: true}, "666f6f00000000000000000000000000010e"},
"walltime and logical": {"foo", hlc.Timestamp{WallTime: 1643550788737652545, Logical: 65535}, "666f6f0016cf10bc050557410000ffff0d"},
"walltime and synthetic": {"foo", hlc.Timestamp{WallTime: 1643550788737652545, Synthetic: true}, "666f6f0016cf10bc0505574100000000010e"},
"logical and synthetic": {"foo", hlc.Timestamp{Logical: 65535, Synthetic: true}, "666f6f0000000000000000000000ffff010e"},
"all": {"foo", hlc.Timestamp{WallTime: 1643550788737652545, Logical: 65535, Synthetic: true}, "666f6f0016cf10bc050557410000ffff010e"},
"empty": {"", hlc.Timestamp{}, "00"},
"only key": {"foo", hlc.Timestamp{}, "666f6f00"},
"no key": {"", hlc.Timestamp{WallTime: 1643550788737652545}, "0016cf10bc0505574109"},
"walltime": {"foo", hlc.Timestamp{WallTime: 1643550788737652545}, "666f6f0016cf10bc0505574109"},
"logical": {"foo", hlc.Timestamp{Logical: 65535}, "666f6f0000000000000000000000ffff0d"},
"walltime and logical": {"foo", hlc.Timestamp{WallTime: 1643550788737652545, Logical: 65535}, "666f6f0016cf10bc050557410000ffff0d"},
}

buf := []byte{}
Expand Down Expand Up @@ -268,6 +258,31 @@ func TestDecodeUnnormalizedMVCCKey(t *testing.T) {
// keys that only contain (at most) a walltime.
equalToNormal: false,
},
"synthetic": {
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",
// 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",
// 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",
// 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,
},
}
for name, tc := range testcases {
t.Run(name, func(t *testing.T) {
Expand Down Expand Up @@ -345,7 +360,6 @@ func BenchmarkEncodeMVCCKey(b *testing.B) {
"empty": {},
"walltime": {WallTime: 1643550788737652545},
"walltime+logical": {WallTime: 1643550788737652545, Logical: 4096},
"all": {WallTime: 1643550788737652545, Logical: 4096, Synthetic: true},
}
buf := make([]byte, 0, 65536)
for keyDesc, key := range keys {
Expand Down Expand Up @@ -373,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
Expand Down Expand Up @@ -585,11 +598,6 @@ func TestMVCCRangeKeyEncodedSize(t *testing.T) {
"only end": {MVCCRangeKey{EndKey: roachpb.Key("foo")}, 5},
"only walltime": {MVCCRangeKey{Timestamp: hlc.Timestamp{WallTime: 1}}, 11},
"only logical": {MVCCRangeKey{Timestamp: hlc.Timestamp{Logical: 1}}, 15},
"all": {MVCCRangeKey{
StartKey: roachpb.Key("start"),
EndKey: roachpb.Key("end"),
Timestamp: hlc.Timestamp{WallTime: 1, Logical: 1, Synthetic: true},
}, 24},
}
for name, tc := range testcases {
t.Run(name, func(t *testing.T) {
Expand Down
11 changes: 0 additions & 11 deletions pkg/storage/mvcc_value.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 4 additions & 6 deletions pkg/storage/mvcc_value_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 0 additions & 4 deletions pkg/storage/pebble_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -440,10 +440,6 @@ func makeRandEncodedKeys() [][]byte {
// 20% of keys have a logical component.
k.Timestamp.Logical = rng.Int31n(4) + 1
}
if rng.Int31n(1000) == 0 && !k.Timestamp.IsEmpty() {
// 0.1% of keys have a synthetic component.
k.Timestamp.Synthetic = true
}
keys[i] = EncodeMVCCKey(k)
}
return keys
Expand Down
Loading

0 comments on commit a61471c

Please sign in to comment.