engine: track MVCC stats in MVCCClearTimeRange#39251
engine: track MVCC stats in MVCCClearTimeRange#39251craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt)
pkg/storage/engine/mvcc.go, line 1970 at r1 (raw file):
k := it.UnsafeKey() if ms != nil {
nit: if ms != nil && len(clearedKey) > 0
pkg/storage/engine/mvcc.go, line 1983 at r1 (raw file):
} else { valueSize := int64(len(it.Value())) restoredMeta.Deleted = valueSize == 0
Same thing, put this in the order that we have it in ComputeStatsGo and call Reset first. Also, don't we need to set the timestamp?
pkg/storage/engine/mvcc.go, line 2016 at r1 (raw file):
return nil, err } continue
Add a comment here.
pkg/storage/engine/mvcc.go, line 2026 at r1 (raw file):
clearMatchingKey(k) clearedKey = append(clearedKey[:0], k.Key...) clearedMeta.ValBytes = int64(len(it.UnsafeValue()))
nit: put this in the same order that we have it in ComputeStatsGo, and also call Clear before doing this.
pkg/storage/engine/mvcc.go, line 2039 at r1 (raw file):
if ms != nil && len(clearedKey) > 0 { origMetaKeySize := int64(len(clearedKey) + 1)
Add a comment here.
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc.go, line 1970 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit:
if ms != nil && len(clearedKey) > 0
Done.
pkg/storage/engine/mvcc.go, line 1983 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Same thing, put this in the order that we have it in
ComputeStatsGoand callResetfirst. Also, don't we need to set the timestamp?
Huh, I assumed those Resets were needed because that same instance of the struct is reused for unmarhsalling, so it could have anything in any field. Here we're writing to, and only to, the same fields on every iteration, so there shouldn't be anything else to Reset.
I can add it anyway, but it seems like superfluous overhead?
As far as timestamp: updateStatsOnAbort has the restored nanos passed in separately and doesn't read the restoredMeta.Timestamp field, so didn't seem like we had any reason to write it.
pkg/storage/engine/mvcc.go, line 2016 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Add a comment here.
Actually decided against adding this continue after all -- I don't think it matters in the keys we're expecting, but I'd rather still hit the else below .
pkg/storage/engine/mvcc.go, line 2026 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: put this in the same order that we have it in
ComputeStatsGo, and also callClearbefore doing this.
Done (though the Reset seems superfluous?)
pkg/storage/engine/mvcc.go, line 2039 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Add a comment here.
Done.
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc.go, line 1983 at r1 (raw file):
I can add it anyway, but it seems like superfluous overhead?
I guess you're right. Feel free to remove.
pkg/storage/engine/mvcc.go, line 1971 at r2 (raw file):
if ms != nil && len(clearedKey) > 0 { metaKeySize := int64(len(clearedKey) + 1) // EncodedSize().
If we maintained clearedKey as an MVCCKey then could we actually use EncodedSize() instead of mocking it out? That seems much cleaner.
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc.go, line 1983 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I can add it anyway, but it seems like superfluous overhead?
I guess you're right. Feel free to remove.
Done.
pkg/storage/engine/mvcc.go, line 1971 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
If we maintained
clearedKeyas anMVCCKeythen could we actually useEncodedSize()instead of mocking it out? That seems much cleaner.
Indeed, done.
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc.go, line 2021 at r3 (raw file):
clearMatchingKey(k) if ms != nil { clearedKey.Key = append(clearedKey.Key[:0], k.Key...)
Don't we also need to maintain the clearedKeys timestamp here?
clearedKey.Key = append(clearedKey.Key[:0], k.Key...)
clearedKey.Timestamp = k.Timestamp
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc.go, line 2021 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Don't we also need to maintain the
clearedKeys timestamp here?clearedKey.Key = append(clearedKey.Key[:0], k.Key...) clearedKey.Timestamp = k.Timestamp
No, we don't want it: we were just keeping Key in the []byte before so the MVCCKey wrapper around it is just to get EncodedSize method -- we never read Timestamp and we don't want a non-zero one causing EncodedSize treat it as a value either.
I originally thought I could leave this to a follow-up change, and just recalculate via iteration for now. However the batch does not reflect the ClearRanges we add so that iteration-based calculation doesn't work. Instead, this switches to incrementally calculate stats changes using the method usually used when an intent is aborted (just without the intent count changes). Release note: none.
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc.go, line 2021 at r3 (raw file):
Previously, dt (David Taylor) wrote…
No, we don't want it: we were just keeping Key in the
[]bytebefore so the MVCCKey wrapper around it is just to get EncodedSize method -- we never read Timestamp and we don't want a non-zero one causingEncodedSizetreat it as a value either.
renamed it to clearedMetaKey to hopefully clear up the confusion
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc.go, line 2021 at r3 (raw file):
Previously, dt (David Taylor) wrote…
renamed it to
clearedMetaKeyto hopefully clear up the confusion
Ah I see, that makes it a lot clearer. Thanks.
|
bors r+ |
Build failed (retrying...) |
39251: engine: track MVCC stats in MVCCClearTimeRange r=dt a=dt I originally thought I could leave this to a follow-up change, and just recalculate via iteration for now. However the batch does not reflect the ClearRanges we add so that iteration-based calculation doesn't work. Instead, this switches to incrementally calculate stats changes using the method usually used when an intent is aborted (just without the intent count changes). Release note: none. Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Build succeeded |
Fixes cockroachdb#39390. Fixes cockroachdb#39407. Fixes cockroachdb#39370. Fixes cockroachdb#39419. A bug introduced in cockroachdb#39251 was causing the IntentCount field in MVCCStats to get out of sync. I was expecting `TestMVCCStatsRandomized/sys/inline=false` to easily catch the bug, but it wasn't. It turns out that the test was updating the transaction's write timestamp but not its read timestamp, which was causing it to hit this error: https://github.com/cockroachdb/cockroach/blob/41cb28e85575292926790e79912c893d13e1054d/pkg/storage/engine/mvcc.go#L1349 This was preventing the randomized test from performing the series of steps we needed it to to hit the bug. This commit fixes the bug, fixes the randomized test so that it would have caught it, and adds a targeted regression test for this case. Release note: None
39206: sql: add RevertTable wrapper for RevertRangeRequest r=dt a=dt (first commit is #39251) kv,batcheval: fix RevertRange to work with DistSender and ResumeSpans This patch includes several changes required to make RevertRange requests get along with DistSender: 1) add RevertRange to the explicit check for maxkey-supported requests. 2) makes RevertRangeResponse combinable, however combining assumes that at-most-one reply returned a ResumeSpan, so it also 3) sets NumKeys to MaxKeys in RevertRange when returning a ResumeSpan, to stop distsender from sending any further requests that might also hit a resume and then trip up combining. Together these make it possible to send batches of RevertRange requests via DistSender and get back resume spans. Release note: none. sql: add RevertTables helper for reverting a Table via RevertRange This adds a helper that can revert a collection of tables to a target timestamp using the RevertRange RPC. It checks that the tables do not overlap the key space of another table not being reverted (i.e. that all the reverting tables are interleaved, if at all, only with each other) and that the tables are offline. It then reverts the table spans using RevertRange, including handling the ResumeSpan pagination. Release note: none. 39427: storage/engine: don't decrement IntentCount on system intent abort r=nvanbenschoten a=nvanbenschoten Fixes #39390. Fixes #39407. Fixes #39370. Fixes #39419. A bug introduced in #39251 was causing the IntentCount field in MVCCStats to get out of sync. I was expecting `TestMVCCStatsRandomized/sys/inline=false` to easily catch the bug, but it wasn't. It turns out that the test was updating the transaction's write timestamp but not its read timestamp, which was causing it to hit this error: https://github.com/cockroachdb/cockroach/blob/41cb28e85575292926790e79912c893d13e1054d/pkg/storage/engine/mvcc.go#L1349 This was preventing the randomized test from performing the series of steps we needed it to hit the bug. This commit fixes the bug, fixes the randomized test so that it would have caught it, and adds a targeted regression test for this case. Release note: None Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
I originally thought I could leave this to a follow-up change, and just recalculate via iteration for now.
However the batch does not reflect the ClearRanges we add so that iteration-based calculation doesn't work.
Instead, this switches to incrementally calculate stats changes using the method usually used when an intent is aborted (just without the intent count changes).
Release note: none.