From 49a6b276608ed9a388a02ea1bbba4a6c9513b553 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 5 Mar 2021 17:10:03 +1100 Subject: [PATCH 1/6] geomfn: simplify InEpsilon checks in tests These checks were used repeatedly and copy-pasta'd, why not simplify them into the one. Release justification: non-production code change Release note: None --- pkg/geo/geomfn/affine_transforms_test.go | 47 +++------------------ pkg/geo/geomfn/distance_test.go | 11 +---- pkg/geo/geomfn/geomfn_test.go | 49 ++++++++++++++++++++++ pkg/geo/geomfn/linestring_test.go | 10 +---- pkg/geo/geomfn/snap_test.go | 18 ++++---- pkg/geo/geomfn/topology_operations_test.go | 24 +---------- 6 files changed, 67 insertions(+), 92 deletions(-) diff --git a/pkg/geo/geomfn/affine_transforms_test.go b/pkg/geo/geomfn/affine_transforms_test.go index 89005136cb58..cc5229cdd22f 100644 --- a/pkg/geo/geomfn/affine_transforms_test.go +++ b/pkg/geo/geomfn/affine_transforms_test.go @@ -423,14 +423,7 @@ func TestRotate(t *testing.T) { actual, err := Rotate(geometry, tc.rotRadians) require.NoError(t, err) - // Compare FlatCoords and assert they are within epsilon. - // This is because they exact matches may encounter rounding issues. - actualGeomT, err := actual.AsGeomT() - require.NoError(t, err) - require.Equal(t, tc.expected.SRID(), actualGeomT.SRID()) - require.Equal(t, tc.expected.Layout(), actualGeomT.Layout()) - require.IsType(t, tc.expected, actualGeomT) - require.InEpsilonSlice(t, tc.expected.FlatCoords(), actualGeomT.FlatCoords(), 0.00001) + requireGeometryWithinEpsilon(t, requireGeometryFromGeomT(t, tc.expected), actual, 1e-5) }) } } @@ -530,15 +523,8 @@ func TestRotateWithPointOrigin(t *testing.T) { require.EqualError(t, err, tt.wantErrStr) } else { require.NoError(t, err) + requireGeometryWithinEpsilon(t, requireGeometryFromGeomT(t, tt.wantGeom), got, 1e-5) } - // Compare FlatCoords and assert they are within epsilon. - // This is because they exact matches may encounter rounding issues. - actualGeomT, err := got.AsGeomT() - require.NoError(t, err) - require.Equal(t, tt.wantGeom.SRID(), actualGeomT.SRID()) - require.Equal(t, tt.wantGeom.Layout(), actualGeomT.Layout()) - require.IsType(t, tt.wantGeom, actualGeomT) - require.InEpsilonSlice(t, tt.wantGeom.FlatCoords(), actualGeomT.FlatCoords(), 0.00001) }) } } @@ -683,14 +669,7 @@ func TestRotateX(t *testing.T) { actual, err := RotateX(geometry, tc.rotRadians) require.NoError(t, err) - // Compare FlatCoords and assert they are within epsilon. - // This is because they exact matches may encounter rounding issues. - actualGeomT, err := actual.AsGeomT() - require.NoError(t, err) - require.Equal(t, tc.expected.SRID(), actualGeomT.SRID()) - require.Equal(t, tc.expected.Layout(), actualGeomT.Layout()) - require.IsType(t, tc.expected, actualGeomT) - require.InEpsilonSlice(t, tc.expected.FlatCoords(), actualGeomT.FlatCoords(), 0.00001) + requireGeometryWithinEpsilon(t, requireGeometryFromGeomT(t, tc.expected), actual, 1e-5) }) } } @@ -765,15 +744,7 @@ func TestRotateY(t *testing.T) { actual, err := RotateY(geometry, tc.rotRadians) require.NoError(t, err) - - // Compare FlatCoords and assert they are within epsilon. - // This is because they exact matches may encounter rounding issues. - actualGeomT, err := actual.AsGeomT() - require.NoError(t, err) - require.Equal(t, tc.expected.SRID(), actualGeomT.SRID()) - require.Equal(t, tc.expected.Layout(), actualGeomT.Layout()) - require.IsType(t, tc.expected, actualGeomT) - require.InEpsilonSlice(t, tc.expected.FlatCoords(), actualGeomT.FlatCoords(), 0.00001) + requireGeometryWithinEpsilon(t, requireGeometryFromGeomT(t, tc.expected), actual, 1e-5) }) } } @@ -848,15 +819,7 @@ func TestRotateZ(t *testing.T) { actual, err := RotateZ(geometry, tc.rotRadians) require.NoError(t, err) - - // Compare FlatCoords and assert they are within epsilon. - // This is because they exact matches may encounter rounding issues. - actualGeomT, err := actual.AsGeomT() - require.NoError(t, err) - require.Equal(t, tc.expected.SRID(), actualGeomT.SRID()) - require.Equal(t, tc.expected.Layout(), actualGeomT.Layout()) - require.IsType(t, tc.expected, actualGeomT) - require.InEpsilonSlice(t, tc.expected.FlatCoords(), actualGeomT.FlatCoords(), 0.00001) + requireGeometryWithinEpsilon(t, requireGeometryFromGeomT(t, tc.expected), actual, 1e-5) }) } } diff --git a/pkg/geo/geomfn/distance_test.go b/pkg/geo/geomfn/distance_test.go index 9079dca834ab..50532354f0bd 100644 --- a/pkg/geo/geomfn/distance_test.go +++ b/pkg/geo/geomfn/distance_test.go @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/geo" "github.com/cockroachdb/cockroach/pkg/geo/geos" "github.com/stretchr/testify/require" - "github.com/twpayne/go-geom" ) var distanceTestCases = []struct { @@ -979,18 +978,12 @@ func TestClosestPoint(t *testing.T) { gB, err := geo.ParseGeometry(tc.geomB) require.NoError(t, err) - ret, err := ClosestPoint(gA, gB) - require.NoError(t, err) - retAsGeomT, err := ret.AsGeomT() - require.NoError(t, err) - expected, err := geo.ParseGeometry(tc.expected) require.NoError(t, err) - expectedAsGeomT, err := expected.AsGeomT() + ret, err := ClosestPoint(gA, gB) require.NoError(t, err) - require.InEpsilon(t, expectedAsGeomT.(*geom.Point).X(), retAsGeomT.(*geom.Point).X(), 2e-10) - require.InEpsilon(t, expectedAsGeomT.(*geom.Point).Y(), retAsGeomT.(*geom.Point).Y(), 2e-10) + requireGeometryWithinEpsilon(t, expected, ret, 2e-10) }) } diff --git a/pkg/geo/geomfn/geomfn_test.go b/pkg/geo/geomfn/geomfn_test.go index 505927f49747..caeaeccbcf2e 100644 --- a/pkg/geo/geomfn/geomfn_test.go +++ b/pkg/geo/geomfn/geomfn_test.go @@ -14,7 +14,9 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/geo" + "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" + "github.com/twpayne/go-geom" ) var mismatchingSRIDGeometryA = geo.MustParseGeometry("SRID=4004;POINT(1.0 1.0)") @@ -127,3 +129,50 @@ func TestRemoveConsecutivePointsFromGeomT(t *testing.T) { }) } } + +func requireGeometryFromGeomT(t *testing.T, g geom.T) geo.Geometry { + ret, err := geo.MakeGeometryFromGeomT(g) + require.NoError(t, err) + return ret +} + +// requireGeometryWithinEpsilon and ensures the geometry shape and SRID are equal, +// and that each coordinate is within the provided epsilon. +func requireGeometryWithinEpsilon(t *testing.T, expected, got geo.Geometry, epsilon float64) { + expectedT, err := expected.AsGeomT() + require.NoError(t, err) + gotT, err := got.AsGeomT() + require.NoError(t, err) + requireGeomTWithinEpsilon(t, expectedT, gotT, epsilon) +} + +func requireGeomTWithinEpsilon(t *testing.T, expectedT, gotT geom.T, epsilon float64) { + require.Equal(t, expectedT.SRID(), gotT.SRID()) + require.Equal(t, expectedT.Layout(), gotT.Layout()) + require.IsType(t, expectedT, gotT) + switch lhs := expectedT.(type) { + case *geom.Point, *geom.LineString: + require.InEpsilonSlice(t, expectedT.FlatCoords(), gotT.FlatCoords(), epsilon) + case *geom.MultiPoint, *geom.Polygon, *geom.MultiLineString: + require.Equal(t, expectedT.Ends(), gotT.Ends()) + require.InEpsilonSlice(t, expectedT.FlatCoords(), gotT.FlatCoords(), epsilon) + case *geom.MultiPolygon: + require.Equal(t, expectedT.Ends(), gotT.Ends()) + require.Equal(t, expectedT.Endss(), gotT.Endss()) + require.InEpsilonSlice(t, expectedT.FlatCoords(), gotT.FlatCoords(), epsilon) + case *geom.GeometryCollection: + rhs, ok := gotT.(*geom.GeometryCollection) + require.True(t, ok) + require.Len(t, rhs.Geoms(), len(lhs.Geoms())) + for i := range lhs.Geoms() { + requireGeomTWithinEpsilon( + t, + lhs.Geom(i), + rhs.Geom(i), + epsilon, + ) + } + default: + panic(errors.AssertionFailedf("unknown geometry type: %T", expectedT)) + } +} diff --git a/pkg/geo/geomfn/linestring_test.go b/pkg/geo/geomfn/linestring_test.go index 23304ef076cc..b6e7aa9d4a04 100644 --- a/pkg/geo/geomfn/linestring_test.go +++ b/pkg/geo/geomfn/linestring_test.go @@ -571,15 +571,7 @@ func TestLineSubstring(t *testing.T) { require.Equal(t, tt.wantErrString, err.Error()) return } - - // match the shape - gWantGeom, err := geo.MakeGeometryFromGeomT(tt.wantGeomT) - require.NoError(t, err) - require.Equal(t, gWantGeom.ShapeType().String(), got.ShapeType().String()) - - geomT, err := got.AsGeomT() - require.NoError(t, err) - require.InEpsilonSlice(t, tt.wantGeomT.FlatCoords(), geomT.FlatCoords(), 0.0001) + requireGeometryWithinEpsilon(t, requireGeometryFromGeomT(t, tt.wantGeomT), got, 1e-4) }) } } diff --git a/pkg/geo/geomfn/snap_test.go b/pkg/geo/geomfn/snap_test.go index 15e999ef13aa..703acadefe87 100644 --- a/pkg/geo/geomfn/snap_test.go +++ b/pkg/geo/geomfn/snap_test.go @@ -59,7 +59,13 @@ func TestSnap(t *testing.T) { input: geom.NewMultiPolygonFlat(geom.XY, []float64{1, 1, 2, 2, 3, 3, 1, 1, 4, 4, 5, 5, 6, 6, 4, 4}, [][]int{{8}, {16}}), target: geom.NewLineStringFlat(geom.XY, []float64{5, 107, 54, 84, 101, 100}), tolerance: 200, - expected: geom.NewMultiPolygonFlat(geom.XY, []float64{1, 1, 2, 2, 5, 107, 54, 84, 101, 100, 1, 1, 4, 4, 5, 5, 5, 107, 54, 84, 101, 100, 4, 4}, [][]int{{8}, {16}}), + expected: geom.NewMultiPolygonFlat( + geom.XY, + []float64{ + 1, 1, 2, 2, 5, 107, 54, 84, 101, 100, 1, 1, 4, 4, 5, 5, 5, 107, 54, 84, 101, 100, 4, 4, + }, + [][]int{{12}, {24}}, + ), }, } @@ -73,15 +79,7 @@ func TestSnap(t *testing.T) { actual, err := Snap(input, target, tc.tolerance) require.NoError(t, err) - // Compare FlatCoords and assert they are within epsilon. - // This is because they exact matches may encounter rounding issues. - actualGeomT, err := actual.AsGeomT() - - require.NoError(t, err) - require.Equal(t, tc.expected.SRID(), actualGeomT.SRID()) - require.Equal(t, tc.expected.Layout(), actualGeomT.Layout()) - require.IsType(t, tc.expected, actualGeomT) - require.InEpsilonSlice(t, tc.expected.FlatCoords(), actualGeomT.FlatCoords(), 0.00001) + requireGeometryWithinEpsilon(t, requireGeometryFromGeomT(t, tc.expected), actual, 1e-5) }) } } diff --git a/pkg/geo/geomfn/topology_operations_test.go b/pkg/geo/geomfn/topology_operations_test.go index ea22fb23fbb4..4bf902b5d40c 100644 --- a/pkg/geo/geomfn/topology_operations_test.go +++ b/pkg/geo/geomfn/topology_operations_test.go @@ -73,19 +73,9 @@ func TestCentroid(t *testing.T) { require.NoError(t, err) ret, err := Centroid(g) require.NoError(t, err) - - retAsGeomT, err := ret.AsGeomT() - require.NoError(t, err) - expected, err := geo.ParseGeometry(tc.expected) require.NoError(t, err) - expectedAsGeomT, err := expected.AsGeomT() - require.NoError(t, err) - - // Ensure points are close in terms of precision. - require.InEpsilon(t, expectedAsGeomT.(*geom.Point).X(), retAsGeomT.(*geom.Point).X(), 2e-10) - require.InEpsilon(t, expectedAsGeomT.(*geom.Point).Y(), retAsGeomT.(*geom.Point).Y(), 2e-10) - require.Equal(t, expected.SRID(), ret.SRID()) + requireGeometryWithinEpsilon(t, expected, ret, 2e-10) }) } } @@ -293,19 +283,9 @@ func TestPointOnSurface(t *testing.T) { require.NoError(t, err) ret, err := PointOnSurface(g) require.NoError(t, err) - - retAsGeomT, err := ret.AsGeomT() - require.NoError(t, err) - expected, err := geo.ParseGeometry(tc.expected) require.NoError(t, err) - expectedAsGeomT, err := expected.AsGeomT() - require.NoError(t, err) - - // Ensure points are close in terms of precision. - require.InEpsilon(t, expectedAsGeomT.(*geom.Point).X(), retAsGeomT.(*geom.Point).X(), 2e-10) - require.InEpsilon(t, expectedAsGeomT.(*geom.Point).Y(), retAsGeomT.(*geom.Point).Y(), 2e-10) - require.Equal(t, expected.SRID(), ret.SRID()) + requireGeometryWithinEpsilon(t, expected, ret, 2e-10) }) } } From c869555c1a7cdb51167ccab361fe4f23752006ce Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 4 Mar 2021 15:38:39 +1100 Subject: [PATCH 2/6] sql: prevent PARTITION BY on multi-region databases for CREATE TABLE Also refactored RegionEnumID out of NewTableDesc in favor of taking in RegionConfig instead. This means we have less fetching to do inside NewTableDesc. Release justification: low-risk updates to new functionality. Release note: None --- pkg/ccl/importccl/import_table_creation.go | 7 +- .../testdata/logic_test/multi_region | 92 +++++++++++- .../logic_test/multi_region_mixed_20.2_21.1 | 2 +- .../testdata/logic_test/regional_by_row | 42 ++++-- pkg/sql/catalog/dbdesc/database_desc.go | 5 + pkg/sql/catalog/descriptor.go | 1 + pkg/sql/create_table.go | 131 ++++++++++-------- pkg/sql/testutils.go | 2 +- pkg/sql/virtual_schema.go | 4 +- 9 files changed, 203 insertions(+), 83 deletions(-) diff --git a/pkg/ccl/importccl/import_table_creation.go b/pkg/ccl/importccl/import_table_creation.go index d9ebb246b7e6..167cbb4b2295 100644 --- a/pkg/ccl/importccl/import_table_creation.go +++ b/pkg/ccl/importccl/import_table_creation.go @@ -164,7 +164,7 @@ func MakeSimpleTableDescriptor( parentID, parentSchemaID, tableID, - descpb.InvalidID, + nil, /* regionConfig */ hlc.Timestamp{WallTime: walltime}, descpb.NewDefaultPrivilegeDescriptor(security.AdminRoleName()), affected, @@ -172,6 +172,11 @@ func MakeSimpleTableDescriptor( &evalCtx, &sessiondata.SessionData{}, /* sessionData */ tree.PersistencePermanent, + // We need to bypass the LOCALITY on non multi-region check here because + // we cannot access the database region config at import level. + // There is code that only allows REGIONAL BY TABLE tables to be imported, + // which will safely execute even if the locality check is bypassed. + sql.NewTableDescOptionBypassLocalityOnNonMultiRegionDatabaseCheck(), ) if err != nil { return nil, err diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region b/pkg/ccl/logictestccl/testdata/logic_test/multi_region index 80ae31c7f554..25174932fc3c 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region @@ -16,6 +16,12 @@ ap-southeast-2 {ap-az1,ap-az2,ap-az3} ca-central-1 {ca-az1,ca-az2,ca-az3} us-east-1 {us-az1,us-az2,us-az3} +statement error cannot set LOCALITY on a table in a database that is not multi-region enabled +CREATE TABLE regional_by_table_table (pk int) LOCALITY REGIONAL BY TABLE + +statement error cannot set LOCALITY on a table in a database that is not multi-region enabled +CREATE TABLE global_table (pk int) LOCALITY GLOBAL + statement ok CREATE DATABASE region_test_db PRIMARY REGION "ap-southeast-2" SURVIVE ZONE FAILURE @@ -84,10 +90,10 @@ USE multi_region_test_db query IITI colnames SELECT * FROM system.namespace WHERE name='crdb_internal_region' ---- -parentID parentSchemaID name id -53 29 crdb_internal_region 54 -56 29 crdb_internal_region 57 -59 29 crdb_internal_region 60 +parentID parentSchemaID name id +55 29 crdb_internal_region 56 +58 29 crdb_internal_region 59 +61 29 crdb_internal_region 62 query TTTT colnames SHOW ENUMS FROM region_test_db.public @@ -276,6 +282,82 @@ CREATE DATABASE invalid_region_db PRIMARY REGION "region_no_exists" statement error region "ap-southeast-2" defined multiple times CREATE DATABASE duplicate_region_name_db PRIMARY REGION "ap-southeast-2" REGIONS "ap-southeast-2", "ap-southeast-2" +statement error multi-region tables containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY +) +PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) +) + +statement error multi-region tables containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY +) +PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) +) +LOCALITY GLOBAL + +statement error multi-region tables containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY +) +PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) +) +LOCALITY REGIONAL BY TABLE + +statement error multi-region tables with an INDEX containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY, + INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) + ) +) + +statement error multi-region tables with an INDEX containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY, + INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) + ) +) LOCALITY GLOBAL + +statement error multi-region tables with an INDEX containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY, + INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) + ) +) LOCALITY REGIONAL BY TABLE + +statement error multi-region tables with an UNIQUE constraint containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY, + UNIQUE INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) + ) +) + +statement error multi-region tables with an UNIQUE constraint containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY, + UNIQUE INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) + ) +) +LOCALITY GLOBAL + +statement error multi-region tables with an UNIQUE constraint containing PARTITION BY are not supported +CREATE TABLE partition_by_table_in_mr_database ( + id INT PRIMARY KEY, + UNIQUE INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) + ) +) +LOCALITY REGIONAL BY TABLE + statement ok CREATE TABLE regional_primary_region_table (a int) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION @@ -399,7 +481,7 @@ CREATE DATABASE new_db statement ok USE new_db -statement error database new_db is not multi-region enabled, but table cannot_create_table_no_multiregion has locality GLOBAL set +statement error cannot set LOCALITY on a table in a database that is not multi-region enabled CREATE TABLE cannot_create_table_no_multiregion (a int) LOCALITY GLOBAL # Test adding a primary region to the system database. diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 index fe75dd39a866..1d344181e1ff 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 @@ -18,7 +18,7 @@ ALTER DATABASE d PRIMARY REGION "us-east1" statement error database must have associated regions before a survival goal can be set\nHINT: you must first add a primary region to the database using ALTER DATABASE d PRIMARY REGION ALTER DATABASE d SURVIVE ZONE FAILURE -statement error database test is not multi-region enabled, but table t has locality REGIONAL BY TABLE IN PRIMARY REGION set +statement error cannot set LOCALITY on a table in a database that is not multi-region enabled CREATE TABLE t () LOCALITY REGIONAL BY TABLE statement ok diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index f2047ce08745..f107da0ae40c 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -3,26 +3,44 @@ statement ok CREATE DATABASE multi_region_test_db PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2", "us-east-1" SURVIVE REGION FAILURE -statement error cannot set LOCALITY on a database that is not multi-region enabled +statement error cannot set LOCALITY on a table in a database that is not multi-region enabled CREATE TABLE regional_by_row_table (pk int) LOCALITY REGIONAL BY ROW statement ok USE multi_region_test_db -statement error REGIONAL BY ROW on a TABLE containing PARTITION BY is not supported +statement error multi-region tables containing PARTITION BY are not supported CREATE TABLE regional_by_row_table ( pk int ) PARTITION BY LIST (pk) (PARTITION one VALUES IN ((1))) LOCALITY REGIONAL BY ROW -statement error REGIONAL BY ROW on a TABLE containing PARTITION BY is not supported +statement error multi-region tables containing PARTITION BY are not supported CREATE TABLE regional_by_row_table ( pk int ) PARTITION BY NOTHING LOCALITY REGIONAL BY ROW +statement error multi-region tables with an INDEX containing PARTITION BY are not supported +CREATE TABLE regional_by_row_table ( + pk int, + INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "pk" VALUES IN (1) + ) +) +LOCALITY REGIONAL BY ROW + +statement error multi-region tables with an UNIQUE constraint containing PARTITION BY are not supported +CREATE TABLE regional_by_row_table ( + pk int, + UNIQUE INDEX idx(id) PARTITION BY LIST (id) ( + PARTITION "pk" VALUES IN (1) + ) +) +LOCALITY REGIONAL BY ROW + statement ok SET experimental_enable_hash_sharded_indexes = true @@ -45,7 +63,7 @@ CREATE TABLE regional_by_row_table ( INDEX(a) USING HASH WITH BUCKET_COUNT = 8 ) LOCALITY REGIONAL BY ROW -statement error REGIONAL BY ROW on a table with an INDEX containing PARTITION BY is not supported +statement error multi-region tables with an INDEX containing PARTITION BY are not supported CREATE TABLE regional_by_row_table ( pk int, a int, @@ -53,7 +71,7 @@ CREATE TABLE regional_by_row_table ( ) LOCALITY REGIONAL BY ROW -statement error REGIONAL BY ROW on a table with an INDEX containing PARTITION BY is not supported +statement error multi-region tables with an INDEX containing PARTITION BY are not supported CREATE TABLE regional_by_row_table ( pk int, a int, @@ -61,7 +79,7 @@ CREATE TABLE regional_by_row_table ( ) LOCALITY REGIONAL BY ROW -statement error REGIONAL BY ROW on a table with an INDEX containing PARTITION BY is not supported +statement error multi-region tables with an INDEX containing PARTITION BY are not supported CREATE TABLE regional_by_row_table ( pk int, a int, @@ -70,7 +88,7 @@ CREATE TABLE regional_by_row_table ( ) LOCALITY REGIONAL BY ROW -statement error REGIONAL BY ROW on a table with an UNIQUE constraint containing PARTITION BY is not supported +statement error multi-region tables with an UNIQUE constraint containing PARTITION BY are not supported CREATE TABLE regional_by_row_table ( pk int, a int, @@ -78,7 +96,7 @@ CREATE TABLE regional_by_row_table ( ) LOCALITY REGIONAL BY ROW -statement error REGIONAL BY ROW on a table with an UNIQUE constraint containing PARTITION BY is not supported +statement error multi-region tables with an UNIQUE constraint containing PARTITION BY are not supported CREATE TABLE regional_by_row_table ( pk int, a int, @@ -609,7 +627,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY OR message LIKE 'Scan%' ORDER BY ordinality ASC ---- -Scan /Table/71/1/"@"/1{-/#}, /Table/71/1/"\x80"/1{-/#}, /Table/71/1/"\xc0"/1{-/#} +Scan /Table/73/1/"@"/1{-/#}, /Table/73/1/"\x80"/1{-/#}, /Table/73/1/"\xc0"/1{-/#} fetched: /regional_by_row_table/primary/'ap-southeast-2'/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}' output row: [1 1 2 3 '{"a": "b"}'] @@ -648,7 +666,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY OR message LIKE 'Scan%' ORDER BY ordinality ASC ---- -Scan /Table/71/1/"@"/1{-/#} +Scan /Table/73/1/"@"/1{-/#} fetched: /regional_by_row_table/primary/'ap-southeast-2'/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}' output row: [1 1 2 3 '{"a": "b"}'] @@ -663,8 +681,8 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY OR message LIKE 'Scan%' ORDER BY ordinality ASC ---- -Scan /Table/71/1/"@"/10{-/#} -Scan /Table/71/1/"\x80"/10{-/#}, /Table/71/1/"\xc0"/10{-/#} +Scan /Table/73/1/"@"/10{-/#} +Scan /Table/73/1/"\x80"/10{-/#}, /Table/73/1/"\xc0"/10{-/#} fetched: /regional_by_row_table/primary/'ca-central-1'/10/pk2/a/b -> /10/11/12 output row: [10 10 11 12 NULL] diff --git a/pkg/sql/catalog/dbdesc/database_desc.go b/pkg/sql/catalog/dbdesc/database_desc.go index c919cd31919d..1d84f14862c8 100644 --- a/pkg/sql/catalog/dbdesc/database_desc.go +++ b/pkg/sql/catalog/dbdesc/database_desc.go @@ -211,6 +211,11 @@ func (desc *Immutable) IsMultiRegion() bool { return desc.RegionConfig != nil } +// GetRegionConfig returns the region config for the given database. +func (desc *Immutable) GetRegionConfig() *descpb.DatabaseDescriptor_RegionConfig { + return desc.RegionConfig +} + // RegionNames returns the multi-region regions that have been added to a // database. func (desc *Immutable) RegionNames() (descpb.RegionNames, error) { diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index d5ddc04aed05..a84b8dfb0b65 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -100,6 +100,7 @@ type DatabaseDescriptor interface { tree.SchemaMeta DatabaseDesc() *descpb.DatabaseDescriptor + GetRegionConfig() *descpb.DatabaseDescriptor_RegionConfig RegionNames() (descpb.RegionNames, error) IsMultiRegion() bool PrimaryRegionName() (descpb.RegionName, error) diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index a0e73070725a..e0657de829e5 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -411,20 +411,11 @@ func (n *createTableNode) startExec(params runParams) error { } if desc.LocalityConfig != nil { - _, dbDesc, err := params.p.Descriptors().GetImmutableDatabaseByID( - params.ctx, - params.p.txn, - desc.ParentID, - tree.DatabaseLookupFlags{Required: true}, - ) - if err != nil { - return errors.Wrap(err, "error resolving database for multi-region") - } if err := ApplyZoneConfigForMultiRegionTable( params.ctx, params.p.txn, params.p.ExecCfg(), - *dbDesc.RegionConfig, + *n.dbDesc.GetRegionConfig(), desc, ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes, ); err != nil { @@ -436,7 +427,7 @@ func (n *createTableNode) startExec(params runParams) error { typeDesc, err := params.p.Descriptors().GetMutableTypeVersionByID( params.ctx, params.p.txn, - dbDesc.RegionConfig.RegionEnumID, + n.dbDesc.GetRegionConfig().RegionEnumID, ) if err != nil { return errors.Wrap(err, "error resolving multi-region enum") @@ -1475,6 +1466,21 @@ func newTableDescIfAs( return desc, nil } +type newTableDescOptions struct { + bypassLocalityOnNonMultiRegionDatabaseCheck bool +} + +// NewTableDescOption is an option on NewTableDesc. +type NewTableDescOption func(o *newTableDescOptions) + +// NewTableDescOptionBypassLocalityOnNonMultiRegionDatabaseCheck will allow +// LOCALITY on non multi-region tables. +func NewTableDescOptionBypassLocalityOnNonMultiRegionDatabaseCheck() NewTableDescOption { + return func(o *newTableDescOptions) { + o.bypassLocalityOnNonMultiRegionDatabaseCheck = true + } +} + // NewTableDesc creates a table descriptor from a CreateTable statement. // // txn and vt can be nil if the table to be created does not contain references @@ -1506,7 +1512,8 @@ func NewTableDesc( vt resolver.SchemaResolver, st *cluster.Settings, n *tree.CreateTable, - parentID, parentSchemaID, id, regionEnumID descpb.ID, + parentID, parentSchemaID, id descpb.ID, + regionConfig *descpb.DatabaseDescriptor_RegionConfig, creationTime hlc.Timestamp, privileges *descpb.PrivilegeDescriptor, affected map[descpb.ID]*tabledesc.Mutable, @@ -1514,11 +1521,17 @@ func NewTableDesc( evalCtx *tree.EvalContext, sessionData *sessiondata.SessionData, persistence tree.Persistence, + inOpts ...NewTableDescOption, ) (*tabledesc.Mutable, error) { // Used to delay establishing Column/Sequence dependency until ColumnIDs have // been populated. columnDefaultExprs := make([]tree.TypedExpr, len(n.Defs)) + var opts newTableDescOptions + for _, o := range inOpts { + o(&opts) + } + desc := tabledesc.InitTableDescriptor( id, parentID, parentSchemaID, n.Table.Table(), creationTime, privileges, persistence, ) @@ -1543,37 +1556,53 @@ func NewTableDesc( } } - // Add implied columns under REGIONAL BY ROW. - locality := n.Locality - isRegionalByRow := locality != nil && locality.LocalityLevel == tree.LocalityLevelRow + isRegionalByRow := n.Locality != nil && n.Locality.LocalityLevel == tree.LocalityLevelRow var partitionAllBy *tree.PartitionBy primaryIndexColumnSet := make(map[string]struct{}) - if isRegionalByRow { - // Check the table is multi-region enabled. - dbDesc, err := catalogkv.MustGetDatabaseDescByID(ctx, txn, evalCtx.Codec, parentID) - if err != nil { - return nil, err - } - if !dbDesc.IsMultiRegion() { - return nil, pgerror.Newf( - pgcode.InvalidTableDefinition, - "cannot set LOCALITY on a database that is not multi-region enabled", - ) - } - regionalByRowCol := tree.RegionalByRowRegionDefaultColName - if n.Locality.RegionalByRowColumn != "" { - regionalByRowCol = n.Locality.RegionalByRowColumn - } + if n.Locality != nil && regionConfig == nil && + !opts.bypassLocalityOnNonMultiRegionDatabaseCheck { + return nil, pgerror.Newf( + pgcode.InvalidTableDefinition, + "cannot set LOCALITY on a table in a database that is not multi-region enabled", + ) + } - // Check PARTITION BY is not set on any column definition. + if n.Locality != nil || regionConfig != nil { + // Check PARTITION BY is not set on any column, index or table definition. if n.PartitionByTable.ContainsPartitioningClause() { return nil, pgerror.New( pgcode.FeatureNotSupported, - "REGIONAL BY ROW on a TABLE containing PARTITION BY is not supported", + "multi-region tables containing PARTITION BY are not supported", ) } + for _, def := range n.Defs { + switch d := def.(type) { + case *tree.IndexTableDef: + if d.PartitionByIndex.ContainsPartitioningClause() { + return nil, pgerror.New( + pgcode.FeatureNotSupported, + "multi-region tables with an INDEX containing PARTITION BY are not supported", + ) + } + case *tree.UniqueConstraintTableDef: + if d.PartitionByIndex.ContainsPartitioningClause() { + return nil, pgerror.New( + pgcode.FeatureNotSupported, + "multi-region tables with an UNIQUE constraint containing PARTITION BY are not supported", + ) + } + } + } + } + + // Add implied columns under REGIONAL BY ROW. + if isRegionalByRow { + regionalByRowCol := tree.RegionalByRowRegionDefaultColName + if n.Locality.RegionalByRowColumn != "" { + regionalByRowCol = n.Locality.RegionalByRowColumn + } // Check no interleaving is on the table. if n.Interleave != nil { @@ -1592,7 +1621,7 @@ func NewTableDesc( if err != nil { return nil, errors.Wrap(err, "error resolving REGIONAL BY ROW column type") } - if t.Oid() != typedesc.TypeIDToOID(dbDesc.RegionConfig.RegionEnumID) { + if t.Oid() != typedesc.TypeIDToOID(regionConfig.RegionEnumID) { err = pgerror.Newf( pgcode.InvalidTableDefinition, "cannot use column %s which has type %s in REGIONAL BY ROW", @@ -1601,7 +1630,7 @@ func NewTableDesc( ) if t, terr := vt.ResolveTypeByOID( ctx, - typedesc.TypeIDToOID(dbDesc.RegionConfig.RegionEnumID), + typedesc.TypeIDToOID(regionConfig.RegionEnumID), ); terr == nil { if n.Locality.RegionalByRowColumn != tree.RegionalByRowRegionNotSpecifiedName { // In this case, someone used REGIONAL BY ROW AS where @@ -1624,20 +1653,7 @@ func NewTableDesc( } return nil, err } - } - case *tree.IndexTableDef: - if d.PartitionByIndex.ContainsPartitioningClause() { - return nil, pgerror.New( - pgcode.FeatureNotSupported, - "REGIONAL BY ROW on a table with an INDEX containing PARTITION BY is not supported", - ) - } - case *tree.UniqueConstraintTableDef: - if d.PartitionByIndex.ContainsPartitioningClause() { - return nil, pgerror.New( - pgcode.FeatureNotSupported, - "REGIONAL BY ROW on a table with an UNIQUE constraint containing PARTITION BY is not supported", - ) + break } } } @@ -1650,7 +1666,7 @@ func NewTableDesc( regionalByRowCol.String(), ) } - oid := typedesc.TypeIDToOID(dbDesc.RegionConfig.RegionEnumID) + oid := typedesc.TypeIDToOID(regionConfig.RegionEnumID) n.Defs = append( n.Defs, regionalByRowDefaultColDef(oid, regionalByRowGatewayRegionDefaultExpr(oid)), @@ -1661,7 +1677,7 @@ func NewTableDesc( // Construct the partitioning for the PARTITION ALL BY. desc.PartitionAllBy = true partitionAllBy = partitionByForRegionalByRow( - *dbDesc.RegionConfig, + *regionConfig, regionalByRowCol, ) // Leading region column of REGIONAL BY ROW is part of the primary @@ -1696,7 +1712,7 @@ func NewTableDesc( } allowImplicitPartitioning := (sessionData != nil && sessionData.ImplicitColumnPartitioningEnabled) || - (locality != nil && locality.LocalityLevel == tree.LocalityLevelRow) + (n.Locality != nil && n.Locality.LocalityLevel == tree.LocalityLevelRow) // We defer index creation of implicit indexes in column definitions // until after all columns have been initialized, in case there is @@ -2338,7 +2354,7 @@ func NewTableDesc( return nil, err } - if regionEnumID != descpb.InvalidID || n.Locality != nil { + if regionConfig != nil || n.Locality != nil { localityTelemetryName := "unspecified" if n.Locality != nil { localityTelemetryName = n.Locality.TelemetryName() @@ -2423,19 +2439,12 @@ func newTableDesc( } } - regionEnumID := descpb.InvalidID _, dbDesc, err := params.p.Descriptors().GetImmutableDatabaseByID( params.ctx, params.p.txn, parentID, tree.DatabaseLookupFlags{}, ) if err != nil { return nil, err } - if dbDesc.IsMultiRegion() { - regionEnumID, err = dbDesc.MultiRegionEnumID() - if err != nil { - return nil, err - } - } // We need to run NewTableDesc with caching disabled, because // it needs to pull in descriptors from FK depended-on tables @@ -2451,7 +2460,7 @@ func newTableDesc( parentID, parentSchemaID, id, - regionEnumID, + dbDesc.GetRegionConfig(), creationTime, privileges, affected, diff --git a/pkg/sql/testutils.go b/pkg/sql/testutils.go index 1a1df55b1b55..5fa31ce9b19e 100644 --- a/pkg/sql/testutils.go +++ b/pkg/sql/testutils.go @@ -51,7 +51,7 @@ func CreateTestTableDescriptor( parentID, keys.PublicSchemaID, id, - descpb.InvalidID, + nil, /* regionConfig */ hlc.Timestamp{}, /* creationTime */ privileges, nil, /* affected */ diff --git a/pkg/sql/virtual_schema.go b/pkg/sql/virtual_schema.go index 0a2fa7bb8071..aecaf087582d 100644 --- a/pkg/sql/virtual_schema.go +++ b/pkg/sql/virtual_schema.go @@ -166,8 +166,8 @@ func (t virtualSchemaTable) initVirtualTableDesc( 0, /* parentID */ parentSchemaID, id, - descpb.InvalidID, /* regionEnumID */ - startTime, /* creationTime */ + nil, /* regionConfig */ + startTime, /* creationTime */ publicSelectPrivileges, nil, /* affected */ nil, /* semaCtx */ From 1702de6ff86cad38573e24a78e29d0398f47cb42 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 4 Mar 2021 16:14:38 +1100 Subject: [PATCH 3/6] sql: prevent ALTER TABLE ... PARTITION BY for multi-region tables Release justification: low-risk update to new functionality Release note: None --- pkg/ccl/logictestccl/testdata/logic_test/multi_region | 10 ++++++++++ .../logictestccl/testdata/logic_test/regional_by_row | 6 ++++++ pkg/sql/alter_table.go | 6 ++++++ 3 files changed, 22 insertions(+) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region b/pkg/ccl/logictestccl/testdata/logic_test/multi_region index 25174932fc3c..e55d9974da73 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region @@ -384,6 +384,11 @@ DATABASE multi_region_test_db ALTER DATABASE multi_region_test_db CONFIGURE ZON voter_constraints = '{+region=ca-central-1: 2}', lease_preferences = '[[+region=ca-central-1]]' +statement error cannot set PARTITION BY on a table in a multi-region enabled database +ALTER TABLE regional_primary_region_table PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) +) + statement ok CREATE TABLE regional_implicit_primary_region_table (a int) LOCALITY REGIONAL BY TABLE @@ -466,6 +471,11 @@ TABLE global_table ALTER TABLE global_table CONFIGURE ZONE USING voter_constraints = '{+region=ca-central-1: 2}', lease_preferences = '[[+region=ca-central-1]]' +statement error cannot set PARTITION BY on a table in a multi-region enabled database +ALTER TABLE global_table PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) +) + query TTTTIT colnames SHOW TABLES ---- diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index f107da0ae40c..71c948f3b6fd 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -488,6 +488,12 @@ CREATE TABLE public.regional_by_row_table ( ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_a_idx CONFIGURE ZONE USING "gc.ttlseconds" = 10 # Prohibit certain actions on a REGIONAL BY ROW table. + +statement error cannot set PARTITION BY on a table in a multi-region enabled database +ALTER TABLE regional_by_row_table PARTITION BY LIST (pk) ( + PARTITION "one" VALUES IN (1) +) + statement error hash sharded indexes are not compatible with REGIONAL BY ROW tables CREATE INDEX bad_idx ON regional_by_row_table(a) USING HASH WITH BUCKET_COUNT = 8 diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 8bdab383de20..69e1bfe8ae08 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -810,6 +810,12 @@ func (n *alterTableNode) startExec(params runParams) error { if t.All { return unimplemented.NewWithIssue(58736, "PARTITION ALL BY not yet implemented") } + if n.tableDesc.GetLocalityConfig() != nil { + return pgerror.Newf( + pgcode.FeatureNotSupported, + "cannot set PARTITION BY on a table in a multi-region enabled database", + ) + } if n.tableDesc.IsPartitionAllBy() { return unimplemented.NewWithIssue(58736, "changing partition of table with PARTITION ALL BY not yet implemented") } From 4298126158a01bcfc474bc00a51f68b05da529b5 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 4 Mar 2021 16:14:47 +1100 Subject: [PATCH 4/6] sql: block ALTER INDEX PARTITION BY for PARTITION BY ALL + multi-region Release justification: low-risk update to new functionality Release note: None --- .../testdata/logic_test/partitioning_implicit | 31 +++++++++++++++---- .../testdata/logic_test/regional_by_row | 5 +++ pkg/sql/alter_index.go | 18 +++++++++-- pkg/sql/alter_table.go | 2 +- 4 files changed, 47 insertions(+), 9 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit b/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit index a0af6848aa8c..4c66365363cf 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit +++ b/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit @@ -293,22 +293,36 @@ CREATE TABLE fk_including_implicit_columns_against_t ( CONSTRAINT a_pk FOREIGN KEY(ref_t_a, ref_t_pk) REFERENCES t(a, pk) ) -statement error cannot ALTER TABLE PARTITION BY on table which already has implicit column partitioning +statement error cannot ALTER TABLE PARTITION BY on a table which already has implicit column partitioning ALTER TABLE t PARTITION BY LIST(d) ( PARTITION pk_implicit VALUES IN (1) ) -statement error cannot ALTER INDEX PARTITION BY on index which already has implicit column partitioning +statement error cannot ALTER INDEX PARTITION BY on an index which already has implicit column partitioning ALTER INDEX t@t_b_idx PARTITION BY LIST(d) ( PARTITION pk_implicit VALUES IN (1) ) -statement error cannot ALTER INDEX PARTITION BY on index which already has implicit column partitioning +statement error cannot ALTER INDEX PARTITION BY on an index which already has implicit column partitioning ALTER INDEX t@t_b_idx PARTITION BY NOTHING -statement error cannot ALTER TABLE PARTITION BY on table which already has implicit column partitioning +statement error cannot ALTER TABLE PARTITION BY on a table which already has implicit column partitioning ALTER TABLE t PARTITION BY NOTHING +statement ok +CREATE TABLE t_implicit_idx ( + id INT PRIMARY KEY, + a INT, + INDEX t_a_idx (a) PARTITION BY LIST (id) ( + PARTITION "one" VALUES IN (1) + ) +) + +statement error cannot ALTER INDEX PARTITION BY on an index which already has implicit column partitioning +ALTER INDEX t_implicit_idx@t_a_idx PARTITION BY LIST (a) ( + PARTITION "one" VALUES IN (1) +) + query TTTTTT colnames SELECT indexrelid, indrelid, indkey, indclass, indoption, indcollation @@ -360,12 +374,12 @@ t t_b_idx CREATE INDEX t_b_idx ON test.public.t USING btree (b ASC t t_c_key CREATE UNIQUE INDEX t_c_key ON test.public.t USING btree (c ASC) t t_j_idx CREATE INDEX t_j_idx ON test.public.t USING gin (j ASC) -statement error cannot ALTER INDEX PARTITION BY on index which already has implicit column partitioning +statement error cannot ALTER INDEX PARTITION BY on an index which already has implicit column partitioning ALTER INDEX new_idx PARTITION BY LIST (a) ( PARTITION d_implicit VALUES IN (1) ) -statement error cannot ALTER TABLE PARTITION BY on table which already has implicit column partitioning +statement error cannot ALTER TABLE PARTITION BY on a table which already has implicit column partitioning ALTER TABLE t PARTITION BY LIST (a) ( PARTITION pk_implicit VALUES IN (1) ) @@ -431,6 +445,11 @@ CREATE INDEX created_idx ON t(c) PARTITION BY LIST (d) ( PARTITION one VALUES IN ((1)) ) +statement error cannot change the partitioning of an index if the table has PARTITION ALL BY defined +ALTER INDEX t@t_a_idx PARTITION BY LIST (a) ( + PARTITION one VALUES IN (1) +) + query T noticetrace CREATE INDEX created_idx ON t(c) ---- diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index 71c948f3b6fd..aa288203e2be 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -494,6 +494,11 @@ ALTER TABLE regional_by_row_table PARTITION BY LIST (pk) ( PARTITION "one" VALUES IN (1) ) +statement error cannot change the partitioning of an index if the table is part of a multi-region database +ALTER INDEX regional_by_row_table@regional_by_row_table_a_idx PARTITION BY LIST (pk2) ( + PARTITION one VALUES IN (1) +) + statement error hash sharded indexes are not compatible with REGIONAL BY ROW tables CREATE INDEX bad_idx ON regional_by_row_table(a) USING HASH WITH BUCKET_COUNT = 8 diff --git a/pkg/sql/alter_index.go b/pkg/sql/alter_index.go index 07ade245f574..51d6343ad94e 100644 --- a/pkg/sql/alter_index.go +++ b/pkg/sql/alter_index.go @@ -16,6 +16,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" @@ -72,10 +74,22 @@ func (n *alterIndexNode) startExec(params runParams) error { switch t := cmd.(type) { case *tree.AlterIndexPartitionBy: telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("index", "partition_by")) - if n.tableDesc.GetPrimaryIndex().GetPartitioning().NumImplicitColumns > 0 { + if n.tableDesc.GetLocalityConfig() != nil { + return pgerror.Newf( + pgcode.FeatureNotSupported, + "cannot change the partitioning of an index if the table is part of a multi-region database", + ) + } + if n.tableDesc.PartitionAllBy { + return pgerror.Newf( + pgcode.FeatureNotSupported, + "cannot change the partitioning of an index if the table has PARTITION ALL BY defined", + ) + } + if n.indexDesc.Partitioning.NumImplicitColumns > 0 { return unimplemented.New( "ALTER INDEX PARTITION BY", - "cannot ALTER INDEX PARTITION BY on index which already has implicit column partitioning", + "cannot ALTER INDEX PARTITION BY on an index which already has implicit column partitioning", ) } allowImplicitPartitioning := params.p.EvalContext().SessionData.ImplicitColumnPartitioningEnabled || diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 69e1bfe8ae08..801258c729a4 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -823,7 +823,7 @@ func (n *alterTableNode) startExec(params runParams) error { if oldPartitioning.NumImplicitColumns > 0 { return unimplemented.NewWithIssue( 58731, - "cannot ALTER TABLE PARTITION BY on table which already has implicit column partitioning", + "cannot ALTER TABLE PARTITION BY on a table which already has implicit column partitioning", ) } newPrimaryIndex, err := CreatePartitioning( From 038260aec40a3c7419d62a3c876a4bb15fb38d6c Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 4 Mar 2021 16:25:40 +1100 Subject: [PATCH 5/6] sql: disallow CREATE INDEX with PARTITION BY on multi-region tables Release justification: low-risk updates for new functionality Release note: None --- pkg/ccl/logictestccl/testdata/logic_test/multi_region | 10 ++++++++++ .../logictestccl/testdata/logic_test/regional_by_row | 5 +++++ pkg/sql/create_index.go | 7 +++++++ 3 files changed, 22 insertions(+) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region b/pkg/ccl/logictestccl/testdata/logic_test/multi_region index e55d9974da73..1de29fdd895f 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region @@ -389,6 +389,11 @@ ALTER TABLE regional_primary_region_table PARTITION BY LIST (id) ( PARTITION "one" VALUES IN (1) ) +statement error cannot define PARTITION BY on a new INDEX in a multi-region database +CREATE INDEX bad_idx ON regional_primary_region_table(a) PARTITION BY LIST (a) ( + PARTITION one VALUES IN (1) +) + statement ok CREATE TABLE regional_implicit_primary_region_table (a int) LOCALITY REGIONAL BY TABLE @@ -476,6 +481,11 @@ ALTER TABLE global_table PARTITION BY LIST (id) ( PARTITION "one" VALUES IN (1) ) +statement error cannot define PARTITION BY on a new INDEX in a multi-region database +CREATE INDEX bad_idx ON global_table(a) PARTITION BY LIST (a) ( + PARTITION one VALUES IN (1) +) + query TTTTIT colnames SHOW TABLES ---- diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index aa288203e2be..0db13935da7a 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -499,6 +499,11 @@ ALTER INDEX regional_by_row_table@regional_by_row_table_a_idx PARTITION BY LIST PARTITION one VALUES IN (1) ) +statement error cannot define PARTITION BY on a new INDEX in a multi-region database +CREATE INDEX bad_idx ON regional_by_row_table(a) PARTITION BY LIST (a) ( + PARTITION one VALUES IN (1) +) + statement error hash sharded indexes are not compatible with REGIONAL BY ROW tables CREATE INDEX bad_idx ON regional_by_row_table(a) USING HASH WITH BUCKET_COUNT = 8 diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go index 24930f9f6922..191aebc65a44 100644 --- a/pkg/sql/create_index.go +++ b/pkg/sql/create_index.go @@ -453,6 +453,13 @@ func (n *createIndexNode) startExec(params runParams) error { } indexDesc.Version = encodingVersion + if n.n.PartitionByIndex != nil && n.tableDesc.GetLocalityConfig() != nil { + return pgerror.New( + pgcode.FeatureNotSupported, + "cannot define PARTITION BY on a new INDEX in a multi-region database", + ) + } + *indexDesc, err = params.p.configureIndexDescForNewIndexPartitioning( params.ctx, n.tableDesc, From a6fd9d853829d639b6a9aaf83625944782f6a31a Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 4 Mar 2021 16:27:20 +1100 Subject: [PATCH 6/6] sql: remove comment on #59719 Release justification: non-production code change Release note: None --- pkg/sql/alter_primary_key.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index 7500f09aef9c..635d2107f553 100644 --- a/pkg/sql/alter_primary_key.go +++ b/pkg/sql/alter_primary_key.go @@ -460,10 +460,6 @@ func (p *planner) AlterPrimaryKey( return err } } - // TODO(#59719): decide what happens if any multiregion tables have - // PARTITION BY statements. We currently do not support this and ignore - // these old statements. - // // Create partitioning if we are newly adding a PARTITION BY ALL statement. if isNewPartitionAllBy { if *newIndex, err = CreatePartitioning(