From 3628ed90e89437f0a74da0570e1c1dca24ca8b26 Mon Sep 17 00:00:00 2001 From: Bilal Akhtar Date: Wed, 29 Apr 2020 11:07:22 -0400 Subject: [PATCH] storage,libroach: Check for MaxKeys when reading from intent history We weren't checking for MaxKeys (or TargetBytes) being reached in the case where we read from intent history in the MVCC scanner. All other cases go through addAndAdvance(), which had these checks. Almost certainly fixes #46652. Would be very surprised if it was something else. Release note (bug fix): Fixes a bug where a read operation in a transaction would error out for exceeding the maximum count of results returned. --- c-deps/libroach/mvcc.h | 6 + pkg/storage/pebble_mvcc_scanner.go | 8 ++ pkg/storage/testdata/mvcc_histories/max_keys | 111 ++++++++++++++++++ .../testdata/mvcc_histories/target_bytes | 51 ++++++++ 4 files changed, 176 insertions(+) diff --git a/c-deps/libroach/mvcc.h b/c-deps/libroach/mvcc.h index c7a326b90397..b766a0563b2a 100644 --- a/c-deps/libroach/mvcc.h +++ b/c-deps/libroach/mvcc.h @@ -407,6 +407,12 @@ template class mvccScanner { // sequence, read that value. const bool found = getFromIntentHistory(); if (found) { + if (target_bytes_ > 0 && kvs_->NumBytes() >= target_bytes_) { + max_keys_ = kvs_->Count(); + } + if (max_keys_ > 0 && kvs_->Count() == max_keys_) { + return false; + } return advanceKey(); } // 11. If no value in the intent history has a sequence number equal to diff --git a/pkg/storage/pebble_mvcc_scanner.go b/pkg/storage/pebble_mvcc_scanner.go index 1f2627efce96..331109d4b5a5 100644 --- a/pkg/storage/pebble_mvcc_scanner.go +++ b/pkg/storage/pebble_mvcc_scanner.go @@ -409,6 +409,14 @@ func (p *pebbleMVCCScanner) getAndAdvance() bool { // history that has a sequence number equal to or less than the read // sequence, read that value. if p.getFromIntentHistory() { + if p.targetBytes > 0 && p.results.bytes >= p.targetBytes { + // When the target bytes are met or exceeded, stop producing more + // keys. We implement this by reducing maxKeys to the current + // number of keys. + // + // TODO(bilal): see if this can be implemented more transparently. + p.maxKeys = p.results.count + } if p.maxKeys > 0 && p.results.count == p.maxKeys { return false } diff --git a/pkg/storage/testdata/mvcc_histories/max_keys b/pkg/storage/testdata/mvcc_histories/max_keys index d60598152680..3ce655dede5f 100644 --- a/pkg/storage/testdata/mvcc_histories/max_keys +++ b/pkg/storage/testdata/mvcc_histories/max_keys @@ -86,3 +86,114 @@ scan: "e" -> /BYTES/val-e @0.000000001,0 scan: "e" -> /BYTES/val-e @0.000000001,0 scan: "c" -> /BYTES/val-c @0.000000001,0 scan: "a" -> /BYTES/val-a @0.000000001,0 + +# Regression test for #46652: Test appropriate termination when the MaxKeys-th +# key is in the intent history. + +run ok +with t=A ts=11,0 max=3 + txn_begin + txn_step seq=10 + put k=k v=a + put k=l v=a + put k=m v=a + put k=n v=a + txn_step seq=20 + put k=k v=b + put k=l v=b + put k=m v=b + put k=n v=b + txn_step seq=30 + put k=k v=c + put k=l v=c + put k=m v=c + put k=n v=c + txn_step seq=20 + scan k=k end=o + scan k=k end=o reverse=true +---- +scan: "k" -> /BYTES/b @0,0 +scan: "l" -> /BYTES/b @0,0 +scan: "m" -> /BYTES/b @0,0 +scan: resume span ["n","o") +scan: "n" -> /BYTES/b @0,0 +scan: "m" -> /BYTES/b @0,0 +scan: "l" -> /BYTES/b @0,0 +scan: resume span ["k","k\x00") +>> at end: +txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=20} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 +data: "a"/0.000000001,0 -> /BYTES/val-a +data: "aa"/0.000000002,0 -> / +data: "aa"/0.000000001,0 -> /BYTES/val-aa +data: "c"/0.000000001,0 -> /BYTES/val-c +data: "e"/0.000000001,0 -> /BYTES/val-e +meta: "k"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "k"/0.000000011,0 -> /BYTES/c +meta: "l"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "l"/0.000000011,0 -> /BYTES/c +meta: "m"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "m"/0.000000011,0 -> /BYTES/c +meta: "n"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "n"/0.000000011,0 -> /BYTES/c + +run ok +with t=A ts=11,0 max=3 + txn_step seq=30 + resolve_intent k=k status=COMMITTED + resolve_intent k=l status=COMMITTED + resolve_intent k=m status=COMMITTED + resolve_intent k=n status=COMMITTED +---- +>> at end: +txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 +data: "a"/0.000000001,0 -> /BYTES/val-a +data: "aa"/0.000000002,0 -> / +data: "aa"/0.000000001,0 -> /BYTES/val-aa +data: "c"/0.000000001,0 -> /BYTES/val-c +data: "e"/0.000000001,0 -> /BYTES/val-e +data: "k"/0.000000011,0 -> /BYTES/c +data: "l"/0.000000011,0 -> /BYTES/c +data: "m"/0.000000011,0 -> /BYTES/c +data: "n"/0.000000011,0 -> /BYTES/c + +# Same case as above, except with a committed value at the key after MaxKeys. + +run ok +with t=B ts=12,0 max=3 + txn_begin + txn_step seq=10 + put k=k v=a + put k=l v=a + put k=m v=a + txn_step seq=20 + put k=k v=b + put k=l v=b + put k=m v=b + txn_step seq=30 + put k=k v=c + put k=l v=c + put k=m v=c + txn_step seq=20 + scan k=k end=o +---- +scan: "k" -> /BYTES/b @0,0 +scan: "l" -> /BYTES/b @0,0 +scan: "m" -> /BYTES/b @0,0 +scan: resume span ["n","o") +>> at end: +txn: "B" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000012,0 min=0,0 seq=20} lock=true stat=PENDING rts=0.000000012,0 wto=false max=0,0 +data: "a"/0.000000001,0 -> /BYTES/val-a +data: "aa"/0.000000002,0 -> / +data: "aa"/0.000000001,0 -> /BYTES/val-aa +data: "c"/0.000000001,0 -> /BYTES/val-c +data: "e"/0.000000001,0 -> /BYTES/val-e +meta: "k"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000012,0 min=0,0 seq=30} ts=0.000000012,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "k"/0.000000012,0 -> /BYTES/c +data: "k"/0.000000011,0 -> /BYTES/c +meta: "l"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000012,0 min=0,0 seq=30} ts=0.000000012,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "l"/0.000000012,0 -> /BYTES/c +data: "l"/0.000000011,0 -> /BYTES/c +meta: "m"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000012,0 min=0,0 seq=30} ts=0.000000012,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "m"/0.000000012,0 -> /BYTES/c +data: "m"/0.000000011,0 -> /BYTES/c +data: "n"/0.000000011,0 -> /BYTES/c diff --git a/pkg/storage/testdata/mvcc_histories/target_bytes b/pkg/storage/testdata/mvcc_histories/target_bytes index e2048338aa9e..71377fd432c0 100644 --- a/pkg/storage/testdata/mvcc_histories/target_bytes +++ b/pkg/storage/testdata/mvcc_histories/target_bytes @@ -246,3 +246,54 @@ scan: "c" -> /BYTES/ghijkllkjihg @0.000000123,45 scan: "aa" -> / @0.000000250,1 scan: "a" -> /BYTES/abcdef @0.000000123,45 scan: 98 bytes (target 65) + +# Regression test for a bug simiar to #46652: Test appropriate termination when +# the TargetBytes-th kv pair is in the intent history. + +run ok +with t=A ts=11,0 targetbytes=32 + txn_begin + txn_step seq=10 + put k=k v=a + put k=l v=a + put k=m v=a + put k=n v=a + txn_step seq=20 + put k=k v=b + put k=l v=b + put k=m v=b + put k=n v=b + txn_step seq=30 + put k=k v=c + put k=l v=c + put k=m v=c + put k=n v=c + txn_step seq=20 + scan k=k end=o + scan k=k end=o reverse=true +---- +scan: "k" -> /BYTES/b @0,0 +scan: "l" -> /BYTES/b @0,0 +scan: resume span ["m","o") +scan: 32 bytes (target 32) +scan: "n" -> /BYTES/b @0,0 +scan: "m" -> /BYTES/b @0,0 +scan: resume span ["k","l\x00") +scan: 32 bytes (target 32) +>> at end: +txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=20} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 +data: "a"/0.000000123,45 -> /BYTES/abcdef +data: "a"/0.000000001,0 -> /BYTES/nevergoingtobeseen +data: "aa"/0.000000250,1 -> / +data: "aa"/0.000000001,0 -> /BYTES/willbetombstoned +data: "c"/0.000000123,45 -> /BYTES/ghijkllkjihg +data: "e"/0.000000123,45 -> /BYTES/mnopqr +data: "e"/0.000000001,0 -> /BYTES/sameasabove +meta: "k"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "k"/0.000000011,0 -> /BYTES/c +meta: "l"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "l"/0.000000011,0 -> /BYTES/c +meta: "m"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "m"/0.000000011,0 -> /BYTES/c +meta: "n"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}} +data: "n"/0.000000011,0 -> /BYTES/c