Skip to content

Commit

Permalink
storage: correctly update the stats for rolled back deletes
Browse files Browse the repository at this point in the history
Previously, a delete on a key rolled back by a savepoint did not
correctly update the MVCCStats when the intent for that key was
resolved. In particular, if the rollback revealed a non-deleted value
for the key, the stats considered that key/value non-live and included
other inaccuracies. This is an example from the output of kvnemesis,
which caught the bug:

```
engine stats:
{ContainsEstimates:0 LastUpdateNanos:1698322873881949000 LockAge:0
GCBytesAge:0 LiveBytes:771 LiveCount:13 KeyBytes:1350 KeyCount:34
ValBytes:714 ValCount:53 IntentBytes:8 IntentCount:0 LockBytes:0
LockCount:0 RangeKeyCount:0 RangeKeyBytes:0 RangeValCount:0
RangeValBytes:0 SysBytes:254 SysCount:6 AbortSpanBytes:0}

computed stats:
{ContainsEstimates:0 LastUpdateNanos:1698322873881949000 LockAge:0
GCBytesAge:0 LiveBytes:161 LiveCount:1 KeyBytes:0 KeyCount:0
ValBytes:8 ValCount:0 IntentBytes:8 IntentCount:0 LockBytes:0
LockCount:0 RangeKeyCount:0 RangeKeyBytes:0 RangeValCount:0
RangeValBytes:0 SysBytes:0 SysCount:0 AbortSpanBytes:0}
```

This patch fixes the logic responsible for computing stats upon intent
resolution and adds testing for rolled back deletes.

Informs: cockroachdb#97444

Release note (bug fix): Rolled back deletes no longer cause a
discrepancy between computed stats and the actual stored values.
  • Loading branch information
miraradeva committed Nov 6, 2023
1 parent 1b2091f commit d44d555
Show file tree
Hide file tree
Showing 3 changed files with 493 additions and 24 deletions.
34 changes: 17 additions & 17 deletions pkg/storage/mvcc.go
Original file line number Diff line number Diff line change
Expand Up @@ -699,13 +699,6 @@ func updateStatsOnResolve(
return ms
}

// An intent can't turn from deleted to non-deleted and vice versa while being
// resolved.
if orig.Deleted != meta.Deleted {
log.Fatalf(context.TODO(), "on resolve, original meta was deleted=%t, but new one is deleted=%t",
orig.Deleted, meta.Deleted)
}

// In the main case, we had an old intent at orig.Timestamp, and a new intent
// or value at meta.Timestamp. We'll walk through the contributions below,
// taking special care for LockAge and GCBytesAge.
Expand All @@ -724,14 +717,16 @@ func updateStatsOnResolve(
ms.KeyBytes -= origMetaKeySize + orig.KeyBytes
ms.ValBytes -= origMetaValSize + orig.ValBytes

// If the old intent is a deletion, then the key already isn't tracked
// in LiveBytes any more (and the new intent/value is also a deletion).
// If we're looking at a non-deletion intent/value, update the live
// bytes to account for the difference between the previous intent and
// the new intent/value.
if !meta.Deleted {
// Next, we adjust LiveBytes based on meta.Deleted and orig.Deleted.
// Note that LiveBytes here corresponds to ts = orig.Timestamp.WallTime.
// LiveBytes at ts = meta.Timestamp.WallTime is adjusted below.
// If the original value was deleted, there is no need to adjust the
// contribution of the original key and value to LiveBytes. Otherwise, we need
// to subtract the original key and value's contribution from LiveBytes.
if !orig.Deleted {
ms.LiveBytes -= origMetaKeySize + origMetaValSize
ms.LiveBytes -= orig.KeyBytes + orig.ValBytes
ms.LiveCount--
}

// LockAge is always accrued from the intent's own timestamp on.
Expand Down Expand Up @@ -768,6 +763,7 @@ func updateStatsOnResolve(
// The new meta key appears.
if !meta.Deleted {
ms.LiveBytes += (metaKeySize + metaValSize) + (meta.KeyBytes + meta.ValBytes)
ms.LiveCount++
}

if !commit {
Expand Down Expand Up @@ -5163,13 +5159,20 @@ func mvccResolveWriteIntent(
// remains, the rolledBackVal is set to a non-nil value.
var rolledBackVal *MVCCValue
var err error
buf.newMeta = *meta
newMeta := &buf.newMeta
if len(intent.IgnoredSeqNums) > 0 {
// NOTE: mvccMaybeRewriteIntentHistory mutates its meta argument.
// TODO(nvanbenschoten): this is an awkward interface. We shouldn't
// be mutating meta and we shouldn't be restoring the previous value
// here. Instead, this should all be handled down below.
var removeIntent bool
removeIntent, rolledBackVal, err = mvccMaybeRewriteIntentHistory(ctx, writer, intent.IgnoredSeqNums, meta, latestKey)
// Instead of modifying meta, pass a copy of it (newMeta), which will be the
// starting point for the updated metadata. It's important to keep meta
// intact and corresponding to the stats in ms to ensure that later on (in
// updateStatsOnResolve) the stats will be updated correctly based on the
// old meta (meta) and the new meta (newMeta).
removeIntent, rolledBackVal, err = mvccMaybeRewriteIntentHistory(ctx, writer, intent.IgnoredSeqNums, newMeta, latestKey)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -5211,9 +5214,6 @@ func mvccResolveWriteIntent(
// is because removeIntent implies rolledBackVal == nil, pushed == false, and
// commit == false.
if commit || pushed || rolledBackVal != nil {
buf.newMeta = *meta
newMeta := &buf.newMeta

// The intent might be committing at a higher timestamp, or it might be
// getting pushed.
newTimestamp := intent.Txn.WriteTimestamp
Expand Down
Loading

0 comments on commit d44d555

Please sign in to comment.