Skip to content

Commit

Permalink
storage: fix a series of intent resolution bugs with ignored seq nums
Browse files Browse the repository at this point in the history 
Previously, the logic in mvccResolveWriteIntent was structured in such a
way that if an intent contained both ignored and non-ignored seq nums
in its intent history, the intent may end up being updated instead of
aborted or unmodified (see examples in 540efac).

This commit fixes the bugs by ensuring that the intent history is
modified only when an intent resolution update is not aborted, and the
update and the actual intent have the same epoch.

Fixes: #117553

Release note: None
  • Loading branch information
@miraradeva
miraradeva committed Jan 9, 2024
1 parent 1e6693c commit 9bd8ca5
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 39 deletions.
15 changes: 10 additions & 5 deletions pkg/storage/mvcc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5264,7 +5264,13 @@ func mvccResolveWriteIntent(
var rolledBackVal *MVCCValue
buf.newMeta = *meta
newMeta := &buf.newMeta
if len(update.IgnoredSeqNums) > 0 {
// Update the MVCC history only if:
// 1. There are IgnoredSeqNums present.
// 2. The update is not going to abort the intent; otherwise, the entire
// history will be removed anyway.
// 3. The epochs of the intent and the update match; otherwise the epochs may
// have different seq nums (and ignored seq nums).
if len(update.IgnoredSeqNums) > 0 && (commit || inProgress) && epochsMatch {
// 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
Expand Down Expand Up @@ -5431,6 +5437,7 @@ func mvccResolveWriteIntent(

// Update or remove the metadata key.
var metaKeySize, metaValSize int64
var logicalOp MVCCLogicalOpType
if !commit {
// Keep existing intent if we're updating it. We update the existing
// metadata's timestamp instead of using the supplied intent meta to avoid
Expand All @@ -5440,6 +5447,7 @@ func mvccResolveWriteIntent(
outcome = lockOverwritten
metaKeySize, metaValSize, err = buf.putLockMeta(
writer, metaKey.Key, lock.Intent, newMeta, true /* alreadyExists */)
logicalOp = MVCCUpdateIntentOpType
} else {
outcome = lockClearedByDelete
useSingleDelete := canSingleDelHelper.onCommitLock()
Expand All @@ -5451,6 +5459,7 @@ func mvccResolveWriteIntent(
ValueSizeKnown: true,
ValueSize: uint32(origMetaValSize),
})
logicalOp = MVCCCommitIntentOpType
}
if err != nil {
return lockNoop, err
Expand All @@ -5463,10 +5472,6 @@ func mvccResolveWriteIntent(
}

// Log the logical MVCC operation.
logicalOp := MVCCCommitIntentOpType
if pushed {
logicalOp = MVCCUpdateIntentOpType
}
writer.LogLogicalOp(logicalOp, MVCCLogicalOpDetails{
Txn: update.Txn,
Key: update.Key,
Expand Down
29 changes: 9 additions & 20 deletions pkg/storage/testdata/mvcc_histories/ignored_seq_nums
View file
Original file line number Diff line number Diff line change
Expand Up @@ -675,9 +675,6 @@ clear_range k=k end=p
# Try to update a pending intent with a lower epoch than the resolution.
# The intent should be aborted because the new epoch may not write it again.

# The test exposes a bug where the intent is updated instead of aborted.
# Moreover, it logs MVCCCommitIntentOp instead of MVCCUpdateIntentOp.

run ok
with t=G k=p
txn_begin ts=50
Expand Down Expand Up @@ -708,9 +705,8 @@ resolve_intent: "p" -> resolved key = true
get: "p" -> <no data>
>> at end:
txn: "G" meta={id=00000007 key="p" iso=Serializable pri=0.00000000 epo=1 ts=50.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=50.000000000,0 wto=false gul=0,0 isn=1
meta: "p"/0,0 -> txn={id=00000007 key="p" iso=Serializable pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=10} ts=50.000000000,0 del=false klen=12 vlen=6 mergeTs=<nil> txnDidNotUpdateMeta=false
data: "p"/50.000000000,0 -> /BYTES/a
logical op: *enginepb.MVCCCommitIntentOp
<no data>
logical op: *enginepb.MVCCAbortIntentOp

run ok
clear_range k=p end=-p
Expand All @@ -722,10 +718,6 @@ clear_range k=p end=-p
# The intent is not pushed, so it is not updated because the intent history
# should be updated only when the epochs match.

# The test exposes a bug where the intent is actually updated, as evident by the
# changed intent history.
# Moreover, it logs MVCCCommitIntentOp instead of MVCCUpdateIntentOp.

run ok
with t=H k=q
txn_begin ts=50
Expand Down Expand Up @@ -753,13 +745,12 @@ with t=H k=q
resolve_intent status=PENDING
check_intent
----
resolve_intent: "q" -> resolved key = true
meta: "q" -> txn={id=00000008 key="q" iso=Serializable pri=0.00000000 epo=1 ts=50.000000000,0 min=0,0 seq=10} ts=50.000000000,0 del=false klen=12 vlen=6 mergeTs=<nil> txnDidNotUpdateMeta=false
resolve_intent: "q" -> resolved key = false
meta: "q" -> txn={id=00000008 key="q" iso=Serializable pri=0.00000000 epo=1 ts=50.000000000,0 min=0,0 seq=20} ts=50.000000000,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}} mergeTs=<nil> txnDidNotUpdateMeta=false
>> at end:
txn: "H" meta={id=00000008 key="q" iso=Serializable pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=50.000000000,0 wto=false gul=0,0 isn=1
meta: "q"/0,0 -> txn={id=00000008 key="q" iso=Serializable pri=0.00000000 epo=1 ts=50.000000000,0 min=0,0 seq=10} ts=50.000000000,0 del=false klen=12 vlen=6 mergeTs=<nil> txnDidNotUpdateMeta=false
data: "q"/50.000000000,0 -> /BYTES/a
logical op: *enginepb.MVCCCommitIntentOp
meta: "q"/0,0 -> txn={id=00000008 key="q" iso=Serializable pri=0.00000000 epo=1 ts=50.000000000,0 min=0,0 seq=20} ts=50.000000000,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}} mergeTs=<nil> txnDidNotUpdateMeta=false
data: "q"/50.000000000,0 -> /BYTES/b

run ok
clear_range k=q end=-q
Expand All @@ -771,8 +762,6 @@ clear_range k=q end=-q
# The intent is pushed, so its timestamp is updated to help the pusher make progress.
# The intent history is not updated because the epochs don't match.

# The test exposes a bug where the intent history is actually updated.

run ok
with t=I k=r
txn_begin ts=50
Expand Down Expand Up @@ -802,9 +791,9 @@ with t=I k=r
check_intent
----
resolve_intent: "r" -> resolved key = true
meta: "r" -> txn={id=00000009 key="r" iso=Serializable pri=0.00000000 epo=1 ts=60.000000000,0 min=0,0 seq=10} ts=60.000000000,0 del=false klen=12 vlen=20 mergeTs=<nil> txnDidNotUpdateMeta=false
meta: "r" -> txn={id=00000009 key="r" iso=Serializable pri=0.00000000 epo=1 ts=60.000000000,0 min=0,0 seq=20} ts=60.000000000,0 del=false klen=12 vlen=20 ih={{10 /BYTES/a}} mergeTs=<nil> txnDidNotUpdateMeta=false
>> at end:
txn: "I" meta={id=00000009 key="r" iso=Serializable pri=0.00000000 epo=0 ts=60.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=50.000000000,0 wto=false gul=0,0 isn=1
meta: "r"/0,0 -> txn={id=00000009 key="r" iso=Serializable pri=0.00000000 epo=1 ts=60.000000000,0 min=0,0 seq=10} ts=60.000000000,0 del=false klen=12 vlen=20 mergeTs=<nil> txnDidNotUpdateMeta=false
data: "r"/60.000000000,0 -> {localTs=50.000000000,0}/BYTES/a
meta: "r"/0,0 -> txn={id=00000009 key="r" iso=Serializable pri=0.00000000 epo=1 ts=60.000000000,0 min=0,0 seq=20} ts=60.000000000,0 del=false klen=12 vlen=20 ih={{10 /BYTES/a}} mergeTs=<nil> txnDidNotUpdateMeta=false
data: "r"/60.000000000,0 -> {localTs=50.000000000,0}/BYTES/b
logical op: *enginepb.MVCCUpdateIntentOp
11 changes: 3 additions & 8 deletions pkg/storage/testdata/mvcc_histories/ignored_seq_nums_abort
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@
# The test exposes a bug where even though the transaction is aborted, its
# intent is updated instead of aborted.

# The test exposes a bug where even though the transaction is aborted, its
# intent is updated instead of aborted.
# Moreover, it logs MVCCCommitIntentOp instead of MVCCUpdateIntentOp.

run ok log-ops
with t=A k=k
txn_begin ts=11
Expand All @@ -21,11 +17,10 @@ with t=A k=k
put: lock acquisition = {k id=00000001 key="k" iso=Serializable pri=0.00000000 epo=0 ts=11.000000000,0 min=0,0 seq=10 Replicated Intent []}
put: lock acquisition = {k id=00000001 key="k" iso=Serializable pri=0.00000000 epo=0 ts=11.000000000,0 min=0,0 seq=20 Replicated Intent []}
resolve_intent: "k" -> resolved key = true
get: "k" -> /BYTES/a @11.000000000,0
get: "k" -> <no data>
>> at end:
txn: "A" meta={id=00000001 key="k" 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
meta: "k"/0,0 -> txn={id=00000001 key="k" iso=Serializable pri=0.00000000 epo=0 ts=11.000000000,0 min=0,0 seq=10} ts=11.000000000,0 del=false klen=12 vlen=6 mergeTs=<nil> txnDidNotUpdateMeta=false
data: "k"/11.000000000,0 -> /BYTES/a
<no data>
logical op: *enginepb.MVCCWriteIntentOp
logical op: *enginepb.MVCCUpdateIntentOp
logical op: *enginepb.MVCCCommitIntentOp
logical op: *enginepb.MVCCAbortIntentOp
7 changes: 1 addition & 6 deletions pkg/storage/testdata/mvcc_histories/ignored_seq_nums_commit
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,6 @@ get: "k" -> <no data>
# Commit an intent with a lower epoch than the resolution.
# The intent should be aborted because the new epoch may not write it again.

# The test exposes a bug where the intent is updated instead of aborted.
# Moreover, it logs MVCCCommitIntentOp instead of MVCCUpdateIntentOp.

run ok
with t=B k=b
txn_begin ts=12
Expand Down Expand Up @@ -179,9 +176,7 @@ resolve_intent: "b" -> resolved key = true
get: "b" -> <no data>
>> at end:
txn: "B" meta={id=00000003 key="b" iso=Serializable pri=0.00000000 epo=1 ts=12.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=12.000000000,0 wto=false gul=0,0 isn=1
meta: "b"/0,0 -> txn={id=00000003 key="b" iso=Serializable pri=0.00000000 epo=0 ts=12.000000000,0 min=0,0 seq=10} ts=12.000000000,0 del=false klen=12 vlen=6 mergeTs=<nil> txnDidNotUpdateMeta=false
data: "b"/12.000000000,0 -> /BYTES/a
data: "k"/11.000000000,0 -> /BYTES/c
data: "k/10"/11.000000000,0 -> /BYTES/10
data: "k/30"/11.000000000,0 -> /BYTES/30
logical op: *enginepb.MVCCCommitIntentOp
logical op: *enginepb.MVCCAbortIntentOp

0 comments on commit 9bd8ca5

Please sign in to comment.