sql: prevent REGIONAL BY ROW and ADD/DROP REGION concurrent transitions

[otan]

see individual commits for details

resolves #63368
resolves #63462
resolves #64011
resolves #63974

[cockroach-teamcity]

This change is 

[arulajmani]

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

---

pkg/ccl/multiregionccl/region_test.go, line 255 at r2 (raw file):

		expectedIndexes []string
	}{
		{

Looks like this is only testing a CREATE REGIONAL BY ROW table now, so we can probably simplify this test a bit?

---

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

	// forEachTableDesc touches all the table keys, which prevents a race
	// with ADD/REGION committing at the same time as the user transaction.
	return forEachTableDesc(

forEachTableDesc is no good here, as it filters out descriptors the user doesn't have privileges on. You probably want forEachMutableTableInDatabase here instead.

[otan]

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

---

pkg/ccl/multiregionccl/region_test.go, line 255 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Looks like this is only testing a CREATE REGIONAL BY ROW table now, so we can probably simplify this test a bit?

i'd rather keep it, since it's easier to add cases if we find out we need them at a later point.
also, don't think the simplification makes it that much easier to read :P

---

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

Previously, arulajmani (Arul Ajmani) wrote…

forEachTableDesc is no good here, as it filters out descriptors the user doesn't have privileges on. You probably want forEachMutableTableInDatabase here instead.

nice cathc, done

[arulajmani]

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

---

pkg/ccl/multiregionccl/region_test.go, line 255 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

i'd rather keep it, since it's easier to add cases if we find out we need them at a later point.
also, don't think the simplification makes it that much easier to read :P

You have hope we'll need this at a later point after last week? haha, I'm fine keeping the structure as is, just update the comment above the test which explains why there's only one op so that readers aren't confused?

---

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

// database are currently being modified.
func (p *planner) checkNoRegionChangeUnderway(
	ctx context.Context, dbID descpb.ID, op string,

nit: consider wrap the error at the call-site instead of passing in this op string?

[ajstorm]

minus the nits. 🎉

Reviewed 12 of 12 files at r3, 4 of 4 files at r4, 8 of 8 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @otan)

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 708 at r4 (raw file):

	regionalByRowChanges := []struct {
		setup                          string
		alterCmd                       string

Nit: We might want to generalize the name of this, since some of the below operations aren't "ALTER"s.

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 778 at r4 (raw file):

	for _, rbrChange := range regionalByRowChanges {
		for _, regionChange := range regionChanges {
			t.Run(fmt.Sprintf("from %s to %s with racing %s", rbrChange.setup, rbrChange.alterCmd, regionChange.cmd), func(t *testing.T) {

Nit: Should this be "statement %s with concurrent %s, racing with %s"?

---

pkg/ccl/multiregionccl/regional_by_row_test.go, line 804 at r4 (raw file):

				setupDB(sqlDB, rbrChange.setup)

				// Perform the alter table command asynchronously; this will be interrupted.

Nit: generalize alter table command?

---

pkg/sql/region_util.go, line 1710 at r4 (raw file):

						detailSuffix,
					),
					"cancel the existing job or try again when the change is complete",

Nit: Any chance we can pull the job id for easier reference?

---

pkg/sql/region_util.go, line 1735 at r4 (raw file):

			}
			// Disallow index changes for REGIONAL BY ROW tables.
			// We do this on the second loop, as index changes may be a precede to the actual PRIMARY KEY swap.

Nit: I think we can drop "be a" and "to". What do you mean by "precede" here? Might be good to provide a bit more context in this comment.

[ajstorm]

@otan just a heads-up that this is one of the last GA blockers needed for the RC that they're hoping to cut Tuesday (EDT). Any chance we can get this in sometime before then?

[otan]

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

---

pkg/sql/region_util.go, line 1710 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Any chance we can pull the job id for easier reference?

that is not straightforward, so i'm not doing that.
there's a bunch of TODOs all over the place to do this for other things.

---

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

Previously, arulajmani (Arul Ajmani) wrote…

nit: consider wrap the error at the call-site instead of passing in this op string?

i'd rather not, because that pushes a lot of lines of code and setup up at the call sites.

[otan]

bors r=ajstorm,arulajmani

[craig]

Build succeeded:

• GitHub CI (Cockroach)