sql: Prevent primary region being same as secondary region#87024
sql: Prevent primary region being same as secondary region#87024craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)
pkg/ccl/logictestccl/testdata/logic_test/secondary_region line 56 at r2 (raw file):
statement error pq: region .* is currently the secondary region ALTER DATABASE db SET PRIMARY REGION "ca-central-1"
what about ALTER DATABASE db SET SECONDARY REGION "same-as-primary"? can you add a test for that?
pkg/sql/alter_database.go line 739 at r2 (raw file):
} if prevRegionConfig.HasSecondaryRegion() && catpb.RegionName(n.n.PrimaryRegion) == prevRegionConfig.SecondaryRegion() {
can you also add validation to validateMultiRegion in pkg/sql/catalog/dbdesc/database_desc.go
e-mbrown
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/logictestccl/testdata/logic_test/secondary_region line 56 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
what about
ALTER DATABASE db SET SECONDARY REGION "same-as-primary"? can you add a test for that?
We have a test like that around ln 53
# Secondary region cannot be the current primary region.
statement error the secondary region cannot be the same as the current primary region
ALTER DATABASE db SET SECONDARY REGION "ap-southeast-2"
pkg/sql/alter_database.go line 739 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
can you also add validation to
validateMultiRegionin pkg/sql/catalog/dbdesc/database_desc.go
I think i've added validation
Code quote (from pkg/sql/catalog/dbdesc/database_desc.go):
if desc.RegionConfig.PrimaryRegion == desc.RegionConfig.SecondaryRegion {
vea.Report(errors.AssertionFailedf(
"primary region is same as secondary region on multi-region db %d", desc.GetID()))
}We found that the primary region could be assigned the same region as the secondary region. This commit adds an error to prevent that. Release justification: Low risk high benefit change to existing functionality Release note: None
Fix notice message to be make it clearer. Release justification: low risk changes to existing functionality Release note: None
rafiss
left a comment
There was a problem hiding this comment.
lgtm! nice
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status:
|
TFTR bors r=rafiss |
|
Build succeeded: |
fixes #86879
We found that the primary region could be assigned the same region as the secondary region. This commit adds an error to prevent that.
Release justification: Low risk high benefit change to existing functionality
Release note: None