Reviewed 2 of 16 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @otan, and @pawalt)

pkg/ccl/logictestccl/testdata/logic_test/domiciling, line 2 at r2 (raw file):

# LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off

nit: rename this file to placement

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_domiciling, line 1 at r2 (raw file):

# LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off

nit: rename this file regional_by_row_placement_restricted

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_domiciling, line 43 at r2 (raw file):

# Alter to RESTRICTED and see that we have no non-voter constraints.
statement ok
ALTER DATABASE testdb SET PLACEMENT RESTRICTED

Add tests to add/drop regions and change survival goal to ensure these operations work correctly with PLACEMENT RESTRICTED.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_domiciling, line 1 at r2 (raw file):

# LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off

Same as above, let's rename this file regional_by_table_placement_restricted

Separately, should we have some tests for global_tables with placement_restricted as well?

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_domiciling, line 45 at r2 (raw file):

# Change primary region to ensure zone config is rebuilt on table changes.
statement ok
ALTER DATABASE testdb SET PRIMARY REGION "ap-southeast-2"

Let's add tests that ensure ADD/DROP region continue to work correctly with PLACEMENT RESTRICTED as well?

pkg/sql/alter_database.go, line 1108 at r2 (raw file):

	}

	// TODO(pawalt): add a check for placement enabled cluster setting

Mind opening an issue for this to keep track?

pkg/sql/alter_database.go, line 1157 at r2 (raw file):

		return err
	}
	n.desc.RegionConfig.Placement = newPlacement

Instead of directly updating the proto like such, let's add a setter method on dbdesc.Mutable instead. In general, we try to avoid raw proto accesses/setters -- they should instead be mediated through the specific descriptor interface or Mutable type respectively.

pkg/sql/alter_database.go, line 1172 at r2 (raw file):

	}

	if err := multiregion.ValidateRegionConfig(regionConfig); err != nil {

SynthesizeRegionConfig gives you a validated RegionConfig, so I think you can get rid of this.

pkg/sql/alter_database.go, line 1194 at r2 (raw file):

	}

	// TODO(pawalt): add telemetry/logging for PLACEMENT events.

Can we open issues for both of these things?

pkg/sql/crdb_internal.go, line 253 at r2 (raw file):

	regions STRING[],
	survival_goal STRING,
	data_placement STRING,

nit: s/data_placement/placement_policy/?

Nicely done!

Reviewed 10 of 16 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @pawalt)

-- commits, line 8 at r2 ([raw file](https://github.com/cockroachdb/cockroach/blob/204929de4c4b49a7057f7d9dcf87cb3f2beb74d9/-- commits#L8)):
I know that we don't want to make this public now, but in the case we need to expose this on short notice, does it make sense to write some basic documentation now? @rmloveland thoughts?

pkg/ccl/logictestccl/testdata/logic_test/domiciling, line 10 at r2 (raw file):

statement ok
CREATE DATABASE zone_survivable_strict PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2", "us-east-1" PLACEMENT RESTRICTED

Can we both show the zone configurations here, and validate them using the builtin? I want to ensure that validate holds up even with PLACEMENT RESTRICTED set.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_domiciling, line 43 at r2 (raw file):

# Alter to RESTRICTED and see that we have no non-voter constraints.
statement ok
ALTER DATABASE testdb SET PLACEMENT RESTRICTED

Can we validate with the builtin here too?

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_domiciling, line 27 at r2 (raw file):

# Alter to RESTRICTED and see that we have no non-voter constraints.
statement ok
ALTER DATABASE testdb SET PLACEMENT RESTRICTED

Validate here too?

pkg/sql/alter_database.go, line 755 at r2 (raw file):

	// must be explicitly rebuilt so as to move their primary region.
	if updatedRegionConfig.IsPlacementRestricted() {
		if err := params.p.updateZoneConfigsForAllTables(params.ctx, n.desc); err != nil {

Does it make sense to be more precise here? What I'm worried about is cases where we have 100s or 1000s of tables and only a handful of GLOBAL tables. This will end up rewriting all of their zone configs. Should we instead create a updateZoneConfigsForAllGlobalTables which filters out the other table types?

pkg/sql/alter_database.go, line 1128 at r2 (raw file):

		return errors.WithHintf(
			pgerror.New(pgcode.InvalidName,
				"database must have associated regions before a survival goal can be set",

Copy pasta? Shouldn't this read that the database must have associated regions before the placement goal can be set? Comment above too.

Also, can we test this case?

pkg/sql/crdb_internal.go, line 294 at r2 (raw file):

						// unless we know the user wants to use PLACEMENT. Therefore, we
						// only add a PLACEMENT clause if we're in restricted mode.
						createNode.Placement = tree.DataPlacementUnspecified

I'm probably missing something, but is there a reason why we want to differentiate between DEFAULT and UNSPECIFIED? Shouldn't they be the same in practice?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @otan, and @pawalt)

-- commits, line 8 at r2 ([raw file](https://github.com/cockroachdb/cockroach/blob/204929de4c4b49a7057f7d9dcf87cb3f2beb74d9/-- commits#L8)):

we need to expose this on short notice

tl;dr: I'd rather just ignore this for now

long version:

if someone really really needs this info ASAP I bet someone could email them instructions how to use it while we get a docs PR ready (est. 1-2 days to write/review/merge)

meta: this convo is part of why I'm knee-jerk anti "put it in the product but don't document it", because then we have to spend time thinking about non-standard workflows like this and maybe end up doing rushed work anyway (cc @awoods187 with my sudden but inevitable complaint :-D )

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @otan, and @pawalt)

-- commits, line 8 at r2 ([raw file](https://github.com/cockroachdb/cockroach/blob/204929de4c4b49a7057f7d9dcf87cb3f2beb74d9/-- commits#L8)):

It's a fair (and somewhat expected) complaint. The reason we voted to have this go into 21.2 is because we're worried that the knee-jerk code-level backfit would be riskier than a knee-jerk doc change, but I'm open to have my mind changed on that. I confess that I don't have a good sense for the risk involved in these types of doc changes.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @otan, and @pawalt)

-- commits, line 8 at r2 ([raw file](https://github.com/cockroachdb/cockroach/blob/204929de4c4b49a7057f7d9dcf87cb3f2beb74d9/-- commits#L8)):

the risk involved in these types of doc changes.

tbh almost no risk from a purely docs POV, it's easy to add when we need it. I agree with the assessment that changing the code is way riskier/harder

I'm just, like, duty bound to complain about it - because historically CRL doesn't do much of this. But I have worked at other places where this becomes "the norm", esp. when dealing with big customers. Work becomes a lot less fun (and harder to do at good quality levels) in those environments. So I'm worried about the slippery slope. Because from any customer's (or revenue team member's) POV it's always "Important (tm)"

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @otan, and @pawalt)

-- commits, line 8 at r2 ([raw file](https://github.com/cockroachdb/cockroach/blob/204929de4c4b49a7057f7d9dcf87cb3f2beb74d9/-- commits#L8)):

I agree. This is one of the reasons I was wondering if we wanted a draft PR (or Google Doc) at the ready should such a need arise? I was thinking that this could make the fire drill less hot if/when it eventually arrives.

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

pkg/ccl/logictestccl/testdata/logic_test/domiciling, line 10 at r2 (raw file):

Yup I'll do it at the database level here and at the table level in each table test.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_domiciling, line 43 at r2 (raw file):

Done.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_domiciling, line 43 at r2 (raw file):

Done.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_domiciling, line 1 at r2 (raw file):

Yep I'll add tests that ensure global tables get the new non-inherited zone config.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_domiciling, line 27 at r2 (raw file):

Done.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_domiciling, line 45 at r2 (raw file):

Done.

pkg/sql/alter_database.go, line 755 at r2 (raw file):

I think that makes sense. I figure we'll want to do this with RBR/RBT tables as well, so I've added a method that allows you to pass in a filter to only apply changes to table descriptors that match the filter.

pkg/sql/alter_database.go, line 1108 at r2 (raw file):

Done.

pkg/sql/alter_database.go, line 1128 at r2 (raw file):

Added testing for creating without a primary region and altering without a primary region.

pkg/sql/alter_database.go, line 1157 at r2 (raw file):

Done.

pkg/sql/alter_database.go, line 1172 at r2 (raw file):

Done.

pkg/sql/alter_database.go, line 1194 at r2 (raw file):

#68597
#68598

pkg/sql/crdb_internal.go, line 294 at r2 (raw file):

They are the same in practice, but the formatter doesn't print for UNSPECIFIED but it does for DEFAULT. In this case, we want to always format as UNSPECIFIED so that a user on the DEFAULT behavior doesn't get PLACEMENT leaked to them.

Reviewed 3 of 10 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, @awoods187, @otan, and @pawalt)

pkg/ccl/logictestccl/testdata/logic_test/global_placement_restricted, line 100 at r3 (raw file):

            lease_preferences = '[[+region=ap-southeast-2]]'

# Alter to DEFAULT and see that our voter constraints are back.

Were they gone before?

pkg/sql/alter_database.go, line 265 at r3 (raw file):

	// RESTRICTED mode, GLOBAL tables will not inherit the database zone config
	// and will have to be explicitly rebuilt.
	if n.desc.RegionConfig.Placement == descpb.DataPlacement_RESTRICTED {

Same comment as below.

pkg/sql/alter_database.go, line 652 at r3 (raw file):

	// Update all GLOBAL tables' zone configurations. This is required as in
	// RESTRICTED mode, GLOBAL tables will not inherit the database zone config
	// and will have to be explicitly rebuilt.

I see you're doing something similar in the DatabaseRegionChangeFinalizer. I may be missing something, but do you need both?

pkg/sql/alter_database.go, line 1220 at r3 (raw file):

	}

	// Update all GLOBAL tables' zone configurations. This is required as in

I think there's more to this. I'd reword this as:

Update all GLOBAL tables' zone configurations. This is required because GLOBAL table's can inherit the database's zone configuration under the DEFAULT placement policy. However, under the RESTRICTED placement policy, non-voters are removed from the database. But global tables need non-voters to behave properly, and as such, need a bespoke zone configuration. Regardless of the transition direction (DEFAULT -> RESTRICTED, RESTRICTED -> DEFAULT), we need to refresh the zone configuration of all GLOBAL table's inside the database to either carry a bespoke configuration or go back to inheriting it from the database.

pkg/sql/crdb_internal.go, line 294 at r2 (raw file):

nit: s/if we're in restricted mode/if the database was configured with restricted placement/g

pkg/sql/create_database.go, line 98 at r3 (raw file):

	if n.Placement != tree.DataPlacementUnspecified {
		// TODO(pawalt): add cluster setting checking for placement

nit: full stop
nit: link the issue here (and above) as well maybe?

pkg/sql/database_region_change_finalizer.go, line 39 at r3 (raw file):

	cleanupFunc         func()
	regionalByRowTables []*tabledesc.Mutable
	globalTables        []*tabledesc.Mutable

Does this need to be a field on this struct? Put another way, is there a reason to pre-fetch all global tables?

pkg/sql/database_region_change_finalizer.go, line 173 at r3 (raw file):

	if err != nil {
		return err
	}

Should we add a check to only do this if the database is configured in PLACEMENT RESTRICTED?

pkg/sql/database_region_change_finalizer.go, line 175 at r3 (raw file):

	}

	for _, tableDesc := range r.globalTables {

Do you need to save all global tables on this struct or can you just fetch them here?

pkg/sql/descriptor.go, line 250 at r3 (raw file):

		return descpb.DataPlacement_RESTRICTED, nil
	default:
		return 0, errors.Newf("unknown data placement: %d", g)

Should this be an assertion failed error?

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

		// placement. Voter placement will be set at the table/partition level to
		// the table/partition region.
		constraints = nil

Why did this change?

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

}

func filterIsLocalityGlobal(t *tabledesc.Mutable) bool {

Consider making this an option. Have updateZoneConfigsForTables take in an optional list of options. By default, the filter function will be a passthrough. Then, you can make a WithOnlyGlobalTables option that applies this filter.

Here's a link in the style guide which talks a bit more about this pattern (if you're unfamiliar): https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/181371303/Go+Golang+coding+guidelines#Functional-Options

pkg/sql/catalog/dbdesc/database_desc.go, line 408 at r3 (raw file):

}

// SetPlacement sets the placement of the region config for a database

s/of the region config/on the region config/g

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_placement_restricted, line 134 at r3 (raw file):

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

# Alter to DEFAULT and see that our voter constraints are back.

Do you mean constraints?

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_placement_restricted, line 10 at r3 (raw file):

statement ok
CREATE TABLE test () LOCALITY REGIONAL BY TABLE IN PRIMARY REGION

Let's add the same set of tests for a REGIONAL BY TABLE in an explicit region as well? Say ap-southeast-2, as it isn't the primary region and you're not dropping it anywhere?

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_placement_restricted, line 97 at r3 (raw file):

                 lease_preferences = '[[+region=ap-southeast-2]]'

# Alter to DEFAULT and see that our voter constraints are back.

Do you mean constraints?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, @awoods187, @otan, and @pawalt)

pkg/sql/alter_database.go, line 755 at r2 (raw file):

Cool!

pkg/sql/crdb_internal.go, line 294 at r2 (raw file):

Gotcha. I think I saw this in your Gru presentation.

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

pkg/ccl/logictestccl/testdata/logic_test/global_placement_restricted, line 100 at r3 (raw file):

Nope ... not sure what I was trying to say here. Fixed.

pkg/sql/alter_database.go, line 265 at r3 (raw file):

Done.

pkg/sql/alter_database.go, line 652 at r3 (raw file):

Ah I didn't think of that. Removed.

pkg/sql/alter_database.go, line 1220 at r3 (raw file):

Done.

pkg/sql/database_region_change_finalizer.go, line 39 at r3 (raw file):

Nope I think fetching them later is fine. Changed.

pkg/sql/database_region_change_finalizer.go, line 173 at r3 (raw file):

Done.

pkg/sql/database_region_change_finalizer.go, line 175 at r3 (raw file):

Done.

pkg/sql/descriptor.go, line 250 at r3 (raw file):

Yep

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

I found out that empty constraints get deserialized as nil even if they're created with [], so I changed this so that the generated zone config is the same as the one stored in the database. You get errors otherwise since the multi-region checks think the zone config has been modified by the user.

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

Gave this a shot. Let me know if it looks ok

pkg/sql/catalog/dbdesc/database_desc.go, line 408 at r3 (raw file):

Done.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_placement_restricted, line 134 at r3 (raw file):

Yep

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_placement_restricted, line 10 at r3 (raw file):

Done.

pkg/ccl/logictestccl/testdata/logic_test/regional_by_table_placement_restricted, line 97 at r3 (raw file):

Yep thanks

Thanks for making the changes! This is quite close, mostly minor comments left.

Let's open an issue for the ADD/DROP REGION failure case that we talked about offline as well?

Reviewed 1 of 10 files at r3, 1 of 10 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, @awoods187, @otan, and @pawalt)

pkg/sql/database_region_change_finalizer.go, line 81 at r4 (raw file):

			dbDesc,
			func(ctx context.Context, scName string, tableDesc *tabledesc.Mutable) error {
				if tableDesc.Dropped() {

Revert this change?

pkg/sql/database_region_change_finalizer.go, line 160 at r4 (raw file):

// updateGlobalTablesZoneConfig recalculates all global tables' zone configs so
// that their zone configs are recalculated after a newly-added region goes
// out of being a transitioning region.

Could you add a nod here that this is only required when the database has the PLACEMENT RESTRICTED policy and why?

Also can we s/recalculate/refresh/g in the comment for consistency, as that's the word we use in other places?

pkg/sql/database_region_change_finalizer.go, line 181 at r4 (raw file):

		return err
	}
	mutableDesc := dbDesc.(*dbdesc.Mutable)

I don't think this is safe -- you shouldn't be getting an immutable database descriptor and then casting it to mutable descriptor. You should instead ask the descsCol for a mutable database descriptor.

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

I see, thanks for the explanation. Let's put this in a // NB: here as well for posterity?

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

Looks great, thanks for making the change! Is this filter function unused now? Should we get rid of it?

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_placement_restricted, line 134 at r4 (raw file):

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

# Alter to DEFAULT and see that our non-voter constraints are back.

Let's just drop non-voter altogether? It's slightly confusing, the way this looks, but the constraints field actually applies to both voters and non-voters.

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

pkg/sql/database_region_change_finalizer.go, line 81 at r4 (raw file):

Done.

pkg/sql/database_region_change_finalizer.go, line 160 at r4 (raw file):

Done.

pkg/sql/database_region_change_finalizer.go, line 181 at r4 (raw file):

I did this initially because I didn't want to have to get the database name and then the mutable descriptor (since we only have a GetMutableDatabaseByName function). I've added a GetMutableDatabaseByID function; let me know if the approach is safe. I think it is because I'm requiring the descriptor to be mutable, but not 100%.

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

Done.

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

Yep removed

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_placement_restricted, line 134 at r4 (raw file):

Done.

One last thing before this is good to go! My bad on not realizing about updateZoneConfigsForAllTables earlier.

Reviewed 1 of 10 files at r4, 1 of 4 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, @awoods187, @otan, and @pawalt)

pkg/sql/database_region_change_finalizer.go, line 181 at r4 (raw file):

I think what you have makes sense, but I'm not sure we need to/should expose this API considering:

1. this is the only usage
2. The reason you need it here is slightly dubious (not because of something you did)

Let me say a bit more about what I mean by point 2 here -- I'm looking at this udpateZoneConfigsForTables method again and looks like it only needs the database descriptor to synthesize the region config. I don't think it needs a mutable descriptor to begin with considering all its using it for is to synthesize the region config. SynthesizeRegionConfig only takes an ID and does so because it wants to read the descriptor from the store so that it gets an up-to-date view of things (as opposed to the leased version). This is all to say that maybe the right thing to do here is to change updateZoneConfigsForTables to take in a catalog.DatabaseDescriptor instead of a mutable database descriptor.

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

		// always deserialized as nil. Therefore, if constraints are set as [] here,
		// the database will have a difference in its expected constraints vs the
		// actual constraints.

nit: vs the actual constraints when comparing using multi-region validation builitins?

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

pkg/sql/database_region_change_finalizer.go, line 181 at r4 (raw file):

Agreed. Changed to catalog.DatabaseDescriptor and removed GetMutableDatabaseByID

Reviewed 3 of 3 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, @awoods187, and @otan)