Merge #133131 #133845

133131: workload/schemachange: setColumnType operations will occasionally fail r=Dedej-Bergin a=Dedej-Bergin Previously this test would fail due to 0A000 and 2BP01 even though those failures are expected. In these code changes we properly handle these errors. Epic: CRDB-25314 Fixes: #66662 Release note: None 133845: sql/schemachanger: Preserving column family order for complex column type changes r=spilchen a=spilchen When altering a column’s type in a way that requires a backfill, we drop the old column and add a new one. Previously, the new column was always added to the end of the column family. This change ensures the column family order is preserved. In the DSC, the column family is updated when a new ColumnType element is added. I introduced a new field to this element to control the order, which requires specifying the column ID that the new column should follow. Epic: CRDB-25314 Closes #133040 Release note: none Co-authored-by: Bergin Dedej <bergin.dedej@cockroachlabs.com> Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
cockroachdb · Nov 1, 2024 · d04294b · d04294b
3 parents 3105f1f + 3ec0074 + 5059f89
commit d04294b
Show file tree

Hide file tree

Showing 17 changed files with 222 additions and 14 deletions.
diff --git a/pkg/ccl/schemachangerccl/testdata/decomp/multiregion b/pkg/ccl/schemachangerccl/testdata/decomp/multiregion
@@ -232,6 +232,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 1
     computeExpr: null
     elementCreationMetadata:
@@ -261,6 +262,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 2
     computeExpr: null
     elementCreationMetadata:
@@ -290,6 +292,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967292e+09
     computeExpr: null
     elementCreationMetadata:
@@ -319,6 +322,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967293e+09
     computeExpr: null
     elementCreationMetadata:
@@ -348,6 +352,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967294e+09
     computeExpr: null
     elementCreationMetadata:
@@ -377,6 +382,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967295e+09
     computeExpr: null
     elementCreationMetadata:
@@ -627,6 +633,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 1
     computeExpr: null
     elementCreationMetadata:
@@ -656,6 +663,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 2
     computeExpr: null
     elementCreationMetadata:
@@ -685,6 +693,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967292e+09
     computeExpr: null
     elementCreationMetadata:
@@ -714,6 +723,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967293e+09
     computeExpr: null
     elementCreationMetadata:
@@ -743,6 +753,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967294e+09
     computeExpr: null
     elementCreationMetadata:
@@ -772,6 +783,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967295e+09
     computeExpr: null
     elementCreationMetadata:
@@ -1064,6 +1076,7 @@ ElementState:
     closedTypeIds:
     - 106
     - 107
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 3
     computeExpr: null
     elementCreationMetadata:
@@ -1094,6 +1107,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 1
     computeExpr: null
     elementCreationMetadata:
@@ -1123,6 +1137,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 2
     computeExpr: null
     elementCreationMetadata:
@@ -1152,6 +1167,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967292e+09
     computeExpr: null
     elementCreationMetadata:
@@ -1181,6 +1197,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967293e+09
     computeExpr: null
     elementCreationMetadata:
@@ -1210,6 +1227,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967294e+09
     computeExpr: null
     elementCreationMetadata:
@@ -1239,6 +1257,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967295e+09
     computeExpr: null
     elementCreationMetadata:

diff --git a/pkg/ccl/schemachangerccl/testdata/decomp/partitioning b/pkg/ccl/schemachangerccl/testdata/decomp/partitioning
@@ -164,6 +164,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 1
     computeExpr: null
     elementCreationMetadata:
@@ -193,6 +194,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 2
     computeExpr: null
     elementCreationMetadata:
@@ -222,6 +224,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 3
     computeExpr: null
     elementCreationMetadata:
@@ -251,6 +254,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967292e+09
     computeExpr: null
     elementCreationMetadata:
@@ -280,6 +284,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967293e+09
     computeExpr: null
     elementCreationMetadata:
@@ -309,6 +314,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967294e+09
     computeExpr: null
     elementCreationMetadata:
@@ -338,6 +344,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967295e+09
     computeExpr: null
     elementCreationMetadata:
@@ -666,6 +673,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 1
     computeExpr: null
     elementCreationMetadata:
@@ -695,6 +703,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 2
     computeExpr: null
     elementCreationMetadata:
@@ -724,6 +733,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967292e+09
     computeExpr: null
     elementCreationMetadata:
@@ -753,6 +763,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967293e+09
     computeExpr: null
     elementCreationMetadata:
@@ -782,6 +793,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967294e+09
     computeExpr: null
     elementCreationMetadata:
@@ -811,6 +823,7 @@ ElementState:
   Status: PUBLIC
 - ColumnType:
     closedTypeIds: []
+    columnFamilyOrderFollowsColumnId: 0
     columnId: 4.294967295e+09
     computeExpr: null
     elementCreationMetadata:

diff --git a/pkg/sql/logictest/testdata/logic_test/alter_column_type b/pkg/sql/logictest/testdata/logic_test/alter_column_type
@@ -205,7 +205,7 @@ INSERT INTO t2 VALUES ('5')
 # Verify ALTER COLUMN TYPE from INT to STRING works correctly.
 # Column order should stay the same.
 statement ok
-CREATE TABLE t3 (id int, id2 int, id3 int)
+CREATE TABLE t3 (id int, id2 int, id3 int, family f1 (id3, id2, id))
 
 statement ok
 INSERT INTO t3 VALUES (1,1,1), (2,2,2), (3,3,3)
@@ -221,6 +221,19 @@ id2    STRING  true   NULL            ·  {t3_pkey}  false
 id3    INT8    true   NULL            ·  {t3_pkey}  false
 rowid  INT8    false  unique_rowid()  ·  {t3_pkey}  true
 
+query TT colnames
+show create table t3
+----
+table_name  create_statement
+t3          CREATE TABLE public.t3 (
+              id INT8 NULL,
+              id2 STRING NULL,
+              id3 INT8 NULL,
+              rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
+              CONSTRAINT t3_pkey PRIMARY KEY (rowid ASC),
+              FAMILY f1 (id3, id2, id, rowid)
+            )
+
 statement ok
 INSERT INTO t3 VALUES (4,'4',4)
 
@@ -985,6 +998,19 @@ c2     CHAR(4) true   NULL            ·  {t_bytes_pkey}  false
 c3     UUID    true   NULL            ·  {t_bytes_pkey}  false
 rowid  INT8    false  unique_rowid()  ·  {t_bytes_pkey}  true
 
+query TT colnames
+show create table t_bytes
+----
+table_name  create_statement
+t_bytes     CREATE TABLE public.t_bytes (
+              c1 STRING NULL,
+              c2 CHAR(4) NULL,
+              c3 UUID NULL,
+              rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
+              CONSTRAINT t_bytes_pkey PRIMARY KEY (rowid ASC),
+              FAMILY f1 (c1, c2, c3, rowid)
+            )
+
 statement ok
 DROP TABLE t_bytes;
 
@@ -1048,6 +1074,18 @@ c1     DECIMAL(7,2)  true   NULL            ·  {t_decimal_pkey}  false
 c2     DECIMAL(10,2) true   NULL            ·  {t_decimal_pkey}  false
 rowid  INT8          false  unique_rowid()  ·  {t_decimal_pkey}  true
 
+query TT colnames
+show create table t_decimal
+----
+table_name  create_statement
+t_decimal   CREATE TABLE public.t_decimal (
+              c1 DECIMAL(7,2) NULL,
+              c2 DECIMAL(10,2) NULL,
+              rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
+              CONSTRAINT t_decimal_pkey PRIMARY KEY (rowid ASC),
+              FAMILY f1 (c1, c2, rowid)
+            )
+
 statement ok
 DROP TABLE t_decimal;
 

diff --git a/pkg/sql/schemachanger/dml_injection_test.go b/pkg/sql/schemachanger/dml_injection_test.go
@@ -170,10 +170,23 @@ func TestAlterTableDMLInjection(t *testing.T) {
 			expectedErr:  "cannot evaluate scalar expressions containing sequence operations in this context",
 		},
 		{
-			desc:         "alter column type",
+			desc:         "alter column type trivial",
 			setup:        []string{"ALTER TABLE tbl ADD COLUMN new_col SMALLINT NOT NULL DEFAULT 100"},
 			schemaChange: "ALTER TABLE tbl ALTER COLUMN new_col SET DATA TYPE BIGINT",
 		},
+		{
+			desc:         "alter column type validate",
+			setup:        []string{"ALTER TABLE tbl ADD COLUMN new_col BIGINT NOT NULL DEFAULT 100"},
+			schemaChange: "ALTER TABLE tbl ALTER COLUMN new_col SET DATA TYPE SMALLINT",
+		},
+		{
+			desc: "alter column type general",
+			setup: []string{
+				"SET enable_experimental_alter_column_type_general=TRUE",
+				"ALTER TABLE tbl ADD COLUMN new_col BIGINT",
+			},
+			schemaChange: "ALTER TABLE tbl ALTER COLUMN new_col SET DATA TYPE TEXT",
+		},
 		{
 			desc:         "add column default udf",
 			setup:        []string{"CREATE FUNCTION f() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$"},

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go
@@ -304,6 +304,11 @@ func handleGeneralColumnConversion(
 	b.Drop(oldColType)
 	handleDropColumnPrimaryIndexes(b, tbl, col)
 
+	// The new column is replacing an existing one, so we want to insert it into the
+	// column family at the same position as the old column. Normally, when adding a new
+	// column, it is appended to the end of the column family.
+	newColType.ColumnFamilyOrderFollowsColumnID = oldColType.ColumnID
+
 	// Add the spec for the new column. It will be identical to the column it is replacing,
 	// except the type will differ, and it will have a transient computed expression.
 	// This expression will reference the original column to facilitate the backfill.
@@ -331,11 +336,8 @@ func handleGeneralColumnConversion(
 		},
 		transientCompute: true,
 		notNull:          retrieveColumnNotNull(b, tbl.TableID, col.ColumnID) != nil,
-		// TODO(#133040): The new column will be placed in the same column family as the
-		// one it's replacing, so there's no need to specify a family. However, the new
-		// column will be added to the end of the family's column ID list, which changes
-		// its internal ordering. This needs to be revisited as CDC may have a dependency
-		// on the same ordering (see TestEventColumnOrderingWithSchemaChanges).
+		// The new column will be placed in the same column family as the one
+		// it's replacing, so there's no need to specify a family.
 		fam: nil,
 	}
 	addColumn(b, spec, t)

diff --git a/pkg/sql/schemachanger/scbuild/testdata/alter_table_alter_column_type b/pkg/sql/schemachanger/scbuild/testdata/alter_table_alter_column_type
@@ -96,7 +96,7 @@ ALTER TABLE t ALTER COLUMN c2 SET DATA TYPE BIGINT USING c2::BIGINT
 - [[ColumnName:{DescID: 104, Name: c2, ColumnID: 4}, PUBLIC], ABSENT]
   {columnId: 4, name: c2, tableId: 104}
 - [[ColumnType:{DescID: 104, ColumnFamilyID: 0, ColumnID: 4, TypeName: INT8}, PUBLIC], ABSENT]
-  {columnId: 4, elementCreationMetadata: {in231OrLater: true, in243OrLater: true}, isNullable: true, tableId: 104, type: {family: IntFamily, oid: 20, width: 64}, typeName: INT8}
+  {columnFamilyOrderFollowsColumnId: 2, columnId: 4, elementCreationMetadata: {in231OrLater: true, in243OrLater: true}, isNullable: true, tableId: 104, type: {family: IntFamily, oid: 20, width: 64}, typeName: INT8}
 - [[ColumnComputeExpression:{DescID: 104, ColumnID: 4}, TRANSIENT_ABSENT], ABSENT]
   {columnId: 4, expr: 'c2::INT8', referencedColumnIds: [2], tableId: 104}
 - [[IndexColumn:{DescID: 104, ColumnID: 4, IndexID: 2}, TRANSIENT_ABSENT], ABSENT]