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

sql: Remove the need for FORCE in transactions with multiple multi-region operations #61551

Closed
ajstorm opened this issue Mar 5, 2021 · 2 comments · Fixed by #62354
Closed
Assignees
Labels
A-multiregion Related to multi-region C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker

Comments

@ajstorm
Copy link
Collaborator

ajstorm commented Mar 5, 2021

Currently we need to specify FORCE for all transactions which contain multiple multi-region operations. This is because validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser first generates a zone configuration based on the RegionConfig, and compares that with the existing zone configuration to see if the user has made any modification. The problem with transactions where there are multiple multi-region operations is that the first operation will modify the RegionConfig, but the corresponding zone configuration is not modified until the subsequent schema change job is complete. As a result, validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser thinks that the user has made a change to the zone configuration, when in fact it's just keying off a partial transaction state.

The fix for this problem is to leverage the refactoring going into #60620 to then generate the comparison zone configuration based on the state which existed at the beginning of the transaction. That way, validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser will only indicate that a FORCE is required if a user modified zone configuration state existed at the beginning of the transaction.

@ajstorm ajstorm added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-multiregion Related to multi-region labels Mar 5, 2021
@ajstorm ajstorm self-assigned this Mar 5, 2021
@arulajmani
Copy link
Collaborator

I think this should follow quite nicely once #61585 goes in. All we need to do is construct a RegionConfig using RegionNamesIncludingTransitioning instead of RegionNames in validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser. There is, ofcourse, the whole part of cleaning up tests after though.

@arulajmani
Copy link
Collaborator

Correction on the comment above -- we can't use RegionNamesIncludingTransitioning, instead we must "reverse" the transitions to a stable state. Concretely, all regions that are in READ ONLY mode and being removed need to be on the RegionConfig constructed and all regions that are in READ ONLY mode and are being added shouldn't be on the RegionConfig.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multiregion Related to multi-region C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants