Skip to content

Commit

Permalink
engine: fix divergent MVCCStats updates
Browse files Browse the repository at this point in the history
Found while investigating cockroachdb#20554 (though this does not at all fix the
issue discovered there; that will be a separate PR).

We were incorrectly updating GCBytesAge when moving an intent. The old code was
pretty broken (and, with hindsight, it is still after this commit, as is exposed
by the various child commits). Its main problem was that it failed to account
for the `GCBytesAge` difference that would result from moving the intent due
to the incorrect assumption that the size of the intent would be the same. The
code was also somewhat intransparent and an attempt has been made to improve
its legibility.

This change raises the question of what to do about the divergent stats that
exist in real-world clusters. As part of addressing cockroachdb#20554, we'll need a
mechanism to correct the stats anyway, and so I will defer its introduction.

You'll want to view this diff with `?w=1` (insensitive to whitespace changes).

Release note: None.
  • Loading branch information
tbg committed Dec 26, 2017
1 parent cc94e70 commit 3472b33
Show file tree
Hide file tree
Showing 3 changed files with 217 additions and 153 deletions.
38 changes: 27 additions & 11 deletions pkg/storage/engine/mvcc.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,32 +302,48 @@ func updateStatsOnResolve(
commit bool,
) enginepb.MVCCStats {
var ms enginepb.MVCCStats

// NB: this is logging for ongoing work on #20554.
if false {
defer func() {
log.Infof(context.TODO(), "onResolve\n"+
"orig: ts=%d metaKeySize=%d metaValSize=%d KeyBytes=%d ValBytes=%d\n"+
"meta: ts=%d metaKeySize=%d metaValSize=%d KeyBytes=%d ValBytes=%d\n"+
"%+v",
orig.Timestamp.WallTime, origMetaKeySize, origMetaValSize, orig.KeyBytes, orig.ValBytes,
meta.Timestamp.WallTime, metaKeySize, metaValSize, meta.KeyBytes, meta.ValBytes,
&ms)
}()
}

// In this case, we're only removing the contribution from having the
// meta key around from orig.Timestamp to meta.Timestamp.
ms.AgeTo(orig.Timestamp.WallTime)
sys := isSysLocal(key)

// Always zero.
keyDiff := metaKeySize - origMetaKeySize
// This is going to be nonpositive: the old meta key was
// real, the new one is implicit.
valDiff := metaValSize - origMetaValSize

if sys {
ms.SysBytes += keyDiff + valDiff
ms.SysBytes += metaKeySize + metaValSize - origMetaValSize - origMetaKeySize
ms.AgeTo(meta.Timestamp.WallTime)
} else {
if !meta.Deleted {
ms.LiveBytes += keyDiff + valDiff
ms.LiveBytes += metaKeySize + metaValSize - origMetaValSize - origMetaKeySize
}
ms.KeyBytes += keyDiff
ms.ValBytes += valDiff
// At orig.Timestamp, the original meta key disappears.
ms.KeyBytes -= origMetaKeySize + orig.KeyBytes
ms.ValBytes -= origMetaValSize + orig.ValBytes

// If committing, subtract out intent counts.
if commit {
ms.IntentBytes -= (meta.KeyBytes + meta.ValBytes)
ms.IntentCount--
}

ms.AgeTo(meta.Timestamp.WallTime)

// At meta.Timestamp, the new meta key appears.
ms.KeyBytes += metaKeySize + meta.KeyBytes
ms.ValBytes += metaValSize + meta.ValBytes
}
ms.AgeTo(meta.Timestamp.WallTime)
return ms
}

Expand Down
Loading

0 comments on commit 3472b33

Please sign in to comment.