Change deleteStore to check StoreHeader before bumping MetaDataVersionStamp#4354
Draft
ScottDugas wants to merge 5 commits into
Draft
Change deleteStore to check StoreHeader before bumping MetaDataVersionStamp#4354ScottDugas wants to merge 5 commits into
ScottDugas wants to merge 5 commits into
Conversation
…nStamp In particular the catalog uses the MetaDataVersion-based store cache, and when you delete a schema (as part of a test cleanup), the old deleteStore would invalidate the metadataversion cache, which would cause any catalog operations (which opened the store) to conflict. By being more restrictive, deleting stores that aren't caching the header means that we won't force a refresh on stores that are using the store header cache. Closes FoundationDB#4335
deleteStore reads STORE_INFO_KEY at snapshot isolation to avoid adding
a read conflict on every delete. This leaves a race with a concurrent
commit that flips the store to cacheable via setStateCacheability(true)
and bumps the meta-data version stamp: our snapshot read misses the
flip, we skip our own bump, and the delete lands. Sibling clients that
populated their caches from the flipper committed state then keep
serving reads out of a header for a store that no longer exists.
Fix: only on the branches where we decide NOT to bump (header absent,
or header parses as non-cacheable), add an explicit point read conflict
on STORE_INFO_KEY. That way:
- the common case (delete of a store that was and stays non-cacheable)
still contributes zero read conflicts, and
- the racy case (concurrent flip to cacheable) is detected: the
writer SET on STORE_INFO_KEY overlaps our added read conflict
and we lose the commit race.
The cacheable-header branch and the invalid-header fallback continue
to bump unconditionally without a read conflict -- bumping is safe
regardless of what a concurrent writer does.
Also updates the javadoc to describe the new semantics.
Adds a regression test (concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump)
that fails against the pre-fix implementation: it runs the two racing
transactions with pinned read versions, forces the writer to commit
first, and asserts that if the delete then commits the stamp must have
advanced past the writer post-commit value.
Closes FoundationDB#4335
A bunch of DRY and removing unnecessary logic. Also have a version of the conflict test with the commits in the other order
ScottDugas
force-pushed
the
subissue-4335
branch
from
ccbe336 to
0f1eefd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes
deleteStoreto only bump the MetaDataVersionStamp if it is deleting a store that is cacheable.By being more restrictive, deleting stores that aren't caching the header means that we won't force a refresh on stores that are using the store header cache. This can be pretty valuable, since you would generally use the store state cache for stores that have a lot of activity, but rarely change store state. Those are unlikely to be deleted, and would be most impacted by a spurious meta-data version-stamp bump.
In particular the catalog uses the MetaDataVersion-based store cache, and when you delete a schema (as part of a test cleanup), the old
deleteStorewould invalidate the metaDataVersion cache, which would cause any catalog operations (which opened the store) to conflict.Closes #4335