Very exciting!

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajstorm, @andreimatei, and @otan)

pkg/sql/region_util.go, line 171 at r1 (raw file):

// theoretical (2-2-1) voting replica configuration, where the primary region
// has 2 voting replicas and the next closest region has another 2. This allows
// for stable write latencies even under single node failures.

stable read and write latencies

pkg/sql/region_util.go, line 187 at r1 (raw file):

	switch regionConfig.SurvivalGoal {
	case descpb.SurvivalGoal_ZONE_FAILURE:
		// numVoters in the region with affinity + 1 replica for every other region.

I don't know how ambitious you want to be here, but a bit of exposition and maybe even a diagram for each case would be fantastic for any future reader trying to understand the rationale behind these choices. As you well know, there were a lot of considerations that went into them and they're deceptively subtle.

pkg/sql/region_util.go, line 189 at r1 (raw file):

		// numVoters in the region with affinity + 1 replica for every other region.
		numVoters = numVotersForZoneSurvival
		numReplicas = numRegions + numVoters - 1

nit: numVoters + numRegions - 1 or even numVoters + (numRegions - 1) would be a little more clear and would match better with the REGION_FAILURE case.

pkg/sql/region_util.go, line 194 at r1 (raw file):

		// other region.
		numVoters = numVotersForRegionSurvival
		numReplicas = numVoters/2 + numRegions - 1

nit: consider pulling out a helper called minorityQuorum or something similar to perform this floor division. Right now, you do it in a few places.

pkg/sql/region_util.go, line 195 at r1 (raw file):

		numVoters = numVotersForRegionSurvival
		numReplicas = numVoters/2 + numRegions - 1
		if numReplicas < numVoters {

Give this a comment.

pkg/sql/region_util.go, line 218 at r1 (raw file):

	case descpb.SurvivalGoal_ZONE_FAILURE:
		return []zonepb.ConstraintsConjunction{
			{

Could you leave a comment about why we don't need the NumReplicas attribute? Basically just copying over the comment on the field and what it means to be unspecified.

pkg/sql/region_util_test.go, line 171 at r1 (raw file):

	for _, tc := range testCases {
		t.Run(tc.desc, func(t *testing.T) {
			res, _ := zoneConfigForDatabase(tc.regionConfig)

Let's check the error using require.NoError.

pkg/sql/logictest/testdata/logic_test/multiregion, line 813 at r1 (raw file):

                            voter_constraints = '[+region=us-east-1]',
                            lease_preferences = '[[+region=us-east-1]]'

In a follow-up commit, should we add some extra test cases for setups that aren't exercised here? For instance, I don't think any of these tests have more than 3 regions, which exercises some interesting cases for region survivability.

I'm still in the process of adding a commit to this PR that contains some 5-region logic tests, but this is RFAL besides that.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/sql/region_util.go, line 171 at r1 (raw file):

Done.

pkg/sql/region_util.go, line 187 at r1 (raw file):

I added some exposition along with basic diagrams inside synthesizeVoterConstraints, and I moved the numVotersFor{Region,Zone}Survival constants to live inside getNumVotersAndReplicas. LMK if that's not what you had in mind, or if its missing some bit of insight that you think should be mentioned.

pkg/sql/region_util.go, line 189 at r1 (raw file):

Done.

pkg/sql/region_util.go, line 194 at r1 (raw file):

Done.

pkg/sql/region_util.go, line 195 at r1 (raw file):

Done. LMK if this is not what you had in mind.

pkg/sql/region_util.go, line 218 at r1 (raw file):

Done.

pkg/sql/region_util_test.go, line 171 at r1 (raw file):

Done.

minor nits for changes in general, makes sense to me but haven't thought about these constraints as deep!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajstorm, @andreimatei, and @nvanbenschoten)

pkg/sql/region_util.go, line 97 at r2 (raw file):

}

// zoneConfigFromRegionConfigForDatabase generates a desired ZoneConfig based

this longer name was deliberate as zoneConfigForTable does not make it clear it is for multi region setup. i'd prefer to keep it this way for other function names too.
(unless we moved this to another package which makes this clear /shrug)

pkg/sql/region_util.go, line 106 at r2 (raw file):

// failure tolerance. This method will generate a ZoneConfig object representing
// the following attributes:
// num_voters = 5

nit: probably feels more natural for these to look like JSON? up to you.

pkg/sql/region_util.go, line 309 at r2 (raw file):

var multiRegionZoneConfigFields = []tree.Name{
	"num_replicas",
	"num_voters",

nit: any reason to change the sorted order? :P

pkg/sql/logictest/testdata/logic_test/multiregion, line 813 at r1 (raw file):

I would argue these belong to the unit tests instead.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/sql/region_util.go, line 97 at r2 (raw file):

does not make it clear it is for multi region setup

There aren't any other contexts where we're synthesizing a "zone config for a database" (are there?) and the first godoc comment makes it clear. IMO the previous naming was a bit too long and mutually inconsistent. The method name also doesn't need to include FromRegionConfig. The typed function parameter(s) are self-documenting in this regard.

pkg/sql/region_util.go, line 106 at r2 (raw file):

It's just the way these are printed when you do a SHOW ZONE CONFIGURATION FOR ... . They aren't JSON and we don't really print these out in JSON-like format anywhere AFAICT.

pkg/sql/region_util.go, line 309 at r2 (raw file):

It's the order in which SHOW ZONE CONFIGURATION FOR.. prints it to the CLI, except that I didn't realize global_reads is before everything else. Fixed now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/sql/region_util.go, line 97 at r2 (raw file):

i'm used to disappointment when i find a method takes in something i don't have because it's name leads me to believe it does something else ;) in this case i see someone wanting to make a zone config for database only to realise it is only for multiregion only, which the name originally meant to disambiguate.

probably more reason to put this in a separate package (multiregion package?), but we can punt on that for now.

pkg/sql/region_util.go, line 106 at r2 (raw file):

ah i thought those constraint things were JSON, no woz

pkg/sql/logictest/testdata/logic_test/multiregion, line 813 at r1 (raw file):

(These being in region_util_test.go)

, but Oliver should also sign off.

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @ajstorm, @andreimatei, and @otan)

pkg/sql/region_util.go, line 187 at r1 (raw file):

Looks great!

pkg/sql/region_util.go, line 168 at r3 (raw file):

}

func minorityQuorum(numVoters int32) int32 {

Add a note here that this requires numVoters to be odd. Or support even and odd voter counts with something like:

return ((numVoters + 1) / 2) - 1

It would also be nice to define in a comment what this function returns. This is leading me to realize that "minority quorum" might not be the right term, at least not in how it's defined elsewhere. What we really want the function to return is "the maximum number of tolerated failures, given a number of total voters" — the f in n=2f+1. So many maxToleratedFailures is a better name. What do you think?

pkg/sql/region_util.go, line 214 at r3 (raw file):

// of a multi-region database or the home region of a table in such a database.
//
// Under region failure tolerance, we will constrain exactly <quorum - 1> voting

nit: in a few of these comments, you describe the region failure tolerance behavior first, but then in all of your switch statements, you handle the zone failure tolerance first. Consider switching the comments around.

pkg/sql/logictest/testdata/logic_test/multiregion, line 813 at r1 (raw file):

That SGTM.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/sql/region_util.go, line 97 at r2 (raw file):

someone wanting to make a zone config for database only to realise it is only for multiregion

Makes sense, changed the names to indicate that they should only be used for multi-region objects. LMK if you dislike.

disappointment when i find a method takes in something i don't have

Well we'd run out of the 100 character limit pretty quickly with methods that take a lot of parameters :)
For instance, how would we rename a method like this?

Not to bike-shed too much on this, but I'll re-iterate that its the job of the function signature to describe what sort of parameters the function expects, and that's the thing to be looking at to avoid the disappointment.

probably more reason to put this in a separate package (multiregion package?), but we can punt on that for now.

Agreed, this'd make things cleaner.

pkg/sql/region_util.go, line 168 at r3 (raw file):

I like it, changed.

pkg/sql/region_util.go, line 214 at r3 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/multiregion, line 813 at r1 (raw file):

👍

Reviewed 2 of 2 files at r4, 2 of 2 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

+1 on exciting!

Left a few comments.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @andreimatei, @nvanbenschoten, and @otan)

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 58 at r5 (raw file):

                              num_voters = 3,
                              constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
                              voter_constraints = '[+region=ca-central-1]',

Should there be a :3 at the end here? According to the functional spec, this should be set to:

voter_constraints = {“+region=A”: num_voters}

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 59 at r5 (raw file):

                              constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
                              voter_constraints = '[+region=ca-central-1]',
                              lease_preferences = '[[+region=ca-central-1]]'

Nit: I know that you didn't modify this part, but looking at it, I'm wondering if the double [[]] is intentional. Thoughts?

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 257 at r5 (raw file):

              num_voters = 3,
              constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
              voter_constraints = '[+region=ap-southeast-2]',

There's a discrepancy here with the functional spec, which says that voter constraints and lease preferences are "Not specified at the table level for GLOBAL tables. Those tables will inherit these fields from the database’s PRIMARY REGION."

Is this difference intentional? If so, can we update the Function Specification?

BTW, if this was intentional, it will have some implications elsewhere namely, when we change the PRIMARY REGION of a database, we'll have to go through all of the GLOBAL tables and update their zone configs, something we don't do today.

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 344 at r5 (raw file):

                                           num_voters = 3,
                                           constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
                                           voter_constraints = '[+region=ap-southeast-2]',

Same comment as above. I was. under the impression that for REGIONAL BY TABLE tables, we'd inherit the zone configuration from the database. I know that you didn't change that aspect with this PR, but we put in these placeholders (which we knew weren't 100% accurate) on the hope that we'd clean them up when your changes were complete. Now that your changes are in, we should have the full discussion.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 429 at r5 (raw file):

                                                                   num_replicas = 5,
                                                                   num_voters = 5,
                                                                   constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',

Why are we showing the constraints here, when we don't show it above for SELECT create_statement FROM [SHOW CREATE TABLE regional_by_row_table]? Will this cause an issue with roundtriping?

pkg/sql/region_util.go, line 101 at r5 (raw file):

// is constrained to each region defined within the given `regionConfig` and
// some voting replicas are constrained to the primary region of the database
// depending on its prescribed failure tolerance setting.

Nit: To be more consistent here, can we say "survivability goal" instead of "failure tolerance setting"?

pkg/sql/region_util.go, line 104 at r5 (raw file):

//
// For instance, for a database with 4 regions: A (primary), B, C, D and region
// failure tolerance. This method will generate a ZoneConfig object representing

Nit: Can we use survivability here too?

pkg/sql/region_util.go, line 105 at r5 (raw file):

// For instance, for a database with 4 regions: A (primary), B, C, D and region
// failure tolerance. This method will generate a ZoneConfig object representing
// the following attributes:

I like this description a lot! What might really allow this to shine though, is a bit more description as to why we're setting each of these the way we are. I personally find these settings difficult to quickly understand, so the more commentary we have here, the better it will be for future readers of this code.

pkg/sql/region_util.go, line 140 at r5 (raw file):

// zoneConfigForMultiRegionPartition generates a ZoneConfig stub for a partition
// that belongs to a (regional by row) table in a multi-region database.

Super Nit: I'd remove "regional by row" from the parentheses. It's an important part of this sentence, and not something that can be easily skipped over without losing out on the meaning of the sentence. Your call though.

pkg/sql/region_util.go, line 181 at r5 (raw file):

) (numVoters, numReplicas int32) {
	const numVotersForZoneSurvival = 3
	// Under region failure tolerance, we use 5 voting replicas to allow for a

Nit: failure tolerance.

pkg/sql/region_util.go, line 186 at r5 (raw file):

	// allows for stable read/write latencies even under single node failures.
	//
	// TODO(aayush): Until we add allocator heuristics to coalesce voting replicas

Until this happens, what will be the behaviour? It would be good to call it out here in the comment just in case we don't get to the TODO in 21.1. Also, do we have an issue open for this TODO?

pkg/sql/region_util.go, line 195 at r5 (raw file):

	switch regionConfig.SurvivalGoal {
	case descpb.SurvivalGoal_ZONE_FAILURE:
		// <numVoters in the region with affinity> + <1 replica for every other

Nit: if we're going to standardize around using "home", we should use it in the comment above. "numVoters in the home region".

pkg/sql/region_util.go, line 198 at r5 (raw file):

		// region>
		numVoters = numVotersForZoneSurvival
		numReplicas = (numVotersForZoneSurvival) + (numRegions - 1)

Nit: We might benefit from a comment here on why we're setting numReplicas this way. It might just be me, but it took me a few minutes to deduce why we're not using maxToleratedFailures in this calculation. To that point, this might also have been made clearer if we had a name for maxToleratedFailures which described it's relation to replicas. Perhaps something like maxConcurrentReplicaFailuresBeforeRangeOutage. It's long, but at it clearly describes what it's returning. If we called it that, we could add a short comment below which would make the REGION FAILURE placement much more intuitive:

// We place the maximum concurrent replicas that can fail before a range outage in the home region, and ensure that there's at least one replica in all other regions.

pkg/sql/region_util.go, line 200 at r5 (raw file):

		numReplicas = (numVotersForZoneSurvival) + (numRegions - 1)
	case descpb.SurvivalGoal_REGION_FAILURE:
		// <(quorum - 1) voters in the region with affinity> + <1 replica for every

Nit: home

pkg/sql/region_util.go, line 221 at r5 (raw file):

//
// Under region failure tolerance, we will constrain exactly <quorum - 1> voting
// replicas in the primary/home region.

This is a great comment! As mentioned above, some of this may also be helpful to be sprinkled around the other functions so that people don't need to understand this function whenever they investigate problems elsewhere in this code.

pkg/sql/region_util.go, line 231 at r5 (raw file):

			{
				// We don't specify `NumReplicas` here to indicate that we want _all_
				// voting replicas to be constrained to this one region.

Another great comment!

Nit: We might want to say here that by not specifying NumReplicas here, we inherit the value at the database level (assuming that's the case).

pkg/sql/region_util.go, line 331 at r5 (raw file):

//
// Relevant multi-region configured fields (as defined in
// `multiRegionZoneConfigFields`) will be overwritten by the calling function

Nit: Might want to leave a TODO here (for me) that we're only planning on overwriting these if they've been set by the user, if sql_safe_updates = false.

pkg/sql/region_util.go, line 339 at r5 (raw file):

	// We only care about NumVoters here at the table level. NumReplicas is set at
	// the database level, not at the table/partition level.
	numVoters, _ := getNumVotersAndNumReplicas(regionConfig)

If the user has set num_replicas manually, do we want to overwrite it here? I would think we might have to, to avoid an inconsistency.

pkg/sql/logictest/testdata/logic_test/multiregion, line 636 at r5 (raw file):

                gc.ttlseconds = 90000,
                global_reads = true,
                num_replicas = 5,

In addition to my comment above about having to modify the zone configurations of all global tables on changing the PRIMARY REGION, we'll also have to modify them all on ADD REGION. I'm not following how this got set though, as the table level functions aren't setting the num_replicas. Is this just displaying inherited fields from the database? If so, this is another issue we'll have to address for round-tripping.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 58 at r5 (raw file):

If we state the constraint as is, without the number of replicas, it applies to all replicas. See: https://www.cockroachlabs.com/docs/v20.2/configure-replication-zones#scope-of-constraints

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 59 at r5 (raw file):

Yup, it's the unfortunate syntax for lease_preferences. See lease_preferences in the table here: https://www.cockroachlabs.com/docs/v20.2/configure-replication-zones

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 257 at r5 (raw file):

It's not a global table at this point in the test. See the line above changing it to a REGIONAL BY TABLE table. For posterity's sake, I changed the table name a bit so its less confusing.

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 344 at r5 (raw file):

REGIONAL BY TABLE tables were always intended to define their own voter_constraints and lease_preferences iff they were homed in a different region than the primary region. Did you mean to say GLOBAL in your comment here? Cause global tables are inheriting a lot of fiends from the database's zone config (except, ofcourse, the global_reads attribute).

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 429 at r5 (raw file):

Well these two statements are referring to different things, right? SHOW CREATE TABLE is the statement that created the table, SHOW ZONE CONFIGURATION FOR... is the statement that shows you the zone config for the object. The former round-trips. The latter isn't really supposed to round-trip as it also shows you the set of fields that are inherited (rather than the ones that are just set at this object's level).

pkg/sql/region_util.go, line 101 at r5 (raw file):

Done.

pkg/sql/region_util.go, line 104 at r5 (raw file):

Done.

pkg/sql/region_util.go, line 105 at r5 (raw file):

I added a bit of commentary next to the part that constrains 1 replica per region and a sentence directing readers to synthesizeVoterConstraints() for the voter_constraints bit.

pkg/sql/region_util.go, line 140 at r5 (raw file):

Done.

pkg/sql/region_util.go, line 181 at r5 (raw file):

Changed

pkg/sql/region_util.go, line 186 at r5 (raw file):

Added a blurb talking about the current behavior. Created #59650 to track.

pkg/sql/region_util.go, line 195 at r5 (raw file):

Done.

pkg/sql/region_util.go, line 198 at r5 (raw file):

We place the maximum concurrent replicas that can fail before a range outage in the home region, and ensure that there's at least one replica in all other regions.

Added.

Changed the name to maxFailuresBeforeUnavailability. LMK if you dislike.

The very first point I'm not sure about. I added a pointer to synthesizeVoterConstraints() where this is explained properly.

pkg/sql/region_util.go, line 200 at r5 (raw file):

Done.

pkg/sql/region_util.go, line 221 at r5 (raw file):

Would you mind clarifying where else you'd like this to be added?

pkg/sql/region_util.go, line 231 at r5 (raw file):

The NumReplicas attribute in a ConstraintsConjunction defines "the number of replicas that the constraint must be satisfied by". This is different from the NumReplicas attribute of a ZoneConfig. The latter is the one we're inheriting or not. For this one:

// We don't specify `NumReplicas` here to indicate that we want _all
// voting replicas to be constrained to this one region.

pkg/sql/region_util.go, line 331 at r5 (raw file):

They'll always be over-written by this logic, regardless of what sql_safe_updates is set to. The sql_safe_updates discussion was about letting the users override these values. Oliver has a TODO for this here:

pkg/sql/region_util.go, line 339 at r5 (raw file):

Yes, num_replicas is a function of num_voters, the survivability setting and the number of regions. So we must override whatever the user has set.

pkg/sql/logictest/testdata/logic_test/multiregion, line 636 at r5 (raw file):

In addition to my comment above about having to modify the zone configurations of all global tables on changing the PRIMARY REGION

We wouldn't have to do anything like this, since the global tables inherit that field from the database level zone config, which also means we wouldn't have to modify any of them upon ADD REGION. Every field here, except for global_reads, is inherited from the database level zone config. Does that answer your question perhaps?

If so, this is another issue we'll have to address for round-tripping.

I'm not sure I understand. Would you mind clarifying?

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 59 at r5 (raw file):

I'll have to take your word for it. That link doesn't seem to have an example where there's only one region in teh lease_preferences.

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 257 at r5 (raw file):

Ah, I didn't look high enough up. Looks good.

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 344 at r5 (raw file):

Agreed, this looks good. I was mistaken with my comment. I meant REGIONAL BY TABLE, but forgot that those not in the PRIMARY REGION would need their own zone config.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 429 at r5 (raw file):

Interesting, TIL. I was under the impression that both of them were intended to round-trip.

pkg/sql/region_util.go, line 105 at r5 (raw file):

Great, thanks!

pkg/sql/region_util.go, line 186 at r5 (raw file):

Looks good.

pkg/sql/region_util.go, line 198 at r5 (raw file):

Looks good. On the pointer fixes my first part.

pkg/sql/region_util.go, line 221 at r5 (raw file):

I think we're good now. The sprinkling you did above helps a lot.

pkg/sql/region_util.go, line 231 at r5 (raw file):

Ah, my mistake. I follow it now.

pkg/sql/region_util.go, line 331 at r5 (raw file):

The issue was supposed to go both ways. We also want to prevent overwriting in the system if the user has overridden at any point in the past (unless sql_safe_updates is true).

pkg/sql/region_util.go, line 339 at r5 (raw file):

Is that override of num_replicas going to happen here? We only seem to be setting num_voters (unless I'm missing something).

pkg/sql/logictest/testdata/logic_test/multiregion, line 636 at r5 (raw file):

All good, stems from my confusion above.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 59 at r5 (raw file):

The thing to note is that lease_preferences lets you specify an ordered list of preferences, each of those individual preferences is a Constraint which is enclosed in those inner [ ]. It looks weird in our case because we only have one lease preference.

Is it hard-to-read / unergonomic? Absolutely.

pkg/sql/region_util.go, line 331 at r5 (raw file):

This is yours/@otan's call and outside the scope of this PR, but I would think that we wouldn't want to do that. Particularly not based off of sql_safe_updates as that's a pretty coarse setting which could be enabled for various other reasons (like a completely unrelated DROP DATABASE CASCADE).

Additionally, imo not overwriting the zone configs when these multi-region DDL statements are issued partly defeats the purpose of adding/removing regions. My understanding was that the reason we need to guard this behind something like sql_safe_updates is because our statements can over-write the user's changes and they need to "sign off" on that, but I may not have kept up-to-date on the latest discussions on this subject.

What do you think?

pkg/sql/region_util.go, line 339 at r5 (raw file):

Nope, that's at the database level zone config, see zoneConfigForMultiRegionDatabase.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @ajstorm, @andreimatei, @nvanbenschoten, and @otan)

pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality, line 59 at r5 (raw file):

Yeah, what was throwing me off was that when you go down to one constraint, the [ ] is still required. I'd think that it would be optional at that point.

pkg/sql/region_util.go, line 331 at r5 (raw file):

Sorry, I might not have been clear above. There are two things we want to guard against with sql_safe_updates:

1. A user updates a zone configuration and a multi-region DDL statement will overwrite that change.
2. A multi-region zone configuration is in affect and a user wants to update some part of that zone configuration.

In the second case, the user is potentially going to mess up the multi-region configuration, so we want to let them know about that. In the first case, we're potentially going to overwrite something that is important to the user, so we want to at least let them know about it before we do something bad (from their perspective).

Important to note is that in the first case, the plan isn't to not overwrite the zone config, but instead, fail the DDL statement with an appropriate error message. I completely agree with you that we don't want an invalid zone configuration in place after we've completed a multi-region DDL statement.