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: #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
miraradeva committed Oct 30, 2023
1 parent b7fb29c commit 4175f47
Show file tree
Hide file tree
Showing 2 changed files with 191 additions and 17 deletions.
35 changes: 18 additions & 17 deletions pkg/storage/mvcc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,13 +644,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 @@ -669,12 +662,13 @@ 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
}
Expand Down Expand Up @@ -715,6 +709,14 @@ func updateStatsOnResolve(
ms.LiveBytes += (metaKeySize + metaValSize) + (meta.KeyBytes + meta.ValBytes)
}

// If the value is deleted or a delete was rolled back, adjust LiveCount.
if !meta.Deleted && orig.Deleted {
ms.LiveCount++
}
if meta.Deleted && !orig.Deleted {
ms.LiveCount--
}

if !commit {
// If not committing, the intent reappears (but at meta.Timestamp).
//
Expand Down Expand Up @@ -5077,13 +5079,15 @@ 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)
removeIntent, rolledBackVal, err = mvccMaybeRewriteIntentHistory(ctx, writer, intent.IgnoredSeqNums, newMeta, latestKey)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -5125,9 +5129,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
173 changes: 173 additions & 0 deletions pkg/storage/testdata/mvcc_histories/ignored_seq_nums_delete
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
# When a write is rolled back and either the rolled back or preceding value is a
# delete, there are 3 cases to consider:
# 0. Rolled back put with a previous put (no actual delete; this is tested in
# ignored_seq_nums_commit).
# 1. Rolled back delete with a previous put.
# 2. Rolled back put with a preceding delete.
# 3. Rolled back delete with a preceding delete.
# We also test a case where the rolled back writes are surrounded by other puts.

# Rolled back delete with a previous put.
run stats ok
with t=A
txn_begin ts=11
txn_step seq=10
put k=k v=a
txn_step seq=20
del k=k
txn_step seq=30
txn_ignore_seqs seqs=(19-21)
resolve_intent k=k
----
>> put k=k v=a t=A
stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+56 live_count=+1 live_bytes=+70 intent_count=+1 intent_bytes=+18 lock_count=+1 lock_age=+89
>> del k=k t=A
del: "k": found key true
stats: val_bytes=+4 live_count=-1 live_bytes=-70 gc_bytes_age=+6586 intent_bytes=-6
>> resolve_intent k=k t=A
resolve_intent: "k" -> resolved key = true
stats: val_bytes=-54 live_count=+1 live_bytes=+20 gc_bytes_age=-6586 intent_count=-1 intent_bytes=-12 lock_count=-1 lock_age=-89
>> at end:
txn: "A" meta={id=00000001 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=11.000000000,0 min=0,0 seq=30} lock=true stat=PENDING rts=11.000000000,0 wto=false gul=0,0 isn=1
data: "k"/11.000000000,0 -> /BYTES/a
stats: key_count=1 key_bytes=14 val_count=1 val_bytes=6 live_count=1 live_bytes=20

run ok
scan k=k end=-k
get k=k
----
scan: "k"-"l" -> <no data>
get: "k" -> <no data>

run ok
clear_range k=k end=-k
txn_remove t=A
----
>> at end:
<no data>

# Rolled back put with a preceding delete.
run stats ok
with t=A
txn_begin ts=11
txn_step seq=10
del k=k
txn_step seq=20
put k=k v=a
txn_step seq=30
txn_ignore_seqs seqs=(19-21)
resolve_intent k=k
----
>> del k=k t=A
del: "k": found key false
stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+50 gc_bytes_age=+5696 intent_count=+1 intent_bytes=+12 lock_count=+1 lock_age=+89
>> put k=k v=a t=A
stats: val_bytes=+10 live_count=+1 live_bytes=+74 gc_bytes_age=-5696 intent_bytes=+6
>> resolve_intent k=k t=A
resolve_intent: "k" -> resolved key = true
stats: val_bytes=-60 live_count=-1 live_bytes=-74 gc_bytes_age=+1246 intent_count=-1 intent_bytes=-18 lock_count=-1 lock_age=-89
>> at end:
txn: "A" meta={id=00000002 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=11.000000000,0 min=0,0 seq=30} lock=true stat=PENDING rts=11.000000000,0 wto=false gul=0,0 isn=1
data: "k"/11.000000000,0 -> /<empty>
stats: key_count=1 key_bytes=14 val_count=1 gc_bytes_age=1246

run ok
scan k=k end=-k
get k=k
----
scan: "k"-"l" -> <no data>
get: "k" -> <no data>

run ok
clear_range k=k end=-k
txn_remove t=A
----
>> at end:
<no data>

# Rolled back delete with a preceding delete.
run stats ok
with t=A
txn_begin ts=11
txn_step seq=10
del k=k
txn_step seq=20
del k=k
txn_step seq=30
txn_ignore_seqs seqs=(19-21)
resolve_intent k=k
----
>> del k=k t=A
del: "k": found key false
stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+50 gc_bytes_age=+5696 intent_count=+1 intent_bytes=+12 lock_count=+1 lock_age=+89
>> del k=k t=A
del: "k": found key false
stats: val_bytes=+4 gc_bytes_age=+356
>> resolve_intent k=k t=A
resolve_intent: "k" -> resolved key = true
stats: val_bytes=-54 gc_bytes_age=-4806 intent_count=-1 intent_bytes=-12 lock_count=-1 lock_age=-89
>> at end:
txn: "A" meta={id=00000003 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=11.000000000,0 min=0,0 seq=30} lock=true stat=PENDING rts=11.000000000,0 wto=false gul=0,0 isn=1
data: "k"/11.000000000,0 -> /<empty>
stats: key_count=1 key_bytes=14 val_count=1 gc_bytes_age=1246

run ok
scan k=k end=-k
get k=k
----
scan: "k"-"l" -> <no data>
get: "k" -> <no data>

run ok
clear_range k=k end=-k
txn_remove t=A
----
>> at end:
<no data>

# Rolled back delete and put with a previous values.
run stats ok
with t=A
txn_begin ts=11
txn_step seq=10
put k=k v=a
txn_step seq=20
del k=k
txn_step seq=30
put k=k v=b
txn_step seq=40
put k=k v=c
txn_step seq=50
txn_ignore_seqs seqs=(19-31)
resolve_intent k=k
----
>> put k=k v=a t=A
stats: key_count=+1 key_bytes=+14 val_count=+1 val_bytes=+56 live_count=+1 live_bytes=+70 intent_count=+1 intent_bytes=+18 lock_count=+1 lock_age=+89
>> del k=k t=A
del: "k": found key true
stats: val_bytes=+4 live_count=-1 live_bytes=-70 gc_bytes_age=+6586 intent_bytes=-6
>> put k=k v=b t=A
stats: val_bytes=+12 live_count=+1 live_bytes=+86 gc_bytes_age=-6586 intent_bytes=+6
>> put k=k v=c t=A
stats: val_bytes=+12 live_bytes=+12
>> resolve_intent k=k t=A
resolve_intent: "k" -> resolved key = true
stats: val_bytes=-78 live_bytes=-78 intent_count=-1 intent_bytes=-18 lock_count=-1 lock_age=-89
>> at end:
txn: "A" meta={id=00000004 key=/Min iso=Serializable pri=0.00000000 epo=0 ts=11.000000000,0 min=0,0 seq=50} lock=true stat=PENDING rts=11.000000000,0 wto=false gul=0,0 isn=1
data: "k"/11.000000000,0 -> /BYTES/c
stats: key_count=1 key_bytes=14 val_count=1 val_bytes=6 live_count=1 live_bytes=20

run ok
scan k=k end=-k
get k=k
----
scan: "k"-"l" -> <no data>
get: "k" -> <no data>

run ok
clear_range k=k end=-k
txn_remove t=A
----
>> at end:
<no data>

0 comments on commit 4175f47

Please sign in to comment.