multi-region: Unblock IMPORT into allowable multi-region tables

[ajstorm]

Unblock IMPORTing into allowable multi-region tables. We only support
importing into GLOBAL and REGIONAL BY TABLE tables as #61133 is blocking
IMPORT into any tables which have columns generated using a user defined
type (which covers REGIONAL BY ROW tables, as they have the crdb_region
column which is generated using the crdb_internal_region type).

This commit includes tests for both IMPORT and IMPORT INTO, as well as
cases which illustrate #61133 for non-multi-region tables.

Of note is the change to remove validation in create_table.go . This change was
required as the validation at this point is not possible from within
IMPORT, and it was uncovered through testing that this validation when
generating the LocalityConfig was not required anyway, as we validate
before writing the descriptors in all cases (i.e. after the
LocalityConfig is generated, but before the descriptors are written to
disk).

Release note: None

Release justification: Fixes bug in interaction between existing functionality and new multi-region feature.

[cockroach-teamcity]

This change is 

[ajstorm]

Hold off on reviewing for now, diagnosing test failures....

[otan]

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

---

pkg/ccl/importccl/import_stmt_test.go, line 34 at r1 (raw file):

	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl"
	_ "github.com/cockroachdb/cockroach/pkg/ccl/kvccl"
	_ "github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl"

hmm, correcting myself, i think _ "github.com/cockroachdb/cockroach/pkg/ccl" should do it.

---

pkg/sql/create_table.go, line 369 at r1 (raw file):

	dg := catalogkv.NewOneLevelUncachedDescGetter(params.p.txn, params.ExecCfg().Codec)
	if desc.LocalityConfig != nil {
		if err := desc.ValidateTableLocalityConfig(params.ctx, dg); err != nil {

are you sure we want to move this to startExec? this means other places which call NewTableDesc doesn't get validation

[ajstorm]

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

---

pkg/ccl/importccl/import_stmt_test.go, line 34 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

hmm, correcting myself, i think _ "github.com/cockroachdb/cockroach/pkg/ccl" should do it.

No dice. Since we're in importccl, importing the the whole ccl directory causes a circular import.

---

pkg/sql/create_table.go, line 369 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

are you sure we want to move this to startExec? this means other places which call NewTableDesc doesn't get validation

Have a look at the update commit record and let me know if this covers your concerns.

[ajstorm]

All tests are passing now. @adityamaru, @arulajmani, ready for a look now (and another look from @otan).

[pbardea]

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

---

pkg/ccl/backupccl/restore_job.go, line 388 at r2 (raw file):

				if table.GetLocalityConfig() != nil {
					return pgerror.Newf(pgcode.FeatureNotSupported,
						"cannot write descriptor for multi-region table %d into non-multi-region database %d",

Let's add the table & database names in addition to the table ID in this error message? It makes the message a lot easier to grok for users.

[ajstorm]

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

---

pkg/ccl/backupccl/restore_job.go, line 388 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Let's add the table & database names in addition to the table ID in this error message? It makes the message a lot easier to grok for users.

Done.

[otan]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajstorm, @arulajmani, and @pbardea)

---

pkg/ccl/importccl/import_stmt_test.go, line 34 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

No dice. Since we're in importccl, importing the the whole ccl directory causes a circular import.

ah it'd work if you renamed the test package to importccl_test, but no biggie

[arulajmani]

Mostly looks good, just a couple minor suggestions and nits

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

---

pkg/ccl/backupccl/restore_job.go, line 380 at r3 (raw file):

			}
			if dbDesc.GetRegionConfig() != nil {
				if table.GetLocalityConfig() == nil {

nit: unnecessary nesting, you could revert this diff

---

pkg/ccl/importccl/import_stmt_test.go, line 1219 at r3 (raw file):

		verifyQuery string
		expected    [][]string
		err         bool

nit: non-empty error string <-> expect an error

---

pkg/ccl/importccl/import_stmt_test.go, line 6439 at r3 (raw file):

	const (
		nodes = 3

nit: unnecessary braces, you could just: const nodes = 3

---

pkg/ccl/importccl/import_stmt_test.go, line 6476 at r3 (raw file):

	}

	tc := testcluster.StartTestCluster(t, nodes, clusterArgs)

I'd recommend modifying TestingCreateMultiRegionCluster to accept a baseDir so that you don't have to do this whole setup thing here.

---

pkg/ccl/importccl/import_stmt_test.go, line 6490 at r3 (raw file):

	// Table schemas for USING
	tableSchemaMR := fmt.Sprintf("nodelocal://0/%s", "simple-schema-multi-region.sql")

Is it worth adding one for GLOBAL tables here as well?

---

pkg/ccl/importccl/import_stmt_test.go, line 6559 at r3 (raw file):

			if test.err {
				if test.errString == "" {

nit: instead of having a err and an errString, you could make the empty string imply no error.

[pbardea]

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

[ajstorm]

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

---

pkg/ccl/backupccl/restore_job.go, line 380 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: unnecessary nesting, you could revert this diff

The reason I wrote it this way was because I wanted to avoid an else if branch below, which we would need to confirm that the RegionConfig was nil. I guess I could reorder it to avoid the nesting.

---

pkg/ccl/importccl/import_stmt_test.go, line 1219 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: non-empty error string <-> expect an error

good call. Done.

---

pkg/ccl/importccl/import_stmt_test.go, line 6439 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: unnecessary braces, you could just: const nodes = 3

copy pasta. Fixed.

---

pkg/ccl/importccl/import_stmt_test.go, line 6476 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I'd recommend modifying TestingCreateMultiRegionCluster to accept a baseDir so that you don't have to do this whole setup thing here.

Good suggestion. If only this had existed when I got start :-/. Done.

---

pkg/ccl/importccl/import_stmt_test.go, line 6490 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Is it worth adding one for GLOBAL tables here as well?

We can if you feel passionate about it. Was reluctant as the RBR is the most different, and every table type requires a new schema file.

---

pkg/ccl/importccl/import_stmt_test.go, line 6559 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: instead of having a err and an errString, you could make the empty string imply no error.

Done.

[arulajmani]

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

---

pkg/ccl/backupccl/restore_job.go, line 380 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

The reason I wrote it this way was because I wanted to avoid an else if branch below, which we would need to confirm that the RegionConfig was nil. I guess I could reorder it to avoid the nesting.

Ah, I missed that part. Discard my nit

---

pkg/ccl/importccl/import_stmt_test.go, line 6476 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Good suggestion. If only this had existed when I got start :-/. Done.

Hehe you can blame me for that :P

---

pkg/ccl/multiregionccl/multiregionccltestutils/testutils.go, line 35 at r4 (raw file):

	}

	b := ""

nit: you could change baseDir to be a string instead of a string pointer and pass in an empty string in all the places where we pass nil.

[ajstorm]

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

---

pkg/ccl/multiregionccl/multiregionccltestutils/testutils.go, line 35 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: you could change baseDir to be a string instead of a string pointer and pass in an empty string in all the places where we pass nil.

Yeah, contemplated that, but it seemed more intuitive to me to have nil denote that something's not in use, as opposed to "". If you feel strongly about it, I can make the change.

[arulajmani]

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

---

pkg/ccl/multiregionccl/multiregionccltestutils/testutils.go, line 35 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Yeah, contemplated that, but it seemed more intuitive to me to have nil denote that something's not in use, as opposed to "". If you feel strongly about it, I can make the change.

Naa no strong feelings, I'm fine merging as is

[ajstorm]

TFTRs @arulajmani @otan and @pbardea.

Will bors as soon as I can get a clean run through testrace, which is pretty reliably failing due to OOM.

[ajstorm]

bors r=otan,arulajmani,pbardea

[craig]

Build succeeded:

• GitHub CI (Cockroach)