Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: intent resolution incorrectly updates intents with ignored seq nums #117553

Closed
miraradeva opened this issue Jan 9, 2024 · 0 comments · Fixed by #117541
Closed

storage: intent resolution incorrectly updates intents with ignored seq nums #117553

miraradeva opened this issue Jan 9, 2024 · 0 comments · Fixed by #117541
Assignees
Labels
A-kv Anything in KV that doesn't belong in a more specific category. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-kv KV Team

Comments

@miraradeva
Copy link
Contributor

miraradeva commented Jan 9, 2024

Describe the problem

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.

To Reproduce

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.

Additional context
This has most likely been an issue since the ignored seq nums were introduced into the MVCC code in f6a4dc5.

Jira issue: CRDB-35245

@miraradeva miraradeva added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jan 9, 2024
@miraradeva miraradeva self-assigned this Jan 9, 2024
@miraradeva miraradeva added A-kv Anything in KV that doesn't belong in a more specific category. T-kv KV Team labels Jan 9, 2024
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 9, 2024
Rename the intent argument to the mvccResolveWriteIntent function 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
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 9, 2024
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
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 9, 2024
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 added a commit to miraradeva/cockroach that referenced this issue Jan 9, 2024
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
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 9, 2024
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
craig bot pushed a commit that referenced this issue Jan 10, 2024
115888: sql: update pausable portal todo r=rharding6373 a=rharding6373

Epic: None
Informs: #115887

Release note: None

116830: hlc: remove the Synthetic field from Timestamp and LegacyTimestamp r=erikgrinaker a=nvanbenschoten

Closes #101938.

This PR completes the work to remove the `Synthetic` field from `Timestamp` and `LegacyTimestamp`. It removes the remaining uses, removes the fields from the proto definitions, and removes all access to the fields in methods.

Release note: None



117429: revertccl: ALTER VIRTUAL CLUSTER RESET DATA r=dt a=dt

This enables resetting a virtual cluster's data to a prior timestamp.

This is possible if the prior timestamp is still retained in the mvcc
history of the virtual cluster, the virtual cluster has stopped service,
and is run by a user with the MANAGEVIRTUALCLUSTER (or admin) privilege
in the system tenant.

Revisions of data in the system tenant newer than the target time to
which it is being reset are destroyed, reverting the tenant to the state
it was in as of the time reverted to. Destroyed revisions are not
recoverable; once a tenant has been reset to a timestamp, it cannot be
'un-reset' back to a higher timestamp.

Release note (cluster virtualization): Added a new 'flashback' command
to revert a virtual cluster to an earlier state using ALTER VIRTUAL
CLUSTER RESET DATA.


Epic: CRDB-34233.

117541: storage: fix a series of intent resolution bugs with ignored seq nums r=nvanbenschoten a=miraradeva

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

117563: distsql: improve columnar operator test harness for decimals r=yuzefovich a=yuzefovich

We recently merged a change to add decimals with different numbers of trailing zeroes in the "interesting datums" set, and it made some existing tests fail because they used direct string comparison for equality. This commit adjusts the test harness to be smarter for decimals.

Fixes: #117543.

Release note: None

Co-authored-by: rharding6373 <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Mira Radeva <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
@craig craig bot closed this as completed in 9bd8ca5 Jan 10, 2024
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 10, 2024
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
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 10, 2024
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.

This commit only reproduces the bugs.

Informs: cockroachdb#117553

Release note: None
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 10, 2024
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 added a commit to miraradeva/cockroach that referenced this issue Jan 10, 2024
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
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 10, 2024
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
miraradeva added a commit to miraradeva/cockroach that referenced this issue Jan 10, 2024
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
@github-project-automation github-project-automation bot moved this to Closed in KV Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv Anything in KV that doesn't belong in a more specific category. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-kv KV Team
Projects
No open projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

1 participant