diff --git a/pkg/ccl/backupccl/restore_data_processor_test.go b/pkg/ccl/backupccl/restore_data_processor_test.go index 1c929cb5bc60..d120c6a4e0d2 100644 --- a/pkg/ccl/backupccl/restore_data_processor_test.go +++ b/pkg/ccl/backupccl/restore_data_processor_test.go @@ -113,7 +113,11 @@ func slurpSSTablesLatestKey( } else if !ok || !it.UnsafeKey().Less(end) { break } - kvs = append(kvs, storage.MVCCKeyValue{Key: it.Key(), Value: it.Value()}) + val, err := storage.DecodeMVCCValue(it.Value()) + if err != nil { + t.Fatal(err) + } + kvs = append(kvs, storage.MVCCKeyValue{Key: it.Key(), Value: val.Value.RawBytes}) } return kvs } diff --git a/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go b/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go index b29e721d04bf..cf016db1222e 100644 --- a/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go @@ -772,8 +772,10 @@ func TestEvalAddSSTable(t *testing.T) { ts := iter.Key().Timestamp.WallTime var value []byte if iter.Key().IsValue() { - if len(iter.Value()) > 0 { - value, err = roachpb.Value{RawBytes: iter.Value()}.GetBytes() + mvccVal, err := storage.DecodeMVCCValue(iter.Value()) + require.NoError(t, err) + if !mvccVal.IsTombstone() { + value, err = mvccVal.Value.GetBytes() require.NoError(t, err) } } else { @@ -1091,6 +1093,7 @@ func runTestDBAddSSTable( func TestAddSSTableMVCCStats(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + storage.SkipIfSimpleValueEncodingDisabled(t) const max = 1 << 10 ctx := context.Background() @@ -1209,6 +1212,7 @@ func TestAddSSTableMVCCStats(t *testing.T) { func TestAddSSTableMVCCStatsDisallowShadowing(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + storage.SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() st := cluster.MakeTestingClusterSettings() @@ -1467,7 +1471,9 @@ func TestAddSSTableSSTTimestampToRequestTimestampRespectsClosedTS(t *testing.T) require.NoError(t, err) require.Len(t, kvs, 1) require.Equal(t, storage.MVCCKey{Key: roachpb.Key("key"), Timestamp: writeTS}, kvs[0].Key) - v, err := roachpb.Value{RawBytes: kvs[0].Value}.GetBytes() + mvccVal, err := storage.DecodeMVCCValue(kvs[0].Value) + require.NoError(t, err) + v, err := mvccVal.Value.GetBytes() require.NoError(t, err) require.Equal(t, "sst", string(v)) } diff --git a/pkg/kv/kvserver/batcheval/cmd_export_test.go b/pkg/kv/kvserver/batcheval/cmd_export_test.go index 837ff893d3bd..de6fd2d11681 100644 --- a/pkg/kv/kvserver/batcheval/cmd_export_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_export_test.go @@ -674,6 +674,7 @@ func assertEqualKVs( func TestRandomKeyAndTimestampExport(t *testing.T) { defer leaktest.AfterTest(t)() + storage.SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() diff --git a/pkg/kv/kvserver/consistency_queue_test.go b/pkg/kv/kvserver/consistency_queue_test.go index 951f071e7aee..279968db5b8b 100644 --- a/pkg/kv/kvserver/consistency_queue_test.go +++ b/pkg/kv/kvserver/consistency_queue_test.go @@ -234,6 +234,9 @@ func TestCheckConsistencyInconsistent(t *testing.T) { // good to make sure we're overly redacting said diff. defer log.TestingSetRedactable(true)() + // Test expects simple MVCC value encoding. + storage.SkipIfSimpleValueEncodingDisabled(t) + // Test uses sticky registry to have persistent pebble state that could // be analyzed for existence of snapshots and to verify snapshot content // after failures. diff --git a/pkg/kv/kvserver/gc/gc_iterator_test.go b/pkg/kv/kvserver/gc/gc_iterator_test.go index 8c148f30d82e..6137c6d237e3 100644 --- a/pkg/kv/kvserver/gc/gc_iterator_test.go +++ b/pkg/kv/kvserver/gc/gc_iterator_test.go @@ -28,6 +28,7 @@ import ( // engine and then validating the state of the iterator as it iterates that // data. func TestGCIterator(t *testing.T) { + storage.SkipIfSimpleValueEncodingDisabled(t) // dataItem represents a version in the storage engine and optionally a // corresponding transaction which will make the MVCCKeyValue an intent. type dataItem struct { diff --git a/pkg/kv/kvserver/mvcc_gc_queue_test.go b/pkg/kv/kvserver/mvcc_gc_queue_test.go index b44d3c031a52..95d0eba35631 100644 --- a/pkg/kv/kvserver/mvcc_gc_queue_test.go +++ b/pkg/kv/kvserver/mvcc_gc_queue_test.go @@ -356,6 +356,7 @@ func (cws *cachedWriteSimulator) shouldQueue( func TestMVCCGCQueueMakeGCScoreRealistic(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + storage.SkipIfSimpleValueEncodingDisabled(t) cws := newCachedWriteSimulator(t) @@ -462,6 +463,7 @@ func TestMVCCGCQueueMakeGCScoreRealistic(t *testing.T) { func TestMVCCGCQueueProcess(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + storage.SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() tc := testContext{} stopper := stop.NewStopper() diff --git a/pkg/kv/kvserver/replica_test.go b/pkg/kv/kvserver/replica_test.go index 90acce579380..a622cffd67ae 100644 --- a/pkg/kv/kvserver/replica_test.go +++ b/pkg/kv/kvserver/replica_test.go @@ -6247,6 +6247,7 @@ func verifyRangeStats( func TestRangeStatsComputation(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + storage.SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() tc := testContext{} stopper := stop.NewStopper() diff --git a/pkg/sql/row/fetcher_mvcc_test.go b/pkg/sql/row/fetcher_mvcc_test.go index 48f6495eee9e..5d108dbd2771 100644 --- a/pkg/sql/row/fetcher_mvcc_test.go +++ b/pkg/sql/row/fetcher_mvcc_test.go @@ -57,10 +57,14 @@ func slurpUserDataKVs(t testing.TB, e storage.Engine) []roachpb.KeyValue { if !it.UnsafeKey().IsValue() { return errors.Errorf("found intent key %v", it.UnsafeKey()) } - kvs = append(kvs, roachpb.KeyValue{ - Key: it.Key().Key, - Value: roachpb.Value{RawBytes: it.Value(), Timestamp: it.UnsafeKey().Timestamp}, - }) + mvccValue, err := storage.DecodeMVCCValue(it.Value()) + if err != nil { + t.Fatal(err) + } + value := mvccValue.Value + value.Timestamp = it.UnsafeKey().Timestamp + kv := roachpb.KeyValue{Key: it.Key().Key, Value: value} + kvs = append(kvs, kv) } return nil }) diff --git a/pkg/storage/BUILD.bazel b/pkg/storage/BUILD.bazel index af04e1c3b7b6..8f54ee576591 100644 --- a/pkg/storage/BUILD.bazel +++ b/pkg/storage/BUILD.bazel @@ -57,6 +57,7 @@ go_library( "//pkg/settings/cluster", "//pkg/storage/enginepb", "//pkg/storage/fs", + "//pkg/testutils/skip", "//pkg/util", "//pkg/util/bufalloc", "//pkg/util/encoding", diff --git a/pkg/storage/mvcc_history_test.go b/pkg/storage/mvcc_history_test.go index 5834e73d2503..7c13fb62d13e 100644 --- a/pkg/storage/mvcc_history_test.go +++ b/pkg/storage/mvcc_history_test.go @@ -90,6 +90,7 @@ import ( func TestMVCCHistories(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() diff --git a/pkg/storage/mvcc_incremental_iterator_test.go b/pkg/storage/mvcc_incremental_iterator_test.go index 79c1363b89f9..0ced12facc5e 100644 --- a/pkg/storage/mvcc_incremental_iterator_test.go +++ b/pkg/storage/mvcc_incremental_iterator_test.go @@ -497,6 +497,7 @@ func assertEqualKVs( func TestMVCCIncrementalIteratorNextIgnoringTime(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() var ( @@ -634,6 +635,7 @@ func TestMVCCIncrementalIteratorNextIgnoringTime(t *testing.T) { func TestMVCCIncrementalIteratorNextKeyIgnoringTime(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() var ( @@ -764,6 +766,7 @@ func TestMVCCIncrementalIteratorNextKeyIgnoringTime(t *testing.T) { func TestMVCCIncrementalIteratorInlinePolicy(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() var ( @@ -858,6 +861,7 @@ func TestMVCCIncrementalIteratorInlinePolicy(t *testing.T) { func TestMVCCIncrementalIteratorIntentPolicy(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() var ( @@ -1038,6 +1042,7 @@ func expectIntent(t *testing.T, iter SimpleMVCCIterator, intent roachpb.Intent) func TestMVCCIncrementalIterator(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) ctx := context.Background() var ( @@ -1318,6 +1323,7 @@ func TestMVCCIncrementalIteratorIntentRewrittenConcurrently(t *testing.T) { func TestMVCCIncrementalIteratorIntentDeletion(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) txn := func(key roachpb.Key, ts hlc.Timestamp) *roachpb.Transaction { return &roachpb.Transaction{ @@ -1535,6 +1541,7 @@ func TestMVCCIncrementalIteratorIntentStraddlesSStables(t *testing.T) { func TestMVCCIterateTimeBound(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + SkipIfSimpleValueEncodingDisabled(t) dir, cleanupFn := testutils.TempDir(t) defer cleanupFn() diff --git a/pkg/storage/mvcc_stats_test.go b/pkg/storage/mvcc_stats_test.go index 2d45b747c525..57c98d66e61f 100644 --- a/pkg/storage/mvcc_stats_test.go +++ b/pkg/storage/mvcc_stats_test.go @@ -77,6 +77,11 @@ func assertEqLocal(t *testing.T, rw ReadWriter, debug string, ms, expMS *enginep assertEqImpl(t, rw, debug, false /* globalKeys */, ms, expMS) } +var emptyMVCCValueHeaderSize = func() int64 { + var h enginepb.MVCCValueHeader + return extendedPreludeSize + int64(h.Size()) +}() + // TestMVCCStatsDeleteCommitMovesTimestamp exercises the case in which a value // is written, later deleted via an intent and the deletion committed at an even // higher timestamp. This exercises subtleties related to the implicit push of @@ -105,13 +110,16 @@ func TestMVCCStatsDeleteCommitMovesTimestamp(t *testing.T) { mKeySize := int64(mvccKey(key).EncodedSize()) // 2 vKeySize := MVCCVersionTimestampSize // 12 vValSize := int64(len(value.RawBytes)) // 10 + if disableSimpleValueEncoding { + vValSize += emptyMVCCValueHeaderSize // 17 + } expMS := enginepb.MVCCStats{ - LiveBytes: mKeySize + vKeySize + vValSize, // 24 + LiveBytes: mKeySize + vKeySize + vValSize, // 24[+7] LiveCount: 1, KeyBytes: mKeySize + vKeySize, // 14 KeyCount: 1, - ValBytes: vValSize, // 10 + ValBytes: vValSize, // 10[+7] ValCount: 1, LastUpdateNanos: 1e9, } @@ -201,18 +209,21 @@ func TestMVCCStatsPutCommitMovesTimestamp(t *testing.T) { mValSize += 2 vKeySize := MVCCVersionTimestampSize // 12 vValSize := int64(len(value.RawBytes)) // 10 + if disableSimpleValueEncoding { + vValSize += emptyMVCCValueHeaderSize // 17 + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, - LiveBytes: mKeySize + mValSize + vKeySize + vValSize, // 2+(46[+2])+12+10 = 68[+2] + LiveBytes: mKeySize + mValSize + vKeySize + vValSize, // 2+(46[+2])+12+(10[+7]) = 68[+2][+7] LiveCount: 1, KeyBytes: mKeySize + vKeySize, // 2+12 =14 KeyCount: 1, - ValBytes: mValSize + vValSize, // (46[+2])+10 = 54[+2] + ValBytes: mValSize + vValSize, // (46[+2])+(10[+7]) = 54[+2][+7] ValCount: 1, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize + vValSize, // 12+10 = 22 + IntentBytes: vKeySize + vValSize, // 12+(10[+7]) = 22[+7] GCBytesAge: 0, } assertEq(t, engine, "after put", aggMS, &expMS) @@ -233,7 +244,7 @@ func TestMVCCStatsPutCommitMovesTimestamp(t *testing.T) { // that it now uses the extended MVCCValue encoding. vValHeader := enginepb.MVCCValueHeader{LocalTimestamp: hlc.ClockTimestamp(ts1)} vValHeaderSize := extendedPreludeSize + int64(vValHeader.Size()) // 13 - vValSize += vValHeaderSize // 23 + vValSize = int64(len(value.RawBytes)) + vValHeaderSize // 23 expAggMS := enginepb.MVCCStats{ LastUpdateNanos: 4e9, @@ -290,19 +301,22 @@ func TestMVCCStatsPutPushMovesTimestamp(t *testing.T) { mValSize += 2 vKeySize := MVCCVersionTimestampSize // 12 vValSize := int64(len(value.RawBytes)) // 10 + if disableSimpleValueEncoding { + vValSize += emptyMVCCValueHeaderSize // 17 + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, - LiveBytes: mKeySize + mValSize + vKeySize + vValSize, // 2+(46[+2])+12+10 = 70[+2] + LiveBytes: mKeySize + mValSize + vKeySize + vValSize, // 2+(46[+2])+12+(10[+7]) = 70[+2][+7] LiveCount: 1, KeyBytes: mKeySize + vKeySize, // 2+12 = 14 KeyCount: 1, - ValBytes: mValSize + vValSize, // (46[+2])+10 = 54[+2] + ValBytes: mValSize + vValSize, // (46[+2])+(10[+7]) = 54[+2][+7] ValCount: 1, IntentAge: 0, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize + vValSize, // 12+10 = 22 + IntentBytes: vKeySize + vValSize, // 12+(10[+7]) = 22[+7] } assertEq(t, engine, "after put", aggMS, &expMS) @@ -323,7 +337,7 @@ func TestMVCCStatsPutPushMovesTimestamp(t *testing.T) { // that it now uses the extended MVCCValue encoding. vValHeader := enginepb.MVCCValueHeader{LocalTimestamp: hlc.ClockTimestamp(ts1)} vValHeaderSize := extendedPreludeSize + int64(vValHeader.Size()) // 13 - vValSize += vValHeaderSize // 23 + vValSize = int64(len(value.RawBytes)) + vValHeaderSize // 23 expAggMS := enginepb.MVCCStats{ LastUpdateNanos: 4e9, @@ -404,19 +418,23 @@ func TestMVCCStatsDeleteMovesTimestamp(t *testing.T) { vValSize := int64(len(value.RawBytes)) require.EqualValues(t, vValSize, 10) + if disableSimpleValueEncoding { + vValSize += emptyMVCCValueHeaderSize + require.EqualValues(t, vValSize, 17) + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, - LiveBytes: mKeySize + m1ValSize + vKeySize + vValSize, // 2+(46[+2])+12+10 = 70[+2] + LiveBytes: mKeySize + m1ValSize + vKeySize + vValSize, // 2+(46[+2])+12+(10[+7]) = 70[+2][+7] LiveCount: 1, KeyBytes: mKeySize + vKeySize, // 2+12 = 14 KeyCount: 1, - ValBytes: mVal1Size + vValSize, // (46[+2])+10 = 56([+2]) + ValBytes: mVal1Size + vValSize, // (46[+2])+(10[+7]) = 56[+2][+7] ValCount: 1, IntentAge: 0, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize + vValSize, // 12+10 = 22 + IntentBytes: vKeySize + vValSize, // 12+(10[+7]) = 22[+7] } assertEq(t, engine, "after put", aggMS, &expMS) @@ -430,19 +448,30 @@ func TestMVCCStatsDeleteMovesTimestamp(t *testing.T) { // Annoyingly, the new meta value is actually a little larger thanks to the // sequence number. Also since there was a write previously on the same // transaction, the IntentHistory will add a few bytes to the metadata. + encValue, err := EncodeMVCCValue(MVCCValue{Value: value}) + require.NoError(t, err) m2ValSize := int64((&enginepb.MVCCMetadata{ Timestamp: ts2.ToLegacyTimestamp(), Txn: &txn.TxnMeta, IntentHistory: []enginepb.MVCCMetadata_SequencedIntent{ - {Sequence: 0, Value: value.RawBytes}, + {Sequence: 0, Value: encValue}, }, }).Size()) - require.EqualValues(t, m2ValSize, 64) + expM2ValSize := 64 + if disableSimpleValueEncoding { + expM2ValSize = 71 + } + require.EqualValues(t, m2ValSize, expM2ValSize) if err := MVCCDelete(ctx, engine, aggMS, key, txn.ReadTimestamp, hlc.ClockTimestamp{}, txn); err != nil { t.Fatal(err) } + vVal2Size := int64(0) // tombstone + if disableSimpleValueEncoding { + vVal2Size = emptyMVCCValueHeaderSize // 7 + } + expAggMS := enginepb.MVCCStats{ LastUpdateNanos: 2e9, LiveBytes: 0, @@ -453,12 +482,12 @@ func TestMVCCStatsDeleteMovesTimestamp(t *testing.T) { // One versioned key counts for vKeySize. KeyBytes: mKeySize + vKeySize, // The intent is still there, but this time with mVal2Size, and a zero vValSize. - ValBytes: m2ValSize, // 10+46 = 56 + ValBytes: m2ValSize + vVal2Size, IntentAge: 0, IntentCount: 1, // still there SeparatedIntentCount: 1, - IntentBytes: vKeySize, // still there, but now without vValSize - GCBytesAge: 0, // this was once erroneously negative + IntentBytes: vKeySize + vVal2Size, // still there, but now without vValSize + GCBytesAge: 0, // this was once erroneously negative } assertEq(t, engine, "after deleting", aggMS, &expAggMS) @@ -496,8 +525,6 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) { t.Fatal(err) } - value := roachpb.MakeValueFromString("value") - mKeySize := int64(mvccKey(key).EncodedSize()) require.EqualValues(t, mKeySize, 2) @@ -512,8 +539,10 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) { vKeySize := MVCCVersionTimestampSize require.EqualValues(t, vKeySize, 12) - vValSize := int64(len(value.RawBytes)) - require.EqualValues(t, vValSize, 10) + vVal1Size := int64(0) // tombstone + if disableSimpleValueEncoding { + vVal1Size = emptyMVCCValueHeaderSize // 7 + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, @@ -521,12 +550,12 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) { LiveCount: 0, KeyBytes: mKeySize + vKeySize, // 2 + 12 = 24 KeyCount: 1, - ValBytes: mVal1Size, // 46[+2] + ValBytes: mVal1Size + vVal1Size, // 46[+2] [+7] ValCount: 1, IntentAge: 0, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize, // 12 + IntentBytes: vKeySize + vVal1Size, // 12 [+7] GCBytesAge: 0, } assertEq(t, engine, "after delete", aggMS, &expMS) @@ -541,14 +570,29 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) { // Annoyingly, the new meta value is actually a little larger thanks to the // sequence number. Also the value is larger because the previous intent on the // transaction is recorded in the IntentHistory. - m2ValSize := int64((&enginepb.MVCCMetadata{ + encVal1, err := EncodeMVCCValue(MVCCValue{Value: roachpb.Value{RawBytes: []byte{}}}) + require.NoError(t, err) + mVal2Size := int64((&enginepb.MVCCMetadata{ Timestamp: ts2.ToLegacyTimestamp(), Txn: &txn.TxnMeta, IntentHistory: []enginepb.MVCCMetadata_SequencedIntent{ - {Sequence: 0, Value: []byte{}}, + {Sequence: 0, Value: encVal1}, }, }).Size()) - require.EqualValues(t, m2ValSize, 54) + expMVal2Size := 54 + if disableSimpleValueEncoding { + expMVal2Size = 61 + } + require.EqualValues(t, mVal2Size, expMVal2Size) + + value := roachpb.MakeValueFromString("value") + + vVal2Size := int64(len(value.RawBytes)) + require.EqualValues(t, vVal2Size, 10) + if disableSimpleValueEncoding { + vVal2Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal2Size, 17) + } if err := MVCCPut(ctx, engine, aggMS, key, txn.ReadTimestamp, hlc.ClockTimestamp{}, value, txn); err != nil { t.Fatal(err) @@ -556,7 +600,7 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) { expAggMS := enginepb.MVCCStats{ LastUpdateNanos: 2e9, - LiveBytes: mKeySize + m2ValSize + vKeySize + vValSize, // 2+46+12+10 = 70 + LiveBytes: mKeySize + mVal2Size + vKeySize + vVal2Size, // 2+46+12+(10[+7]) = 70[+7] LiveCount: 1, KeyCount: 1, ValCount: 1, @@ -564,12 +608,12 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) { // One versioned key counts for vKeySize. KeyBytes: mKeySize + vKeySize, // The intent is still there, but this time with mVal2Size, and a zero vValSize. - ValBytes: vValSize + m2ValSize, // 10+46 = 56 + ValBytes: vVal2Size + mVal2Size, // (10[+7])+46 = 56[+7] IntentAge: 0, IntentCount: 1, // still there SeparatedIntentCount: 1, - IntentBytes: vKeySize + vValSize, // still there, now bigger - GCBytesAge: 0, // this was once erroneously negative + IntentBytes: vKeySize + vVal2Size, // still there, now bigger + GCBytesAge: 0, // this was once erroneously negative } assertEq(t, engine, "after put", aggMS, &expAggMS) @@ -611,17 +655,22 @@ func TestMVCCStatsDelDelCommitMovesTimestamp(t *testing.T) { vKeySize := MVCCVersionTimestampSize require.EqualValues(t, vKeySize, 12) + vValSize := int64(0) // tombstone + if disableSimpleValueEncoding { + vValSize = emptyMVCCValueHeaderSize // 7 + } + expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, KeyBytes: mKeySize + vKeySize, KeyCount: 1, - ValBytes: 0, + ValBytes: vValSize, ValCount: 1, } assertEq(t, engine, "after non-transactional delete", aggMS, &expMS) - // Write an tombstone intent at t=2s. + // Write a tombstone intent at t=2s. txn := &roachpb.Transaction{ TxnMeta: enginepb.TxnMeta{ID: uuid.MakeV4(), WriteTimestamp: ts2}, ReadTimestamp: ts2, @@ -643,13 +692,13 @@ func TestMVCCStatsDelDelCommitMovesTimestamp(t *testing.T) { LastUpdateNanos: 2e9, KeyBytes: mKeySize + 2*vKeySize, // 2+2*12 = 26 KeyCount: 1, - ValBytes: mValSize, // 46[+2] + ValBytes: mValSize + 2*vValSize, // 46[+2] [+7] ValCount: 2, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize, // TBD + IntentBytes: vKeySize + vValSize, // The original non-transactional write (at 1s) has now aged one second. - GCBytesAge: 1 * vKeySize, + GCBytesAge: 1 * (vKeySize + vValSize), } assertEq(t, engine, "after put", aggMS, &expMS) @@ -672,21 +721,21 @@ func TestMVCCStatsDelDelCommitMovesTimestamp(t *testing.T) { // The initial write used the simple MVCCValue encoding. When resolved to // a higher timestamp, the MVCCValue retained its local timestamp, meaning // that it now uses the extended MVCCValue encoding. - vValHeader := enginepb.MVCCValueHeader{LocalTimestamp: hlc.ClockTimestamp(ts2)} - vValHeaderSize := extendedPreludeSize + int64(vValHeader.Size()) // 13 - vValSize := vValHeaderSize + 0 // tombstone, so just a header + vVal2Header := enginepb.MVCCValueHeader{LocalTimestamp: hlc.ClockTimestamp(ts2)} + vVal2HeaderSize := extendedPreludeSize + int64(vVal2Header.Size()) // 13 + vVal2Size := vVal2HeaderSize + 0 // tombstone, so just a header expAggMS := enginepb.MVCCStats{ LastUpdateNanos: 3e9, KeyBytes: mKeySize + 2*vKeySize, // 2+2*12 = 26 KeyCount: 1, - ValBytes: vValSize, + ValBytes: vValSize + vVal2Size, ValCount: 2, IntentCount: 0, IntentBytes: 0, // The very first write picks up another second of age. Before a bug fix, // this was failing to do so. - GCBytesAge: 2 * vKeySize, + GCBytesAge: 2 * (vKeySize + vValSize), } assertEq(t, engine, "after committing", &aggMS, &expAggMS) @@ -709,14 +758,14 @@ func TestMVCCStatsDelDelCommitMovesTimestamp(t *testing.T) { LastUpdateNanos: 3e9, KeyBytes: mKeySize + vKeySize, // 2+12 = 14 KeyCount: 1, - ValBytes: 0, + ValBytes: vValSize, ValCount: 1, IntentCount: 0, IntentBytes: 0, // We aborted our intent, but the value we first wrote was a tombstone, and // so it's expected to retain its age. Since it's now the only value, it // also contributes as a meta key. - GCBytesAge: 2 * (mKeySize + vKeySize), + GCBytesAge: 2 * (mKeySize + vKeySize + vValSize), } assertEq(t, engine, "after aborting", &aggMS, &expAggMS) @@ -765,6 +814,9 @@ func TestMVCCStatsPutDelPutMovesTimestamp(t *testing.T) { vValSize := int64(len(value.RawBytes)) require.EqualValues(t, vValSize, 10) + if disableSimpleValueEncoding { + vValSize += emptyMVCCValueHeaderSize // 17 + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, @@ -795,15 +847,20 @@ func TestMVCCStatsPutDelPutMovesTimestamp(t *testing.T) { require.EqualValues(t, mValSize, 46) mValSize += 2 + vDelSize := int64(0) // tombstone + if disableSimpleValueEncoding { + vDelSize = emptyMVCCValueHeaderSize // 7 + } + expMS = enginepb.MVCCStats{ LastUpdateNanos: 2e9, KeyBytes: mKeySize + 2*vKeySize, // 2+2*12 = 26 KeyCount: 1, - ValBytes: mValSize + vValSize, // 46[+2]+10 = 56[+2] + ValBytes: mValSize + vValSize + vDelSize, // 46[+2]+10[+7] = 56[+2][+7] ValCount: 2, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize, // 12 + IntentBytes: vKeySize + vDelSize, // 12[+7] // The original non-transactional write becomes non-live at 2s, so no age // is accrued yet. GCBytesAge: 0, @@ -862,6 +919,10 @@ func TestMVCCStatsPutDelPutMovesTimestamp(t *testing.T) { val2 := roachpb.MakeValueFromString("longvalue") vVal2Size := int64(len(val2.RawBytes)) require.EqualValues(t, vVal2Size, 14) + if disableSimpleValueEncoding { + vVal2Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal2Size, 21) + } txn.WriteTimestamp.Forward(ts3) if err := MVCCPut(ctx, engine, &aggMS, key, txn.ReadTimestamp, hlc.ClockTimestamp{}, val2, txn); err != nil { @@ -870,24 +931,29 @@ func TestMVCCStatsPutDelPutMovesTimestamp(t *testing.T) { // Annoyingly, the new meta value is actually a little larger thanks to the // sequence number. - m2ValSizeWithHistory := int64((&enginepb.MVCCMetadata{ + encDel, err := EncodeMVCCValue(MVCCValue{Value: roachpb.Value{RawBytes: []byte{}}}) + require.NoError(t, err) + mVal2SizeWithHistory := int64((&enginepb.MVCCMetadata{ Timestamp: ts3.ToLegacyTimestamp(), Txn: &txn.TxnMeta, IntentHistory: []enginepb.MVCCMetadata_SequencedIntent{ - {Sequence: 0, Value: []byte{}}, + {Sequence: 0, Value: encDel}, }, }).Size()) - - require.EqualValues(t, m2ValSizeWithHistory, 54) + expMVal2Size := 54 + if disableSimpleValueEncoding { + expMVal2Size = 61 + } + require.EqualValues(t, mVal2SizeWithHistory, expMVal2Size) expAggMS := enginepb.MVCCStats{ LastUpdateNanos: 3e9, KeyBytes: mKeySize + 2*vKeySize, // 2+2*12 = 26 KeyCount: 1, - ValBytes: m2ValSizeWithHistory + vValSize + vVal2Size, + ValBytes: mVal2SizeWithHistory + vValSize + vVal2Size, ValCount: 2, LiveCount: 1, - LiveBytes: mKeySize + m2ValSizeWithHistory + vKeySize + vVal2Size, + LiveBytes: mKeySize + mVal2SizeWithHistory + vKeySize + vVal2Size, IntentCount: 1, SeparatedIntentCount: 1, IntentBytes: vKeySize + vVal2Size, @@ -932,13 +998,18 @@ func TestMVCCStatsDelDelGC(t *testing.T) { mKeySize := int64(mvccKey(key).EncodedSize()) // 2 vKeySize := MVCCVersionTimestampSize // 12 + vValSize := int64(0) // tombstone + if disableSimpleValueEncoding { + vValSize = emptyMVCCValueHeaderSize // 7 + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 2e9, KeyBytes: mKeySize + 2*vKeySize, // 26 + ValBytes: 2 * vValSize, KeyCount: 1, ValCount: 2, - GCBytesAge: 1 * vKeySize, // first tombstone, aged from ts1 to ts2 + GCBytesAge: 1 * (vKeySize + vValSize), // first tombstone, aged from ts1 to ts2 } assertEq(t, engine, "after two puts", aggMS, &expMS) @@ -1016,18 +1087,21 @@ func TestMVCCStatsPutIntentTimestampNotPutTimestamp(t *testing.T) { m1ValSize += 2 vKeySize := MVCCVersionTimestampSize // 12 vValSize := int64(len(value.RawBytes)) // 10 + if disableSimpleValueEncoding { + vValSize += emptyMVCCValueHeaderSize // 17 + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 2e9 + 1, - LiveBytes: mKeySize + m1ValSize + vKeySize + vValSize, // 2+(46[+2])+12+10 = 68[+2] + LiveBytes: mKeySize + m1ValSize + vKeySize + vValSize, // 2+(46[+2])+12+(10[+7]) = 68[+2][+7] LiveCount: 1, KeyBytes: mKeySize + vKeySize, // 14 KeyCount: 1, - ValBytes: m1ValSize + vValSize, // (46[+2])+10 = 54[+2] + ValBytes: m1ValSize + vValSize, // (46[+2])+(10[+7]) = 54[+2][+7] ValCount: 1, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize + vValSize, // 12+10 = 22 + IntentBytes: vKeySize + vValSize, // 12+(10[+7]) = 22[+7] } assertEq(t, engine, "after first put", aggMS, &expMS) @@ -1042,11 +1116,13 @@ func TestMVCCStatsPutIntentTimestampNotPutTimestamp(t *testing.T) { // Annoyingly, the new meta value is actually a little larger thanks to the // sequence number. + encValue, err := EncodeMVCCValue(MVCCValue{Value: value}) + require.NoError(t, err) m2ValSize := int64((&enginepb.MVCCMetadata{ // 46 Timestamp: ts201.ToLegacyTimestamp(), Txn: &txn.TxnMeta, IntentHistory: []enginepb.MVCCMetadata_SequencedIntent{ - {Sequence: 0, Value: value.RawBytes}, + {Sequence: 0, Value: encValue}, }, }).Size()) if err := MVCCPut(ctx, engine, aggMS, key, txn.ReadTimestamp, hlc.ClockTimestamp{}, value, txn); err != nil { @@ -1059,15 +1135,15 @@ func TestMVCCStatsPutIntentTimestampNotPutTimestamp(t *testing.T) { IntentAge: 0, LastUpdateNanos: 2e9 + 1, - LiveBytes: mKeySize + m2ValSize + vKeySize + vValSize, // 2+46+12+10 = 70 + LiveBytes: mKeySize + m2ValSize + vKeySize + vValSize, // 2+(46[+7])+12+(10[+7]) = 70[+14] LiveCount: 1, KeyBytes: mKeySize + vKeySize, // 14 KeyCount: 1, - ValBytes: m2ValSize + vValSize, // 46+10 = 56 + ValBytes: m2ValSize + vValSize, // (46[+7])+(10[+7]) = 56[+14] ValCount: 1, IntentCount: 1, SeparatedIntentCount: 1, - IntentBytes: vKeySize + vValSize, // 12+10 = 22 + IntentBytes: vKeySize + vValSize, // 12+(10[+7]) = 22[+7] } assertEq(t, engine, "after second put", aggMS, &expAggMS) @@ -1109,15 +1185,19 @@ func TestMVCCStatsPutWaitDeleteGC(t *testing.T) { vValSize := int64(len(val1.RawBytes)) require.EqualValues(t, vValSize, 10) + if disableSimpleValueEncoding { + vValSize += emptyMVCCValueHeaderSize + require.EqualValues(t, vValSize, 17) + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, KeyCount: 1, KeyBytes: mKeySize + vKeySize, // 2+12 = 14 ValCount: 1, - ValBytes: vValSize, // 10 + ValBytes: vValSize, // 10[+7] LiveCount: 1, - LiveBytes: mKeySize + vKeySize + vValSize, // 2+12+10 = 24 + LiveBytes: mKeySize + vKeySize + vValSize, // 2+12+(10[+7]) = 24[+7] } assertEq(t, engine, "after first put", aggMS, &expMS) @@ -1127,11 +1207,16 @@ func TestMVCCStatsPutWaitDeleteGC(t *testing.T) { t.Fatal(err) } + vVal2Size := int64(0) // tombstone + if disableSimpleValueEncoding { + vVal2Size = emptyMVCCValueHeaderSize // 7 + } + expMS = enginepb.MVCCStats{ LastUpdateNanos: 2e9, KeyCount: 1, KeyBytes: mKeySize + 2*vKeySize, // 2+2*12 = 26 - ValBytes: vValSize, // 10 + ValBytes: vValSize + vVal2Size, // 10[+7] ValCount: 2, LiveBytes: 0, LiveCount: 0, @@ -1151,7 +1236,7 @@ func TestMVCCStatsPutWaitDeleteGC(t *testing.T) { LastUpdateNanos: 2e9, KeyCount: 1, KeyBytes: mKeySize + vKeySize, // 2+12 = 14 - ValBytes: 0, + ValBytes: vVal2Size, ValCount: 1, LiveBytes: 0, LiveCount: 0, @@ -1210,14 +1295,22 @@ func TestMVCCStatsTxnSysPutPut(t *testing.T) { vVal1Size := int64(len(val1.RawBytes)) require.EqualValues(t, vVal1Size, 10) + if disableSimpleValueEncoding { + vVal1Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal1Size, 17) + } val2 := roachpb.MakeValueFromString("longvalue") vVal2Size := int64(len(val2.RawBytes)) require.EqualValues(t, vVal2Size, 14) + if disableSimpleValueEncoding { + vVal2Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal2Size, 21) + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, - SysBytes: mKeySize + mValSize + vKeySize + vVal1Size, // 11+(46[+2])+12+10 = 79[+2] + SysBytes: mKeySize + mValSize + vKeySize + vVal1Size, // 11+(46[+2])+12+(10[+7]) = 79[+2][+7] SysCount: 1, } assertEqLocal(t, engine, "after first put", aggMS, &expMS) @@ -1229,15 +1322,21 @@ func TestMVCCStatsTxnSysPutPut(t *testing.T) { // The new meta value grows because we've bumped `txn.Sequence`. // The value also grows as the older value is part of the same // transaction and so contributes to the intent history. + encVal1, err := EncodeMVCCValue(MVCCValue{Value: val1}) + require.NoError(t, err) mVal2Size := int64((&enginepb.MVCCMetadata{ Timestamp: ts2.ToLegacyTimestamp(), Deleted: false, Txn: &txn.TxnMeta, IntentHistory: []enginepb.MVCCMetadata_SequencedIntent{ - {Sequence: 0, Value: val1.RawBytes}, + {Sequence: 0, Value: encVal1}, }, }).Size()) - require.EqualValues(t, mVal2Size, 64) + expMVal2Size := 64 + if disableSimpleValueEncoding { + expMVal2Size = 71 + } + require.EqualValues(t, mVal2Size, expMVal2Size) if err := MVCCPut(ctx, engine, aggMS, key, txn.ReadTimestamp, hlc.ClockTimestamp{}, val2, txn); err != nil { t.Fatal(err) @@ -1245,7 +1344,7 @@ func TestMVCCStatsTxnSysPutPut(t *testing.T) { expMS = enginepb.MVCCStats{ LastUpdateNanos: 1e9, - SysBytes: mKeySize + mVal2Size + vKeySize + vVal2Size, // 11+46+12+14 = 83 + SysBytes: mKeySize + mVal2Size + vKeySize + vVal2Size, // 11+(46[+7])+12+14 = 83[+7] SysCount: 1, } @@ -1300,14 +1399,22 @@ func TestMVCCStatsTxnSysPutAbort(t *testing.T) { vVal1Size := int64(len(val1.RawBytes)) require.EqualValues(t, vVal1Size, 10) + if disableSimpleValueEncoding { + vVal1Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal1Size, 17) + } val2 := roachpb.MakeValueFromString("longvalue") vVal2Size := int64(len(val2.RawBytes)) require.EqualValues(t, vVal2Size, 14) + if disableSimpleValueEncoding { + vVal2Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal2Size, 21) + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, - SysBytes: mKeySize + mValSize + vKeySize + vVal1Size, // 11+(46[+2])+12+10 = 79[+2] + SysBytes: mKeySize + mValSize + vKeySize + vVal1Size, // 11+(46[+2])+12+(10[+7]) = 79[+2][+7] SysCount: 1, } assertEqLocal(t, engine, "after first put", aggMS, &expMS) @@ -1362,14 +1469,22 @@ func TestMVCCStatsSysPutPut(t *testing.T) { vVal1Size := int64(len(val1.RawBytes)) require.EqualValues(t, vVal1Size, 10) + if disableSimpleValueEncoding { + vVal1Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal1Size, 17) + } val2 := roachpb.MakeValueFromString("longvalue") vVal2Size := int64(len(val2.RawBytes)) require.EqualValues(t, vVal2Size, 14) + if disableSimpleValueEncoding { + vVal2Size += emptyMVCCValueHeaderSize + require.EqualValues(t, vVal2Size, 21) + } expMS := enginepb.MVCCStats{ LastUpdateNanos: 1e9, - SysBytes: mKeySize + vKeySize + vVal1Size, // 11+12+10 = 33 + SysBytes: mKeySize + vKeySize + vVal1Size, // 11+12+(10[+7]) = 33[+7] SysCount: 1, } assertEqLocal(t, engine, "after first put", aggMS, &expMS) diff --git a/pkg/storage/mvcc_value.go b/pkg/storage/mvcc_value.go index 587a42f0973f..2edb4819b611 100644 --- a/pkg/storage/mvcc_value.go +++ b/pkg/storage/mvcc_value.go @@ -16,6 +16,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" + "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/errors" "github.com/cockroachdb/redact" @@ -136,10 +138,40 @@ func (v MVCCValue) SafeFormat(w redact.SafePrinter, _ rune) { w.Print(v.Value.PrettyPrint()) } +// When running a metamorphic build, disable the simple MVCC value encoding to +// prevent code from assuming that the MVCCValue encoding is identical to the +// roachpb.Value encoding. +var disableSimpleValueEncoding = util.ConstantWithMetamorphicTestBool( + "mvcc-value-disable-simple-encoding", false) + +// SkipIfSimpleValueEncodingDisabled skips this test during metamorphic runs +// that have disabled the simple MVCC value encoding. +func SkipIfSimpleValueEncodingDisabled(t skip.SkippableTest) { + t.Helper() + if disableSimpleValueEncoding { + skip.IgnoreLint(t, "disabled under metamorphic") + } +} + +var emptyValueHeader = func() enginepb.MVCCValueHeader { + var h enginepb.MVCCValueHeader + // Hacky: we don't have room in the mid-stack inlining budget in either + // encodedMVCCValueSize or EncodeMVCCValue to add to the simple encoding + // condition (e.g. `&& !disableSimpleValueEncoding`). So to have the same + // effect, we replace the empty value header with a header we never expect + // to see. We never expect LocalTimestamp to be set to MaxClockTimestamp + // because if it was set to that value, LocalTimestampNeeded would never + // return true. + if disableSimpleValueEncoding { + h.LocalTimestamp = hlc.MaxClockTimestamp + } + return h +}() + // encodedMVCCValueSize returns the size of the MVCCValue when encoded. //gcassert:inline func encodedMVCCValueSize(v MVCCValue) int { - if v.MVCCValueHeader == (enginepb.MVCCValueHeader{}) { + if v.MVCCValueHeader == emptyValueHeader { return len(v.Value.RawBytes) } return extendedPreludeSize + v.MVCCValueHeader.Size() + len(v.Value.RawBytes) @@ -149,7 +181,7 @@ func encodedMVCCValueSize(v MVCCValue) int { // comment on MVCCValue for a description of the encoding scheme. //gcassert:inline func EncodeMVCCValue(v MVCCValue) ([]byte, error) { - if v.MVCCValueHeader == (enginepb.MVCCValueHeader{}) { + if v.MVCCValueHeader == emptyValueHeader { // Simple encoding. Use the roachpb.Value encoding directly with no // modification. No need to re-allocate or copy. return v.Value.RawBytes, nil diff --git a/pkg/storage/mvcc_value_test.go b/pkg/storage/mvcc_value_test.go index fc8ee3473009..ee6ce7f1b7f7 100644 --- a/pkg/storage/mvcc_value_test.go +++ b/pkg/storage/mvcc_value_test.go @@ -110,6 +110,7 @@ func TestMVCCValueFormat(t *testing.T) { func TestEncodeDecodeMVCCValue(t *testing.T) { defer leaktest.AfterTest(t)() + SkipIfSimpleValueEncodingDisabled(t) var strVal, intVal roachpb.Value strVal.SetString("foo") diff --git a/pkg/storage/pebble_test.go b/pkg/storage/pebble_test.go index 0fcf6fac653e..d16eb8e30c70 100644 --- a/pkg/storage/pebble_test.go +++ b/pkg/storage/pebble_test.go @@ -1214,6 +1214,7 @@ func TestPebbleMVCCTimeIntervalCollectorAndFilter(t *testing.T) { func TestPebbleFlushCallbackAndDurabilityRequirement(t *testing.T) { defer leaktest.AfterTest(t)() + SkipIfSimpleValueEncodingDisabled(t) eng := createTestPebbleEngine() defer eng.Close() diff --git a/pkg/storage/sst.go b/pkg/storage/sst.go index 35672f4b1fca..5ddb724ee458 100644 --- a/pkg/storage/sst.go +++ b/pkg/storage/sst.go @@ -74,8 +74,8 @@ func CheckSSTConflicts( return enginepb.MVCCStats{}, err } - extKey, extValue := extIter.UnsafeKey(), extIter.UnsafeValue() - sstKey, sstValue := sstIter.UnsafeKey(), sstIter.UnsafeValue() + extKey, extValueRaw := extIter.UnsafeKey(), extIter.UnsafeValue() + sstKey, sstValueRaw := sstIter.UnsafeKey(), sstIter.UnsafeValue() // Keep seeking the iterators until both keys are equal. if cmp := bytes.Compare(extKey.Key, sstKey.Key); cmp < 0 { @@ -94,7 +94,14 @@ func CheckSSTConflicts( if !sstKey.IsValue() { return enginepb.MVCCStats{}, errors.New("SST keys must have timestamps") } - if len(sstValue) == 0 { + sstValue, ok, err := tryDecodeSimpleMVCCValue(sstValueRaw) + if !ok && err == nil { + sstValue, err = decodeExtendedMVCCValue(sstValueRaw) + } + if err != nil { + return enginepb.MVCCStats{}, err + } + if sstValue.IsTombstone() { return enginepb.MVCCStats{}, errors.New("SST values cannot be tombstones") } if !extKey.IsValue() { @@ -125,6 +132,13 @@ func CheckSSTConflicts( continue } } + extValue, ok, err := tryDecodeSimpleMVCCValue(extValueRaw) + if !ok && err == nil { + extValue, err = decodeExtendedMVCCValue(extValueRaw) + } + if err != nil { + return enginepb.MVCCStats{}, err + } // Allow certain idempotent writes where key/timestamp/value all match: // @@ -133,7 +147,7 @@ func CheckSSTConflicts( allowIdempotent := (!disallowShadowingBelow.IsEmpty() && disallowShadowingBelow.LessEq(extKey.Timestamp)) || (disallowShadowingBelow.IsEmpty() && disallowShadowing) if allowIdempotent && sstKey.Timestamp.Equal(extKey.Timestamp) && - bytes.Equal(extValue, sstValue) { + bytes.Equal(extValueRaw, sstValueRaw) { // This SST entry will effectively be a noop, but its stats have already // been accounted for resulting in double-counting. To address this we // send back a stats diff for these existing KVs so that we can subtract @@ -151,10 +165,10 @@ func CheckSSTConflicts( statsDiff.KeyCount-- // Update the stats to account for the skipped versioned key/value. - totalBytes = int64(len(sstValue)) + MVCCVersionTimestampSize + totalBytes = int64(len(sstValueRaw)) + MVCCVersionTimestampSize statsDiff.LiveBytes -= totalBytes statsDiff.KeyBytes -= MVCCVersionTimestampSize - statsDiff.ValBytes -= int64(len(sstValue)) + statsDiff.ValBytes -= int64(len(sstValueRaw)) statsDiff.ValCount-- sstIter.NextKey() @@ -171,9 +185,9 @@ func CheckSSTConflicts( // a WriteTooOldError -- that error implies that the client should // retry at a higher timestamp, but we already know that such a retry // would fail (because it will shadow an existing key). - if len(extValue) > 0 && (!disallowShadowingBelow.IsEmpty() || disallowShadowing) { + if !extValue.IsTombstone() && (!disallowShadowingBelow.IsEmpty() || disallowShadowing) { allowShadow := !disallowShadowingBelow.IsEmpty() && - disallowShadowingBelow.LessEq(extKey.Timestamp) && bytes.Equal(extValue, sstValue) + disallowShadowingBelow.LessEq(extKey.Timestamp) && bytes.Equal(extValueRaw, sstValueRaw) if !allowShadow { return enginepb.MVCCStats{}, errors.Errorf( "ingested key collides with an existing one: %s", sstKey.Key) @@ -194,10 +208,10 @@ func CheckSSTConflicts( // to take into account the existing KV pair. statsDiff.KeyCount-- statsDiff.KeyBytes -= int64(len(extKey.Key) + 1) - if len(extValue) > 0 { + if !extValue.IsTombstone() { statsDiff.LiveCount-- statsDiff.LiveBytes -= int64(len(extKey.Key) + 1) - statsDiff.LiveBytes -= int64(len(extValue)) + MVCCVersionTimestampSize + statsDiff.LiveBytes -= int64(len(extValueRaw)) + MVCCVersionTimestampSize } sstIter.NextKey() diff --git a/pkg/storage/sst_iterator.go b/pkg/storage/sst_iterator.go index 9bd8b49b1b39..a10951a7e483 100644 --- a/pkg/storage/sst_iterator.go +++ b/pkg/storage/sst_iterator.go @@ -13,7 +13,6 @@ package storage import ( "bytes" - "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/errors" "github.com/cockroachdb/pebble/sstable" "github.com/cockroachdb/pebble/vfs" @@ -107,7 +106,7 @@ func (r *sstIterator) SeekGE(key MVCCKey) { r.err = r.iter.Error() } if r.iterValid && r.err == nil && r.verify && r.mvccKey.IsValue() { - r.err = roachpb.Value{RawBytes: r.value}.Verify(r.mvccKey.Key) + r.verifyValue() } r.prevSeekKey.Key = append(r.prevSeekKey.Key[:0], r.mvccKey.Key...) r.prevSeekKey.Timestamp = r.mvccKey.Timestamp @@ -134,7 +133,7 @@ func (r *sstIterator) Next() { r.err = r.iter.Error() } if r.iterValid && r.err == nil && r.verify && r.mvccKey.IsValue() { - r.err = roachpb.Value{RawBytes: r.value}.Verify(r.mvccKey.Key) + r.verifyValue() } } @@ -158,3 +157,16 @@ func (r *sstIterator) UnsafeKey() MVCCKey { func (r *sstIterator) UnsafeValue() []byte { return r.value } + +// verifyValue verifies the checksum of the current value. +func (r *sstIterator) verifyValue() { + mvccValue, ok, err := tryDecodeSimpleMVCCValue(r.value) + if !ok && err == nil { + mvccValue, err = decodeExtendedMVCCValue(r.value) + } + if err != nil { + r.err = err + } else { + r.err = mvccValue.Value.Verify(r.mvccKey.Key) + } +}