sql: block initial multi-region if zone configs have multi-region fields set #63129
Conversation
|
This change is |
otan
force-pushed
the
block_sc
branch
2 times, most recently
from
44b6677 to
48287b6
Compare
There was a problem hiding this comment.
I think someone more knowledgable should have a look at the set_zone_config.go and zone.go changes. @aayushshah15 could you have a look? Other than that, 
Reviewed 2 of 3 files at r1, 1 of 2 files at r2, 2 of 3 files at r3, 3 of 4 files at r4.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @otan)
pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 17 at r1 (raw file):
USE bad_db; SET override_multi_region_zone_config = true; ALTER DATABASE bad_db CONFIGURE ZONE DISCARD;
Nit: Don't we have this case covered down below? Can we just add the validation call down there?
pkg/sql/region_util.go, line 722 at r3 (raw file):
} // forEachMutableTableInDatabase calls the given function on every table
Nit: With this change, does this function belong in region_util.go?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/region_util.go, line 1165 at r6 (raw file):
desc catalog.TableDescriptor, ) (zonepb.ZoneConfig, error) { return *zonepb.NewZoneConfig(), nil
nit: Consider adding a comment here and in getExpectedDatabaseZoneConfig above that references the fact that we only care about the fields listed in MultiRegionZoneConfigFields and that we expect all of those to be empty in the SetInitialRegion case.
pkg/sql/region_util.go, line 1460 at r6 (raw file):
// the multi-region fields of the table's zone configuration // matches what is expected for the given table. func (p *planner) validateZoneConfigForMultiRegionTable(
ctx and dbDesc don't seem to be used by validateZoneConfigForMultiRegionTable. Do we have them here to conform to some sort of interface? If not, maybe we could get rid of them?
pkg/sql/set_zone_config.go, line 1008 at r1 (raw file):
} // getZoneConfigRaw looks up the zone config with the given IDs.
s/getZoneConfigRaw/getZoneConfigRawBatch
We previously fetched zone configs one-at-a-time for each table when doing zone config validation. Add a new `getZoneConfigRawBatch` function which fetches the zone configs in parallel. Also fix a bug where having no database descriptor for a multi-region zone config would panic on validation. Release note: None
This is in preparation for the validation of zone configs before the initial primary region. * Move the responsibility of getting the RegionConfig up to the struct of the validate interface. This ensures it is done only once, and keeps the abstraction in a nicer place. * Rename -type validateZoneConfigForMultiRegionErrorHandler to zoneConfigForMultiRegionValidator is an interface representing * Add methods `getExpectedDatabaseZoneConfig` and `getExpectedTableZoneConfig` to the validator interface, in case we want to customize these. * Refactored zoneConfigForMultiRegionDatabase to not return a pointer. Release note: None
…nDatabase In preparation for the zone config check change, this call must now allow non-multi-region databases too. Release note: None
…lds set Release note (sql change): Block being able to set the initial PRIMARY REGION of a database if any multi-region fields on any zone configs in the database have been set.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/region_util.go, line 1460 at r6 (raw file):
Previously, aayushshah15 (Aayush Shah) wrote…
ctxanddbDescdon't seem to be used byvalidateZoneConfigForMultiRegionTable. Do we have them here to conform to some sort of interface? If not, maybe we could get rid of them?
Done.
pkg/sql/set_zone_config.go, line 1008 at r1 (raw file):
Previously, aayushshah15 (Aayush Shah) wrote…
s/getZoneConfigRaw/getZoneConfigRawBatch
Done.
|
bors r=ajstorm,aayushshah15 |
|
Build succeeded: |
Refs: #63071
See individual commits for details.
Note I have not done overwriting. This is not going to be planned for 21.1 unless we deem it a GA-blocker.