From 9bd8ca5bb8187a1ed5d244fa525ef1a40c1a3811 Mon Sep 17 00:00:00 2001 From: Mira Radeva Date: Tue, 9 Jan 2024 09:04:26 -0500 Subject: [PATCH] storage: fix a series of intent resolution bugs with ignored seq nums 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 --- pkg/storage/mvcc.go | 15 ++++++---- .../testdata/mvcc_histories/ignored_seq_nums | 29 ++++++------------- .../mvcc_histories/ignored_seq_nums_abort | 11 ++----- .../mvcc_histories/ignored_seq_nums_commit | 7 +---- 4 files changed, 23 insertions(+), 39 deletions(-) diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 548d230a12df..aaf86ccc8a86 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -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 @@ -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 @@ -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() @@ -5451,6 +5459,7 @@ func mvccResolveWriteIntent( ValueSizeKnown: true, ValueSize: uint32(origMetaValSize), }) + logicalOp = MVCCCommitIntentOpType } if err != nil { return lockNoop, err @@ -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, diff --git a/pkg/storage/testdata/mvcc_histories/ignored_seq_nums b/pkg/storage/testdata/mvcc_histories/ignored_seq_nums index bc92ce8839f1..bc01389c4010 100644 --- a/pkg/storage/testdata/mvcc_histories/ignored_seq_nums +++ b/pkg/storage/testdata/mvcc_histories/ignored_seq_nums @@ -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 @@ -708,9 +705,8 @@ resolve_intent: "p" -> resolved key = true get: "p" -> >> 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= txnDidNotUpdateMeta=false -data: "p"/50.000000000,0 -> /BYTES/a -logical op: *enginepb.MVCCCommitIntentOp + +logical op: *enginepb.MVCCAbortIntentOp run ok clear_range k=p end=-p @@ -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 @@ -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= 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= 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= 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= txnDidNotUpdateMeta=false +data: "q"/50.000000000,0 -> /BYTES/b run ok clear_range k=q end=-q @@ -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 @@ -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= 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= 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= 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= txnDidNotUpdateMeta=false +data: "r"/60.000000000,0 -> {localTs=50.000000000,0}/BYTES/b logical op: *enginepb.MVCCUpdateIntentOp diff --git a/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_abort b/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_abort index 7467a3a51455..0fd2d6368926 100644 --- a/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_abort +++ b/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_abort @@ -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 @@ -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" -> >> 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= txnDidNotUpdateMeta=false -data: "k"/11.000000000,0 -> /BYTES/a + logical op: *enginepb.MVCCWriteIntentOp logical op: *enginepb.MVCCUpdateIntentOp -logical op: *enginepb.MVCCCommitIntentOp +logical op: *enginepb.MVCCAbortIntentOp diff --git a/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_commit b/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_commit index 860f115b052d..d84ff0a7066d 100644 --- a/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_commit +++ b/pkg/storage/testdata/mvcc_histories/ignored_seq_nums_commit @@ -142,9 +142,6 @@ get: "k" -> # 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 @@ -179,9 +176,7 @@ resolve_intent: "b" -> resolved key = true get: "b" -> >> 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= 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