From 7f0478358179c7788b07b665eb6339075fd5f8d5 Mon Sep 17 00:00:00 2001 From: sumeerbhola Date: Mon, 3 Oct 2022 15:07:27 +0000 Subject: [PATCH] storage: correctly check that a value is a tombstone We can't rely on the byte slice being of length 0. This was not a correctness bug, but limits a wider MVCC range tombstone. Release note: None --- pkg/storage/mvcc.go | 10 +-- pkg/storage/mvcc_history_test.go | 5 ++ .../delete_range_predicate_continue_tombstone | 72 +++++++++++++++++++ 3 files changed, 82 insertions(+), 5 deletions(-) create mode 100644 pkg/storage/testdata/mvcc_histories/delete_range_predicate_continue_tombstone diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 1acb1a3cc680..a61bcd208c3d 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -2971,7 +2971,11 @@ func MVCCPredicateDeleteRange( return false, false, false, roachpb.NewWriteTooOldError(endTime, k.Timestamp.Next(), k.Key.Clone()) } - if len(vRaw) == 0 { + v, err := DecodeMVCCValue(vRaw) + if err != nil { + return false, false, false, err + } + if v.IsTombstone() { // The latest version of the key is a point tombstone. return true, true, false, nil } @@ -2982,10 +2986,6 @@ func MVCCPredicateDeleteRange( } // TODO (msbutler): use MVCCValueHeader to match on job ID predicate - _, err = DecodeMVCCValue(vRaw) - if err != nil { - return false, false, false, err - } return true, false, false, nil } diff --git a/pkg/storage/mvcc_history_test.go b/pkg/storage/mvcc_history_test.go index f9b2a81cabcc..08adc1e976f5 100644 --- a/pkg/storage/mvcc_history_test.go +++ b/pkg/storage/mvcc_history_test.go @@ -154,6 +154,11 @@ var ( func TestMVCCHistories(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + // TODO(storage-team): this prevents us from easily finding bugs which + // incorrectly assume simple value encoding. We only find bugs where we are + // explicitly using the extended encoding by setting a localTs. One way to + // handle the different test output with extended value encoding would be to + // duplicate each test file for the two cases. storage.DisableMetamorphicSimpleValueEncoding(t) ctx := context.Background() diff --git a/pkg/storage/testdata/mvcc_histories/delete_range_predicate_continue_tombstone b/pkg/storage/testdata/mvcc_histories/delete_range_predicate_continue_tombstone new file mode 100644 index 000000000000..da83301d6f09 --- /dev/null +++ b/pkg/storage/testdata/mvcc_histories/delete_range_predicate_continue_tombstone @@ -0,0 +1,72 @@ +# Tests that MVCCPredicateDeleteRange will continue a run when encountering +# tombstones that do not satisfy the predicate. +# Sets up the following dataset, where x is a tombstone. +# T +# 3 a3 d3 f3 g3 +# 2 x x x +# 1 b1 c1 +# a b c d e f g +# +run stats ok +put k=b ts=1 v=b1 +put k=c ts=1 v=c1 +del k=b ts=2 localTs=1 +del k=c ts=2 localTs=1 +del k=e ts=2 localTs=1 +put k=a ts=3 v=a3 +put k=d ts=3 v=d3 +put k=f ts=3 v=f3 +put k=g ts=3 v=g3 +---- +>> put k=b ts=1 v=b1 +stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+7 live_count=+1 live_bytes=+21 +>> put k=c ts=1 v=c1 +stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+7 live_count=+1 live_bytes=+21 +>> del k=b ts=2 localTs=1 +del: "b": found key true +stats: key_bytes=+12 val_count=+1 val_bytes=+13 live_count=-1 live_bytes=-21 gc_bytes_age=+4508 +>> del k=c ts=2 localTs=1 +del: "c": found key true +stats: key_bytes=+12 val_count=+1 val_bytes=+13 live_count=-1 live_bytes=-21 gc_bytes_age=+4508 +>> del k=e ts=2 localTs=1 +del: "e": found key false +stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+13 gc_bytes_age=+2646 +>> put k=a ts=3 v=a3 +stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+7 live_count=+1 live_bytes=+21 +>> put k=d ts=3 v=d3 +stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+7 live_count=+1 live_bytes=+21 +>> put k=f ts=3 v=f3 +stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+7 live_count=+1 live_bytes=+21 +>> put k=g ts=3 v=g3 +stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+7 live_count=+1 live_bytes=+21 +>> at end: +data: "a"/3.000000000,0 -> /BYTES/a3 +data: "b"/2.000000000,0 -> {localTs=1.000000000,0}/ +data: "b"/1.000000000,0 -> /BYTES/b1 +data: "c"/2.000000000,0 -> {localTs=1.000000000,0}/ +data: "c"/1.000000000,0 -> /BYTES/c1 +data: "d"/3.000000000,0 -> /BYTES/d3 +data: "e"/2.000000000,0 -> {localTs=1.000000000,0}/ +data: "f"/3.000000000,0 -> /BYTES/f3 +data: "g"/3.000000000,0 -> /BYTES/g3 +stats: key_count=7 key_bytes=122 val_count=9 val_bytes=81 live_count=4 live_bytes=84 gc_bytes_age=11662 + +# Even though b, c, e do not satisfy the predicate, their latest versions are +# tombstones, so the run continues and we write [a,g)@4. +run stats ok +del_range_pred k=a end=z ts=4 startTime=2 rangeThreshold=3 +---- +>> del_range_pred k=a end=z ts=4 startTime=2 rangeThreshold=3 +stats: range_key_count=+1 range_key_bytes=+14 range_val_count=+1 live_count=-4 live_bytes=-84 gc_bytes_age=+9408 +>> at end: +rangekey: {a-g\x00}/[4.000000000,0=/] +data: "a"/3.000000000,0 -> /BYTES/a3 +data: "b"/2.000000000,0 -> {localTs=1.000000000,0}/ +data: "b"/1.000000000,0 -> /BYTES/b1 +data: "c"/2.000000000,0 -> {localTs=1.000000000,0}/ +data: "c"/1.000000000,0 -> /BYTES/c1 +data: "d"/3.000000000,0 -> /BYTES/d3 +data: "e"/2.000000000,0 -> {localTs=1.000000000,0}/ +data: "f"/3.000000000,0 -> /BYTES/f3 +data: "g"/3.000000000,0 -> /BYTES/g3 +stats: key_count=7 key_bytes=122 val_count=9 val_bytes=81 range_key_count=1 range_key_bytes=14 range_val_count=1 gc_bytes_age=21070