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: validate primary / secondary region localities at end of txn #103362

Merged
merged 1 commit into from
May 18, 2023

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented May 16, 2023

Previously, if a database was restored with skip_localities, there was no way to modify this database to set the primary region since validation would kick in too early during the statement. This meant fixing the regions in a restored database was impossible if the primary region was no longer valid. To address this, this patch, delays locality validation till the end of the transaction.

Fixes: #103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION did not validate transactionally, which could prevent cleaning up removed regions after a restore.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@fqazi fqazi force-pushed the restoreInvalidRegionAlter branch from fa000a7 to 3db9d6b Compare May 16, 2023 00:17
@fqazi fqazi marked this pull request as ready for review May 16, 2023 01:05
@fqazi fqazi requested review from a team as code owners May 16, 2023 01:05
@fqazi fqazi requested review from adityamaru and removed request for a team May 16, 2023 01:05
@chengxiong-ruan
Copy link
Contributor

While I think this works, is it possible to do the validation in the descriptor validation which is done at commit as well.

@fqazi
Copy link
Collaborator Author

fqazi commented May 16, 2023

@chengxiong-ruan Let me change this a tiny bit. I think we can have a model where we have an interface for validating zone config validation that gets pushed down into collections.

@fqazi fqazi force-pushed the restoreInvalidRegionAlter branch from 3db9d6b to d3137d1 Compare May 16, 2023 15:32
@fqazi fqazi requested a review from chengxiong-ruan May 16, 2023 15:33
pkg/sql/conn_executor_exec.go Outdated Show resolved Hide resolved
Previously, if a database was restored with skip_localities,
there was no way to modify this database to set the primary
region since validation would kick in too early during the
statement. This meant fixing the regions in a restored database
was impossible if the primary region was no longer valid. To
address this, this patch, delays locality validation till the
end of the transaction.

Fixes: cockroachdb#103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION
did not validate transactionally, which could prevent cleaning up
removed regions after a restore.
@fqazi fqazi force-pushed the restoreInvalidRegionAlter branch from d3137d1 to 8b9f052 Compare May 18, 2023 19:02
@fqazi
Copy link
Collaborator Author

fqazi commented May 18, 2023

@chengxiong-ruan TFTR!

@fqazi
Copy link
Collaborator Author

fqazi commented May 18, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented May 18, 2023

Build succeeded:

@craig craig bot merged commit fe71b7c into cockroachdb:master May 18, 2023
@blathers-crl
Copy link

blathers-crl bot commented May 18, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 8b9f052 to blathers/backport-release-22.2-103362: 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 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@fqazi
Copy link
Collaborator Author

fqazi commented May 18, 2023

blathers backport 23.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: after restore with skip_localities its not possible to change primary regions
3 participants