storage: fix a series of intent resolution bugs with ignored seq nums #117541
Conversation
|
This change is |
miraradeva
force-pushed
the
mira-aborted-savepoint
branch
from
4576121
to
50e51fb
Compare
nvanbenschoten
added
backport-23.1.x
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jbowens and @miraradeva)
pkg/storage/mvcc.go line 5183 at r1 (raw file):
iter MVCCIterator, ms *enginepb.MVCCStats, update roachpb.LockUpdate,
Should we perform this same rename in MVCCResolveWriteIntent and MVCCResolveWriteIntentRange?
pkg/storage/mvcc.go line 5440 at r3 (raw file):
// Update or remove the metadata key. var metaKeySize, metaValSize int64 var logicalOp MVCCLogicalOpType
Nice additional find!
Rename the intent argument to the mvccResolveWriteIntent, MVCCResolveWriteIntent, and MVCCResolveWriteIntentRange functions to make it clear that it refers to the state passed in via ResolveIntent, and not the current value of the stored intent. Informs: cockroachdb#117553 Release note: None
The logic in mvccResolveWriteIntent is structured in such a way that if an intent contains both ignored and non-ignored seq nums in its intent history, the intent may end up being updated instead of aborted or unmodified. For the following examples, assume the intent has a history ["a", "b"] where "a" is written first, and "b" is ignored. 1. The intent resolution has status aborted. Instead of aborting the intent, it is modified to have value "a" and an empty intent history. 2. The intent resolution has status pending, and the intent has a lower epoch than the resolution. The intent should be aborted because the new epoch may not write it again. Instead, it is updated with value "a" and an empty intent history. 3. Same as 2 but the intent resolution has status committed. 4. The intent resolution has status pending, the intent is not pushed and has a higher epoch than the resolution. The intent should not be updated because the intent history is updated only when the epochs match. Instead, it is updated with value "a" and an empty intent history. 5. Same as 4 but the intent is pushed. The intent should be updated to bump its timestamp in order to unblock the pusher. The intent history should not be updated for the same reason as in 3. Instead, the intent is updated with value "a" and an empty intent history. Additionally, in cases 1, 2, 3 and 4 above, the resulting intent is not committed but a MVCCCommitIntentOp is logged erroneously. This commit only reproduces the bugs. Informs: cockroachdb#117553 Release note: None
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: cockroachdb#117553 Release note: None
miraradeva
force-pushed
the
mira-aborted-savepoint
branch
from
50e51fb
to
9bd8ca5
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc.go line 5440 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Nice additional find!
Forgot to mention, this change around the logical op type was not necessary. The logical op inconsistency was fixed by the (commit || inProgress) && epochsMatch condition above. But I think the change here makes it less likely that we'll end up in an inconsistent state in the future.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4, 3 of 3 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status:
|
bors r=nvanbenschoten |
|
Build failed (retrying...): |
|
Build succeeded: |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from c5adf13 to blathers/backport-release-23.1-117541: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. error creating merge commit from c5adf13 to blathers/backport-release-23.2-117541: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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 9f00f2a5505).
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