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.

Sign up for GitHub

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

Jump to bottom

kv: set destroy status before destroying data on subsumed replicas #55477

Merged
merged 2 commits into from
Oct 15, 2020
Merged

kv: set destroy status before destroying data on subsumed replicas #55477

merged 2 commits into from
Oct 15, 2020

Commits on Oct 14, 2020

  1. kv: remove obsolete TODO in maybeAcquireSnapshotMergeLock 

    The TODO discussed a merge followed by a split and alluded to an
optimization that could be used to avoid a snapshot. This doesn't make
sense to me. I think that's because the comment was written back in a
time where a range merged into and then back out of its left hand
neighbor would be the same range with the same ID. This is not the case
anymore, so I don't see how we could ever (reasonably) avoid a snapshot
in such a case.
    @nvanbenschoten
    nvanbenschoten committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    ed3010d View commit details
    Browse the repository at this point in the history

  2. kv: set destroy status before destroying data on subsumed replicas 

    This PR cleans up some handling around replica destruction that scared me
when working on cockroachdb#46329 and cockroachdb#55293. Specifically, there were cases during
merges where the destroy status on a replica would not be set until after
that replicas data was destroyed. This was true of merges applied through
entry application, although in those cases, no user data was removed so it
seems mostly harmless. More concerning is that is was true of merges applied
through snapshot application. This was extra concerning because snapshot
application can delete user data if a subsumed range is only partially
covered (see `clearSubsumedReplicaDiskData`). So we were hypothetically
risking user-visible correctness problems, especially now that we allow
follower reads through on subsumed ranges as of cockroachdb#51594.

This PR patches up these issues and then adds a few extra assertions that
enforce stricter preconditions for the functions at play during replica
destruction. Specifically, we now assert that the destroy status is set to
`destroyReasonRemoved` _before_ calling `preDestroyRaftMuLocked`. We also
assert that if `RemoveOptions.DestroyData` is false when passed to
`removeInitializedReplicaRaftMuLocked`, then the destroy status is also set.

This unification allows us to remove the `ignoreDestroyStatus` flag on
`RemoveOptions`, whose meaning is now exactly the inverse of `DestroyData`.
In hindsight, pushing on why this extra flag was needed and what new
information it was conveying to the function could have potentially caught
these issues a little earlier.

I think we'll want to backport this to v20.2, though probably in the first
patch release because there is some risk here (especially without sufficient
time to bake on master) and we aren't aware of seeing any correctness issues
from the combination of the bug fixed here and cockroachdb#51594.

Release note (bug fix): A hypothesized bug that could allow a follower
read to miss data on a range in the middle of being merged away into its
left-hand neighbor was fixed. The bug seems exceedingly unlikely to have
materialized in practice.
    @nvanbenschoten
    nvanbenschoten committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    8142440 View commit details
    Browse the repository at this point in the history