batcheval: add latch key protecting range key stats update#86608
batcheval: add latch key protecting range key stats update#86608craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Other commands needs latches as well. TBD. |
tbg
left a comment
There was a problem hiding this comment.
Looks good mod Erik's comments.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @erikgrinaker)
pkg/keys/keys.go line 1061 at r1 (raw file):
// RangeTombstoneStatsUpdateKey returns a system-local key for last used GC threshold on the // user keyspace. Reads and writes <= this timestamp will not be served. func (b RangeIDPrefixBuf) RangeTombstoneStatsUpdateKey() roachpb.Key {
Inconsistent name and comment.
pkg/kv/kvserver/batcheval/cmd_delete_range.go line 63 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
We only need to set this when
UseRangeTombstoneis set.We'll need this for all other operations that can affect range keys too, i.e.
ClearRange,RevertRange, andAddSSTable. The last one is a bit unfortunate -- we could maybe look at the given SST stats, but I'm not sure we want to rely on them for correctness.
Why is it unfortunate to do this for AddSSTable? Shouldn't matter much, since ranges undergoing GC are unlikely to also be receiving SSTs, no?
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/batcheval/cmd_delete_range.go line 63 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Why is it unfortunate to do this for
AddSSTable? Shouldn't matter much, since ranges undergoing GC are unlikely to also be receiving SSTs, no?
It's just a bit coarse to essentially block all AddSSTableactivity during MVCC range tombstone GC. This is for the general MVCC range tombstone GC, not for the fast path used by schema GC, so this could happen across live keyspace. E.g. if someone cancels an import, then retries it, and the GC triggers during the import. Not a huge deal though, should be fine (at least for now).
38085f1 to
722e313
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
I think we'll have to do this for EndTxn too, for splits/merges, where we peek around the split/merge point and adjust range key stats -- since GC is now latchless and the stats updates are non-commutative.
pkg/keys/keys.go
Outdated
|
|
||
| // RangeTombstoneStatsUpdateKey returns a range local key protecting range | ||
| // tombstone mvcc stats calculations during range tombstone GC. | ||
| func (b RangeIDPrefixBuf) RangeTombstoneStatsUpdateKey() roachpb.Key { |
| // Obtain a read only lock on range key stats update key to serialize with | ||
| // range tombstone GC requests. | ||
| latchSpans.AddNonMVCC(spanset.SpanReadOnly, roachpb.Span{ | ||
| Key: keys.MakeRangeIDPrefixBuf(rs.GetRangeID()).RangeTombstoneStatsUpdateKey(), |
There was a problem hiding this comment.
Let's add a helper for this, similarly to e.g. keys.RaftTruncatedStateKey(rangeID).
|
We discussed earlier that GC takes out a read latch on the range descriptor, but how exactly does that serialize with splits/merges? May well be right, I'd just like to be sure we're covered. |
|
I think you are right here. We are not getting enough latches to cover GC range ops. |
|
While we're here, could you update all the comments around |
722e313 to
f079490
Compare
f079490 to
249575f
Compare
Previously GC needed to get a read latch with max timestamp to ensure that range tombstones are not modified during GC. This is causing all writers to get stuck in queue while GC is validating request and removing range tombstone. This commit adds a dedicated latch key LocalRangeMVCCRangeKeyGCLockSuffix to address the problem. All range tombstone writers obtain this read latch on top of the write latches for the ranges they are interested to update. GC on the other hand will obtain write latch on that key. This approach allows point writers to proceed during GC, but will block new range tombstones from being written. Non conflicting writes of range tombstones could still proceed since their write latch ranges don't overlap. Release justification: this is a safe change as range tombstone behaviour is not enabled yet and the change is needed to address potential performance regressions. Release note: None
249575f to
d15851a
Compare
|
bors r=erikgrinaker |
|
Build succeeded: |
Previously GC needed to get a read latch with max timestamp to
ensure that range tombstones are not modified during GC. This
is causing all writers to get stuck in queue while GC is validating
request and removing range tombstone.
This commit adds a dedicated latch key
LocalRangeRangeTombstoneStatsUpdateLockSuffix to address the problem.
All range tombstone writers obtain this read latch on top of the write
latches for the ranges they are interested to update.
GC on the other hand will obtain write latch on that key. This
approach allows point writers to proceed during GC, but will block new
range tombstones from being written. Non conflicting writes of range
tombstones could still proceed since their write latch ranges don't
overlap.
Release justification: this is a safe change as range tombstone
behaviour is not enabled yet and the change is needed to address
potential performance regressions.
Release note: None
Fixes #84576
Fixes #86551