importccl: add support for IMPORT INTO RBR table

[adityamaru]

This change overrides the default_to_database_primary_region
and gateway_region to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

To ensure that the import is idempotent across resumptions, we cache
the primary region of the database being imported into, during planning.
This information is store in the job details and flow spec to be used
when evaluating the relevant default expr/computed column.

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
importRegionOperator to allow resolution of the primary region
of the database being imported into.

Fixes: #69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality

[cockroach-teamcity]

This change is 

[adityamaru]

The last commit is the relevant one. @ multiregion folks are there any other tests that you think would be worth adding here? Currently, I just tweaked the test that would previously check for an unsupported error.

[ajstorm]

Nice to see this coming together. Have you tested the case where the IMPORT data contains data for the crdb_region column? Can we add a test for that case too?

Reviewed 4 of 5 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, and @dt)

---

pkg/ccl/importccl/import_table_creation.go, line 315 at r4 (raw file):

			dbDesc.GetID(),
			so.cf.NewCollection(nil /* TemporarySchemaProvider */),
			sql.SynthesizeRegionConfigOptionUseCache,

Out of curiosity, why use the cache here? Can we add a comment with the rationale?

---

pkg/sql/row/expr_walker.go, line 523 at r4 (raw file):

}

func importGatewayRegion(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {

Should importGatewayRegion be returning the Primary region? If so, maybe a comment as to why?

---

pkg/sql/row/expr_walker.go, line 684 at r4 (raw file):

				Types:      tree.ArgTypes{},
				ReturnType: tree.FixedReturnType(types.String),
				Info:       "Returns the primary region of the database.",

Returns the gateway region of the database?

[arulajmani]

This does raise an interesting question. I don't think this function is immutable the way it is written. Consider the case where the primary region on the database descriptor changes while the import job is executing -- wouldn't we end up returning different values in that case? I'm not very familiar with this area of the code but I wonder if that's a problem for us?

[arulajmani]

@adityamaru wrote:

This does raise an interesting question. I don't think this function is immutable the way it is written. Consider the case where the primary region on the database descriptor changes while the import job is executing -- wouldn't we end up returning different values in that case? I'm not very familiar with this area of the code but I wonder if that's a problem for us?

Yup, I'll have to cache the primary region (or descriptor) of the database at the time of planning so that it doesn't change while the import is running. Will update the change with this!

[adityamaru]

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

---

pkg/ccl/importccl/import_table_creation.go, line 315 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This does raise an interesting question. I don't think this function is immutable the way it is written. Consider the case where the primary region on the database descriptor changes while the import job is executing -- wouldn't we end up returning different values in that case? I'm not very familiar with this area of the code but I wonder if that's a problem for us?

Changed the implementation to resolve the DB primary region during import planning, and storing the primary region string on the import job for use during execution. The resolution no longer uses the cache.

---

pkg/sql/row/expr_walker.go, line 523 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Should importGatewayRegion be returning the Primary region? If so, maybe a comment as to why?

Both gateway_region and deafult_to_database_primary_region should return the cached primary region of the database being imported into. This is important for import to be idempotent across job resumptions. Added a comment about this!

---

pkg/sql/row/expr_walker.go, line 684 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Returns the gateway region of the database?

As mentioned above, we need to return the primary region of the db even when we are evaluating gateway_region().

[arulajmani]

Reviewed 5 of 25 files at r5, 1 of 1 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajstorm, @arulajmani, and @dt)

---

-- commits, line 14 at r6:
Might be worth expanding the commit message to include details about the proto changes you've made to capture the primary region during planning.

---

pkg/ccl/importccl/import_stmt.go, line 981 at r6 (raw file):

		// computed columns such as `gateway_region`.
		var databasePrimaryRegion descpb.RegionName
		if db.IsMultiRegion() {

Do we need to add a version gate here? What if an import job is planned on a 21.2 node but gets run on a 21.1 node which doesn't know how to deal with this primary region stuff?

Separately, if we do add a version gate, do we need to make some concessions for importing into GLOBAL/REGIONAL BY TABLE tables so as to not break them in a mixed version setting?

---

pkg/ccl/importccl/import_stmt.go, line 982 at r6 (raw file):

		var databasePrimaryRegion descpb.RegionName
		if db.IsMultiRegion() {
			if err := p.ExecCfg().CollectionFactory.Txn(ctx, p.ExecCfg().InternalExecutor, p.ExecCfg().DB,

Can we replace this with sql.DescsTxn instead?

---

pkg/ccl/importccl/read_import_base.go, line 65 at r6 (raw file):

	// assignment.
	evalCtx.DB = flowCtx.Cfg.DB
	evalCtx.Regions = makeImportRegionOperator(*spec.DatabasePrimaryRegion)

Is this missing a nil check on spec.DatabasePrimaryRegion for non multi-region databases?

---

pkg/jobs/jobspb/jobs.proto, line 328 at r6 (raw file):

  // field stores the databases' primary region.
  string database_primary_region = 27 [
    (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.RegionName"

Should this be nullable=false?

---

pkg/sql/execinfrapb/processors_bulk_io.proto, line 149 at r6 (raw file):

  // field stores the databases' primary region.
  optional string database_primary_region = 17 [
    (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.RegionName"

Should this be nullable=false as well?

[ajstorm]

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

---

pkg/ccl/importccl/import_table_creation.go, line 276 at r6 (raw file):

type importDatabaseRegionConfig struct {
	primaryRegion descpb.RegionName

Are we calling this "DatabaseRegionConfig" because we want to allow for its expansion later on? Just wondering why it only contains the primary region?

---

pkg/sql/row/expr_walker.go, line 523 at r4 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Both gateway_region and deafult_to_database_primary_region should return the cached primary region of the database being imported into. This is important for import to be idempotent across job resumptions. Added a comment about this!

Thanks, I get it now.

Nit: Might just be me, but I think it would be more intuitive if you had both the default_to_database_primary_region and gateway_region import functions map to the same importDefaultToDatabasePrimaryRegion function and explain in that single place why both should map to the same thing. Having a separate function here called importGatewayRegion which doesn't map to the gateway region seems like it could cause confusion down the road.

[adityamaru]

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

---

-- commits, line 14 at r6:

Previously, arulajmani (Arul Ajmani) wrote…

Might be worth expanding the commit message to include details about the proto changes you've made to capture the primary region during planning.

done.

---

pkg/ccl/importccl/import_stmt.go, line 981 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Do we need to add a version gate here? What if an import job is planned on a 21.2 node but gets run on a 21.1 node which doesn't know how to deal with this primary region stuff?

Separately, if we do add a version gate, do we need to make some concessions for importing into GLOBAL/REGIONAL BY TABLE tables so as to not break them in a mixed version setting?

I thought this through let me know if this makes sense. Also @dt to sanity check:

There are three phases in an import relevant to us:

• Plan
• Prepare descriptor for ingestion (part of job execution)
• Flow execution

An important point to note is that an older version node can never plan a distsql flow on a newer version node. This is enforced in SetupAllNodesPlanning when we decide what nodes are eligible to run a flow.

Case 1: All nodes single version -- no problem

Case 2: 21.2, 21.1, 21.1:

21.1 node will fail https://github.com/cockroachdb/cockroach/blob/release-21.1/pkg/ccl/importccl/import_stmt.go#L1256 at this point.

Case 3: 21.2, 21.2, 21.1:

If the descriptor is prepared by a 21.2 node but the distsql flow gets planned on a 21.1 node, then it will fail when attempting to type-check a default expression with a UDT. This was the behavior before these 3 PRs. The failure will happen here https://github.com/cockroachdb/cockroach/blob/release-21.1/pkg/sql/row/row_converter.go#L337 since the semaCtx has a nil type resolver in 21.1.

Case 4: 21.1, 21.2, 21.2

In this case, 21.2 node will attempt to evaluate the default expression of crdb_region but will see an empty primary region since the field was never set by the 21.1 node. I added a check that if the primary region is "" then fail the import.

Case 5: 21.1, 21.2, 21.1

Similar to case 3.

---

pkg/ccl/importccl/import_stmt.go, line 982 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Can we replace this with sql.DescsTxn instead?

done.

---

pkg/ccl/importccl/import_table_creation.go, line 276 at r6 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Are we calling this "DatabaseRegionConfig" because we want to allow for its expansion later on? Just wondering why it only contains the primary region?

I'm not sure about expansion but I needed a minimal implementation of the tree.DatabaseRegionConfig interface that would give me access to the cached primary region when CurrentDatabaseRegionConfig is called. This is needed in the builtin override that only receives an evalCtx as a parameter.
It seemed intuitive to name it importDatabaseRegionConfig so as to imply that it is a stripped-down db region config that only has access to certain fields. I'm happy to rename it to something more suitable?

---

pkg/ccl/importccl/read_import_base.go, line 65 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Is this missing a nil check on spec.DatabasePrimaryRegion for non multi-region databases?

It goes away once the proto field in processors_bulk_io.proto is made non-nullable.

---

pkg/jobs/jobspb/jobs.proto, line 328 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should this be nullable=false?

In proto3 I don't think we need to set nullable to false. It'll just be set to its zero value if it is unset which is "" I think?

---

pkg/sql/execinfrapb/processors_bulk_io.proto, line 149 at r6 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should this be nullable=false as well?

Nice catch, unfortunately, this proto is still on proto2. I'm hesitant to change it in this PR so adding the nullable clause.

---

pkg/sql/row/expr_walker.go, line 523 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Thanks, I get it now.

Nit: Might just be me, but I think it would be more intuitive if you had both the default_to_database_primary_region and gateway_region import functions map to the same importDefaultToDatabasePrimaryRegion function and explain in that single place why both should map to the same thing. Having a separate function here called importGatewayRegion which doesn't map to the gateway region seems like it could cause confusion down the road.

good point, done.

[ajstorm]

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

---

pkg/ccl/importccl/import_table_creation.go, line 276 at r6 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

I'm not sure about expansion but I needed a minimal implementation of the tree.DatabaseRegionConfig interface that would give me access to the cached primary region when CurrentDatabaseRegionConfig is called. This is needed in the builtin override that only receives an evalCtx as a parameter.
It seemed intuitive to name it importDatabaseRegionConfig so as to imply that it is a stripped-down db region config that only has access to certain fields. I'm happy to rename it to something more suitable?

That makes sense. Is it worth adding a comment above this which states that this is just a stripped-down version of multiregion.RegionConfig for use by import? That might help frame this for people who aren't immediately familiar with the RegionConfig structure.

---

pkg/sql/row/expr_walker.go, line 523 at r4 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

good point, done.

Looks good. Thanks!

[arulajmani]

Things look good to me. See my comment around the mixed version stuff, but I'll defer to your and @dt's call on how to best handle that!

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

---

pkg/ccl/importccl/import_stmt.go, line 981 at r6 (raw file):

Case 5: 21.1, 21.2, 21.1
Similar to case 3.

Did you mean to say Case 2?

This explanation makes sense at a high level. My first instinct would have been to version gate this, but looks like we're not breaking any existing behaviour here so maybe it's fine. I do wish that we had tests for all these tricky mixed version cases, but I know that'll be a fair bit of work.

Considering I don't live around these code ends, I'll leave all this to your and @dt's call.

---

pkg/ccl/importccl/import_stmt_test.go, line 6432 at r7 (raw file):

	simpleOcf := fmt.Sprintf("nodelocal://0/avro/%s", "simple.ocf")

	data := "1,\"foo\",NULL,us-east1\n"

Is it worth writing a test which writes a row referencing a region not present on the multi-region enum?

---

pkg/jobs/jobspb/jobs.proto, line 328 at r6 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

In proto3 I don't think we need to set nullable to false. It'll just be set to its zero value if it is unset which is "" I think?

heh I didn't realize we had proto3s here (or anywhere, if I'm being honest)

[dt]

In the mixed version cases that get into these error cases, with no version gate the IMPORT just fails with an error, right? and if we added a version gate, that would... also fail with an error? If so, I don't feel strongly that we need the gate, vs if there was correctness concern. /2¢

[adityamaru]

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

---

pkg/ccl/importccl/import_stmt_test.go, line 6432 at r7 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Is it worth writing a test which writes a row referencing a region not present on the multi-region enum?

done!

---

pkg/ccl/importccl/import_table_creation.go, line 276 at r6 (raw file):

Previously, ajstorm (Adam Storm) wrote…

That makes sense. Is it worth adding a comment above this which states that this is just a stripped-down version of multiregion.RegionConfig for use by import? That might help frame this for people who aren't immediately familiar with the RegionConfig structure.

done.

[adityamaru]

Unrelated bazel flake in TestSeparatedIntentsMigration.

TFTRs!

bors r=arulajmani,ajstorm,dt

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from fb180a9 to blathers/backport-release-21.2-69903: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}