sql: implement REGIONAL BY ROW logic on CREATE TABLE

[otan]

Resolves #58335
Resolves #57672

Release note (sql change): Implemented REGIONAL BY ROW logic on create
table. This is gated behind the experimental session variable
experimental_enable_implicit_column_partitioning.

[cockroach-teamcity]

This change is 

[ajstorm]

Looks really good. Nice job! 🎉

Doesn't need to be handled in this PR, but is there a way to make REGIONAL BY ROW not require the session variable (i.e. bypass the session variable check or automatically set it under the covers)? This would allow us to expose REGIONAL BY ROW directly, and keep PARTITION ALL experimental (added a comment about it below as well. Might be easy to do in this PR).

Reviewed 1 of 10 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 36 at r1 (raw file):

LOCALITY REGIONAL BY ROW

statement error REGIONAL BY ROW on an UNIQUE constraint containing PARTITION BY is not supported

Nit: We might want to reword this error message. Would "REGIONAL BY ROW on a table with a UNIQUE constraint containing PARTITION BY is not supported" be more accurate? Or simply, "REGIONAL BY ROW is not supported on partitioned tables/indexes"?

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 54 at r1 (raw file):

) LOCALITY REGIONAL BY ROW

# TODO(otan): hide the crdb_region column.

Nit: What to do here will depend on your decision around Andrei's update to the RFC. Hiding is one option, and using the AS syntax is the other option.

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 56 at r1 (raw file):

# TODO(otan): hide the crdb_region column.
query T
SELECT create_statement FROM [SHOW CREATE TABLE regional_by_row_table]

Nit: Once we get this working, can we add a test to ensure that the SHOW CREATE TABLE statement roundtrips? Maybe a TODO in this PR for that?

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 128 at r1 (raw file):

statement ok
INSERT INTO regional_by_row_table (crdb_region, pk, a, b) VALUES ('ca-central-1', 7, 8, 9)

Can we add a test where we explicitly call gateway_region() too?

---

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

	// Add implied columns under REGIONAL BY ROW.
	locality := n.Locality

Nit: might be nice to pull this out into a separate function in region_util.go.

---

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

					return nil, pgerror.New(
						pgcode.FeatureNotSupported,
						"REGIONAL BY ROW on an UNIQUE constraint containing PARTITION BY is not supported",

See comment above on wording here. Do we lose much detail by combining all of these into a single error message: "REGIONAL BY ROW can not be combined with PARTITION BY" (and down the road, "or PARTITION ALL BY"?

---

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

			Type: &tree.OIDTypeReference{OID: oid},
		}
		c.Nullable.Nullability = tree.NotNull

Can you also hide this column? And add a test case that selects all columns and validates that it doesn't show up?

---

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

		columnDefaultExprs = append(columnDefaultExprs, nil)

		// Construct the partitioning for the PARTITION ALL BY we will replace.

Super Nit: Are we really "replacing" the PARTITION ALL BY? Or "utilizing" that partitioning?

---

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

	if n.PartitionByTable != nil && n.PartitionByTable.All {
		if !evalCtx.SessionData.ImplicitColumnPartitioningEnabled {

per the comment above, I'd be in favour of removing this check here an allowing REGIONAL BY ROW table to not have to enable the experimental implicit column partitioning.

If we eventually decide that REGIONAL BY ROW isn't sufficiently baked for 21.1, we can slap the experimental flag back on it (but with a message and flag that pertain directly to REGIONAL BY ROW).

---

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

}

// zoneConfigFromRegionConfigForIndexPartition generates a desired ZoneConfig based

Nit: Does it make more sense to call this zoneConfigFromRegionConfigForPartition seeing as how the primary key index is really the table?

---

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

				SurvivalGoal:  descpb.SurvivalGoal_ZONE_FAILURE,
			},
			expected: &zonepb.ZoneConfig{

Is this just checking one of the partition's zone configs? If so, is there value in checking the others?

[otan]

de-coupling that is a bit gnarly from a glance, so if we're strong on that let's do it in a separate PR

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

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 128 at r1 (raw file):

Previously, ajstorm wrote…

Can we add a test where we explicitly call gateway_region() too?

Done.

---

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

Previously, ajstorm wrote…

Nit: might be nice to pull this out into a separate function in region_util.go.

There's some stuff in here that messes with local state (n.Defs, columnDefaultExprs) which makes this irritable to pull out.

---

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

Previously, ajstorm wrote…

See comment above on wording here. Do we lose much detail by combining all of these into a single error message: "REGIONAL BY ROW can not be combined with PARTITION BY" (and down the road, "or PARTITION ALL BY"?

I prefer being a bit more explicit since a) we can without much effort and b) the user know wheres to look :P

---

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

Previously, ajstorm wrote…

Can you also hide this column? And add a test case that selects all columns and validates that it doesn't show up?

Not without additional work which I will define as out of scope for this PR :)

---

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

Previously, ajstorm wrote…

Super Nit: Are we really "replacing" the PARTITION ALL BY? Or "utilizing" that partitioning?

Done.

---

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

Previously, ajstorm wrote…

per the comment above, I'd be in favour of removing this check here an allowing REGIONAL BY ROW table to not have to enable the experimental implicit column partitioning.

If we eventually decide that REGIONAL BY ROW isn't sufficiently baked for 21.1, we can slap the experimental flag back on it (but with a message and flag that pertain directly to REGIONAL BY ROW).

REGIONAL BY ROW relies on implicit column partitioning, and these checks are pretty deep in here.
I can streamline them out, but I'd also say that's out of scope, and people will be running into the experimental behaviour so I prefer to keep this for now (at least for this PR)

---

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

Previously, ajstorm wrote…

Is this just checking one of the partition's zone configs? If so, is there value in checking the others?

This specifically unit tests for a single partition given a name - checking all of them I think should be the logic test with the appropriate regions set.

[ajstorm]

All good with another PR. Can you leave a TODO as breadcrumbs (or open a new issue)?

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

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 157 at r2 (raw file):

query TI
INSERT INTO regional_by_row_table (crdb_region, pk, a, b) VALUES
(gateway_region()::crdb_internal_region, 23, 24, 25)

awesome!

---

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

Previously, otan (Oliver Tan) wrote…

There's some stuff in here that messes with local state (n.Defs, columnDefaultExprs) which makes this irritable to pull out.

Was worth a try. Some of these functions are already way too large for my linkings.

---

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

Previously, otan (Oliver Tan) wrote…

I prefer being a bit more explicit since a) we can without much effort and b) the user know wheres to look :P

That's fair. This one in particular I find confusing. Are we really defining "REGIONAL BY ROW on a UNIQUE constraint"? Isn't it a REGIONAL BY ROW table with a UNIQUE constraint which contains a PARTITION BY clause? Or something like that?

---

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

Previously, otan (Oliver Tan) wrote…

Not without additional work which I will define as out of scope for this PR :)

I'm starting to get the sense that you get paid on commission ;-). I'm good with another PR. Can you leave some breadcrumbs here?

---

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

Previously, otan (Oliver Tan) wrote…

REGIONAL BY ROW relies on implicit column partitioning, and these checks are pretty deep in here.
I can streamline them out, but I'd also say that's out of scope, and people will be running into the experimental behaviour so I prefer to keep this for now (at least for this PR)

You get paid on PRs, I get paid on breadcrumbs :). Can we add some here?

[otan]

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

---

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 157 at r2 (raw file):

Previously, ajstorm wrote…

awesome!

Done.

---

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

Previously, ajstorm wrote…

That's fair. This one in particular I find confusing. Are we really defining "REGIONAL BY ROW on a UNIQUE constraint"? Isn't it a REGIONAL BY ROW table with a UNIQUE constraint which contains a PARTITION BY clause? Or something like that?

ive reworded this to be "on a table with a UNIQUE constraint"

---

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

Previously, ajstorm wrote…

I'm starting to get the sense that you get paid on commission ;-). I'm good with another PR. Can you leave some breadcrumbs here?

I get paid off the highs of rolling in PRs :P yes, thought i did with the line below.
// TODO(#multiregion): set the field visibility to be hidden.

---

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

Previously, ajstorm wrote…

You get paid on PRs, I get paid on breadcrumbs :). Can we add some here?

Done.

[ajstorm]

Merge away....

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

---

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

Previously, otan (Oliver Tan) wrote…

ive reworded this to be "on a table with a UNIQUE constraint"

perfect

---

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

Previously, otan (Oliver Tan) wrote…

I get paid off the highs of rolling in PRs :P yes, thought i did with the line below.
// TODO(#multiregion): set the field visibility to be hidden.

Ahh, didn't see that line in the last review.

Nit: "field visibility"? Or "column visibility"?

[otan]

bors r=ajstorm

TFTR!

[craig]

Build failed:

• GitHub CI (Cockroach)

[otan]

bors r=ajstorm

[craig]

Build failed:

• GitHub CI (Cockroach)

[otan]

love a fresh rebase in the morning

bors r=ajstorm

[otan]

bors r-

[craig]

Canceled.

[otan]

i dislike mondays

bors r=ajstorm

[craig]

Build failed:

• GitHub CI (Cockroach)

[otan]

bors r=ajstorm

[craig]

Build succeeded:

• GitHub CI (Cockroach)