storage,libroach: Check for MaxKeys when reading from intent history #48160
Conversation
|
This change is |
There was a problem hiding this comment.
In the Release note: s/resturned/returned/g.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @itsbilal, @jbowens, @petermattis, and @tbg)
pkg/storage/testdata/mvcc_histories/max_keys, line 90 at r1 (raw file):
scan: "a" -> /BYTES/val-a @0.000000001,0 # Regression test for #46652: Test appropriate termination where the MaxKeys-th
Nit: s/where/when/g (applies to similar comments elsewhere)
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 cockroachdb#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.
itsbilal
force-pushed
the
maxkeys-intent-history-fix
branch
from
31e683b
to
3628ed9
Compare
There was a problem hiding this comment.
TFTR! Addressed the nits. Also changed values in test cases to a,b,c for the 3 sequences instead of a,a,a, just to make it clear what sequence number is being read.
Reviewable status:
|
Thanks for the fix! The accounting around those limits looks brittle. Is there something non-invasive that we could do? Like making it impossible to add to the result set without going through the check. |
|
|
|
No opposition from me. Will be easier to fix when there's only the pebble version to update anyway |
|
❌ The GitHub CI (Cockroach) build has failed on 3628ed90. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
TFTRs! bors r+ |
Build failed (retrying...) |
Build succeeded |
|
There's an |
|
Good point. @tbg what do you think about removing ^ ? There's some value to still keeping it there (since this is a bit of a speculative fix), but I'm happy with removing it too. |
Previously, when doing an MVCCGet with the Pebble MVCC scanner, we would advance keys after getting (or not getting) the value we're looking for. In some cases this could result in a seek, such as if there are too many revisions of that MVCC key. Seeks are costly and at best avoided when it's ultimatey unnecessary. Another side effect of doing this seek was that it would trip an assertion in spanSet.Iterator that checked if all reads were happening on allowed key spans. This caused TestDumpData to fail on Pebble (but not on RocksDB since the RocksDB MVCC scanner is specialized in C++ and doesn't flow through the same assertion for seeks). There's one additional change, stemming from a follow-up conversation in cockroachdb#48160, around unifying logic for terminating an MVCC scan. The getFromIntentHistory() case also calls into addAndAdvance() now. Fixes cockroachdb#48147. Release note: None.
Previously, when doing an MVCCGet with the Pebble MVCC scanner, we would advance keys after getting (or not getting) the value we're looking for. In some cases this could result in a seek, such as if there are too many revisions of that MVCC key. Seeks are costly and at best avoided when it's ultimatey unnecessary. Another side effect of doing this seek was that it would trip an assertion in spanSet.Iterator that checked if all reads were happening on allowed key spans. This caused TestDumpData to fail on Pebble (but not on RocksDB since the RocksDB MVCC scanner is specialized in C++ and doesn't flow through the same assertion for seeks). There's one additional change, stemming from a follow-up conversation in cockroachdb#48160, around unifying logic for terminating an MVCC scan. The getFromIntentHistory() case also calls into addAndAdvance() now. Fixes cockroachdb#48147. Release note: None.
48239: storage: Don't advance keys in MVCCGet with Pebble r=itsbilal a=itsbilal Previously, when doing an MVCCGet with the Pebble MVCC scanner, we would advance keys after getting (or not getting) the value we're looking for. In some cases this could result in a seek, such as if there are too many revisions of that MVCC key. Seeks are costly and at best avoided when it's ultimatey unnecessary. Another side effect of doing this seek was that it would trip an assertion in spanSet.Iterator that checked if all reads were happening on allowed key spans. This caused TestDumpData to fail on Pebble (but not on RocksDB since the RocksDB MVCC scanner is specialized in C++ and doesn't flow through the same assertion for seeks). There's one additional change, stemming from a follow-up conversation in #48160, around unifying logic for terminating an MVCC scan. The getFromIntentHistory() case also calls into addAndAdvance() now. Fixes #48147. Release note: None. Co-authored-by: Bilal Akhtar <bilal@cockroachlabs.com>
Backport of cockroachdb#48160. 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 cockroachdb#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.
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.