storage: tighten MVCCDeleteRangeUsingTombstone key span checks#87403
Conversation
|
This change is |
a3e595d to
464c5ee
Compare
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, and @tbg)
pkg/kv/kvserver/mvcc_gc_queue_test.go line 515 at r1 (raw file):
require.NoError(t, storage.MVCCPut(ctx, rw, ms, key, delTime, hlc.ClockTimestamp{}, roachpb.Value{}, nil)) } }
how about adding a require.Error test case that will fail with the new logic and that did not fail before?
e5805d8 to
3e9bfe4
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/mvcc_gc_queue_test.go line 515 at r1 (raw file):
Previously, sumeerbhola wrote…
how about adding a
require.Errortest case that will fail with the new logic and that did not fail before?
Yeah, may as well, done (TestMVCCHistories). This shouldn't be possible in production code though, since we always use an appropriate key prefix.
|
I think this will blow up with changes that were done to MVCCStats randomized tests as they use range deletions for local space as well if I'm not mistaken. |
I asked Tobi to address that in the original PR, so I think it should be fixed? But I'll make sure to adapt any failing tests otherwise. |
We've previously disallowed writing MVCC range tombstones across local keyspace. However, this check used `keys.IsLocal()`, which simply checks whether the key has the local prefix (`\x01`), so it was possible for callers to use e.g. `\x00` as a start key and span the local keyspace anyway. This patch changes the check to ensure the start key is at or after `keys.LocalMax`. Release justification: bug fixes and low-risk updates to new functionality Release note: None
3e9bfe4 to
b4b2ebe
Compare
`intentInterleavingIter` will error if given bounds that span from the local to the global keyspace. However, it only checked whether the start key satisfied `keys.IsLocal()`, i.e. that the key began with `\x01`. This meant that it was possible to give a start key with a `\x00` prefix that spanned across the local and global keyspaces, without triggering the error. This could cause the iterator to be incorrectly positioned in the lock table keyspace. This patch fixes the check by comparing the start key with `keys.LocalMax` instead. This does not appear to have a measurable impact on performance: ``` name old time/op new time/op delta MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24 2.39µs ± 1% 2.39µs ± 1% ~ (p=0.810 n=10+10) ``` Release justification: low risk, high benefit changes to existing functionality Release note: None
|
@sumeerbhola It turns out |
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:
pkg/storage/intent_interleaving_iter.go line 171 at r4 (raw file):
} func isLocal(k roachpb.Key) bool {
this isLocal method is also used in mvcc.go in CanGCEntireRange -- it looks like this behavior change is fine for that case too.
|
Looks like I won the flake jackpot with 3 different and unrelated CI failures. bors r=sumeerbhola |
|
Build succeeded: |
storage: tighten
MVCCDeleteRangeUsingTombstonekey span checksWe've previously disallowed writing MVCC range tombstones across local
keyspace. However, this check used
keys.IsLocal(), which simply checkswhether the key has the local prefix (
\x01), so it was possible forcallers to use e.g.
\x00as a start key and span the local keyspaceanyway.
This patch changes the check to ensure the start key is at or after
keys.LocalMax.Release justification: bug fixes and low-risk updates to new functionality
Release note: None
storage: fix
intentInterleavingIterlocal key detectionintentInterleavingIterwill error if given bounds that span from thelocal to the global keyspace. However, it only checked whether the start
key satisfied
keys.IsLocal(), i.e. that the key began with\x01.This meant that it was possible to give a start key with a
\x00prefixthat spanned across the local and global keyspaces, without triggering
the error. This could cause the iterator to be incorrectly positioned in
the lock table keyspace.
This patch fixes the check by comparing the start key with
keys.LocalMaxinstead. This does not appear to have a measurableimpact on performance:
Resolves #87489.
Release justification: low risk, high benefit changes to existing functionality
Release note: None