Merge #127823

127823: sql/schemachanger: Validate ALTER TABLE .. TYPE in the DSC r=fqazi,annrpom a=spilchen When altering the data type of a column, some changes only require validating the existing data. For example, when changing from `CHAR(20)` to `CHAR(10)`, we don't need to rewrite the data; we only need to ensure all existing data fits within `CHAR(10)`. This change implements that validation logic in the declarative schema changer. The validation logic uses a temporary check constraint. This constraint is added transiently so that it never transitions to PUBLIC and is automatically removed. The check constraint casts the column to the new type and then back to the old type. If the value matches its original value after these casts, the row is deemed valid. To add a schema changer test, I disabled the `enable_experimental_alter_column_type_general` setting for validation. It remains enabled for column type alterations requiring a rewrite and for validation-only changes in the legacy schema changer. I updated the dependency rules to allow the check constraint to be added transiently. I think this is fine even for older releases because I'm not introducing new elements or attributes for the check constraint; I'm just allowing a state transition to transient. When validation occurs, it will look something like this: ``` [email protected]:26257/demoapp/movr> ALTER TABLE t1 ALTER COLUMN c1 SET DATA TYPE CHAR(3); ERROR: validation of CHECK "CAST(CAST(c1 AS CHAR(3)) AS CHAR(20)) = c1" failed on row: c1='123456789', rowid=990187907647504385 SQLSTATE: 23514 ``` Epic: CRDB-25314 Closes: #127516 Release note: None Co-authored-by: Matt Spilchen <[email protected]>
cockroachdb · Aug 2, 2024 · b610202 · b610202
2 parents 16bfbb7 + c22dd0a
commit b610202
Show file tree

Hide file tree

Showing 24 changed files with 2,134 additions and 116 deletions.
diff --git a/pkg/ccl/changefeedccl/cdcevent/event_test.go b/pkg/ccl/changefeedccl/cdcevent/event_test.go
@@ -460,36 +460,36 @@ func TestEventColumnOrderingWithSchemaChanges(t *testing.T) {
 			testName:   "main/main_cols",
 			familyName: "main",
 			actions: []string{
-				"INSERT INTO foo (i,j,a,b) VALUES (0,1,'a0','b0')",
-				"ALTER TABLE foo ALTER COLUMN a SET DATA TYPE VARCHAR(16)",
-				"INSERT INTO foo (i,j,a,b) VALUES (1,2,'a1','b1')",
+				"INSERT INTO foo (i,j,a,b) VALUES (0,1,'2002-05-02','b0')",
+				"ALTER TABLE foo ALTER COLUMN a SET DATA TYPE DATE USING a::DATE",
+				"INSERT INTO foo (i,j,a,b) VALUES (1,2,'2021-01-01','b1')",
 			},
 			expectMainFamily: []decodeExpectation{
 				{
 					keyValues: []string{"1", "0"},
-					allValues: []string{"0", "1", "a0", "b0"},
+					allValues: []string{"0", "1", "2002-05-02", "b0"},
 				},
 				{
 					keyValues: []string{"1", "0"},
-					allValues: []string{"0", "1", "a0", "b0"},
+					allValues: []string{"0", "1", "2002-05-02", "b0"},
 				},
 				{
 					keyValues: []string{"1", "0"},
-					allValues: []string{"0", "1", "a0", "b0"},
+					allValues: []string{"0", "1", "2002-05-02", "b0"},
 				},
 				{
 					keyValues: []string{"2", "1"},
-					allValues: []string{"1", "2", "a1", "b1"},
+					allValues: []string{"1", "2", "2021-01-01", "b1"},
 				},
 			},
 		},
 		{
 			testName:   "ec/ec_cols",
 			familyName: "ec",
 			actions: []string{
-				"INSERT INTO foo (i,j,e,c) VALUES (2,3,'e2','c2')",
-				"ALTER TABLE foo ALTER COLUMN c SET DATA TYPE VARCHAR(16)",
-				"INSERT INTO foo (i,j,e,c) VALUES (3,4,'e3','c3')",
+				"INSERT INTO foo (i,j,e,c) VALUES (2,3,'e2','2024-08-02')",
+				"ALTER TABLE foo ALTER COLUMN c SET DATA TYPE DATE USING c::DATE",
+				"INSERT INTO foo (i,j,e,c) VALUES (3,4,'e3','2024-05-21')",
 			},
 			expectMainFamily: []decodeExpectation{
 				{
@@ -502,29 +502,29 @@ func TestEventColumnOrderingWithSchemaChanges(t *testing.T) {
 			expectECFamily: []decodeExpectation{
 				{
 					keyValues: []string{"3", "2"},
-					allValues: []string{"c2", "e2"},
+					allValues: []string{"2024-08-02", "e2"},
 				},
 				{
 					keyValues: []string{"3", "2"},
-					allValues: []string{"c2", "e2"},
+					allValues: []string{"2024-08-02", "e2"},
 				},
 				{
 					keyValues: []string{"3", "2"},
-					allValues: []string{"c2", "e2"},
+					allValues: []string{"2024-08-02", "e2"},
 				},
 				{
 					keyValues: []string{"4", "3"},
-					allValues: []string{"c3", "e3"},
+					allValues: []string{"2024-05-21", "e3"},
 				},
 			},
 		},
 		{
 			testName:   "ec/ec_cols_with_virtual",
 			familyName: "ec",
 			actions: []string{
-				"INSERT INTO foo (i,j,e,c) VALUES (4,5,'e4','c4')",
-				"ALTER TABLE foo ALTER COLUMN c SET DATA TYPE VARCHAR(16)",
-				"INSERT INTO foo (i,j,e,c) VALUES (5,6,'e5','c5')",
+				"INSERT INTO foo (i,j,e,c) VALUES (4,5,'e4','2012-11-06')",
+				"ALTER TABLE foo ALTER COLUMN c SET DATA TYPE DATE USING c::DATE",
+				"INSERT INTO foo (i,j,e,c) VALUES (5,6,'e5','2014-05-06')",
 			},
 			includeVirtual: true,
 			expectMainFamily: []decodeExpectation{
@@ -538,20 +538,20 @@ func TestEventColumnOrderingWithSchemaChanges(t *testing.T) {
 			expectECFamily: []decodeExpectation{
 				{
 					keyValues: []string{"5", "4"},
-					allValues: []string{"c4", "NULL", "e4"},
+					allValues: []string{"2012-11-06", "NULL", "e4"},
 				},
 				{
 					keyValues:         []string{"5", "4"},
-					allValues:         []string{"c4", "NULL", "e4"},
+					allValues:         []string{"2012-11-06", "NULL", "e4"},
 					refreshDescriptor: true,
 				},
 				{
 					keyValues: []string{"5", "4"},
-					allValues: []string{"c4", "NULL", "e4"},
+					allValues: []string{"2012-11-06", "NULL", "e4"},
 				},
 				{
 					keyValues: []string{"6", "5"},
-					allValues: []string{"c5", "NULL", "e5"},
+					allValues: []string{"2014-05-06", "NULL", "e5"},
 				},
 			},
 		},
@@ -619,8 +619,8 @@ func TestEventColumnOrderingWithSchemaChanges(t *testing.T) {
 				require.NoError(t, err)
 				require.True(t, updatedRow.IsInitialized())
 
-				require.Equal(t, expect.keyValues, slurpDatums(t, updatedRow.ForEachKeyColumn()))
-				require.Equal(t, expect.allValues, slurpDatums(t, updatedRow.ForEachColumn()))
+				require.Equal(t, expect.keyValues, slurpDatums(t, updatedRow.ForEachKeyColumn()), "row %d", i)
+				require.Equal(t, expect.allValues, slurpDatums(t, updatedRow.ForEachColumn()), "row %d", i)
 			}
 			sqlDB.Exec(t, `DROP TABLE foo`)
 		})

diff --git a/pkg/ccl/schemachangerccl/backup_base_generated_test.go b/pkg/ccl/schemachangerccl/backup_base_generated_test.go
diff --git a/pkg/cli/testdata/declarative-rules/deprules b/pkg/cli/testdata/declarative-rules/deprules
@@ -24,6 +24,69 @@ deprules
     - descriptorIsDataNotBeingAdded-24.1($descID)
     - joinTargetNode($prev, $prev-Target, $prev-Node)
     - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: TRANSIENT_ABSENT->ABSENT'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = ABSENT
+    - $prev-Node[CurrentStatus] = TRANSIENT_ABSENT
+    - $next-Node[CurrentStatus] = ABSENT
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: TRANSIENT_VALIDATED->VALIDATED'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = ABSENT
+    - $prev-Node[CurrentStatus] = TRANSIENT_VALIDATED
+    - $next-Node[CurrentStatus] = VALIDATED
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: TRANSIENT_WRITE_ONLY->VALIDATED'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = ABSENT
+    - $prev-Node[CurrentStatus] = TRANSIENT_WRITE_ONLY
+    - $next-Node[CurrentStatus] = VALIDATED
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
 - name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: VALIDATED->ABSENT'
   from: prev-Node
   kind: PreviousTransactionPrecedence
@@ -43,7 +106,7 @@ deprules
     - $descriptor-data-Node[CurrentStatus] = PUBLIC
     - $descriptor-data[DescID] = $descID
     - descriptorIsDataNotBeingAdded-24.1($descID)
-    - nodeNotExistsWithStatusIn_WRITE_ONLY($prev-Target)
+    - nodeNotExistsWithStatusIn_TRANSIENT_VALIDATED_WRITE_ONLY_TRANSIENT_WRITE_ONLY($prev-Target)
     - joinTargetNode($prev, $prev-Target, $prev-Node)
     - joinTargetNode($next, $next-Target, $next-Node)
 - name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: WRITE_ONLY->VALIDATED'
@@ -130,6 +193,133 @@ deprules
     - descriptorIsDataNotBeingAdded-24.1($descID)
     - joinTargetNode($prev, $prev-Target, $prev-Node)
     - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to TRANSIENT_ABSENT uphold 2-version invariant: ABSENT->WRITE_ONLY'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = TRANSIENT_ABSENT
+    - $prev-Node[CurrentStatus] = ABSENT
+    - $next-Node[CurrentStatus] = WRITE_ONLY
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to TRANSIENT_ABSENT uphold 2-version invariant: PUBLIC->TRANSIENT_VALIDATED'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = TRANSIENT_ABSENT
+    - $prev-Node[CurrentStatus] = PUBLIC
+    - $next-Node[CurrentStatus] = TRANSIENT_VALIDATED
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to TRANSIENT_ABSENT uphold 2-version invariant: TRANSIENT_VALIDATED->TRANSIENT_ABSENT'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = TRANSIENT_ABSENT
+    - $prev-Node[CurrentStatus] = TRANSIENT_VALIDATED
+    - $next-Node[CurrentStatus] = TRANSIENT_ABSENT
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - nodeNotExistsWithStatusIn_TRANSIENT_WRITE_ONLY($prev-Target)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to TRANSIENT_ABSENT uphold 2-version invariant: TRANSIENT_WRITE_ONLY->TRANSIENT_VALIDATED'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = TRANSIENT_ABSENT
+    - $prev-Node[CurrentStatus] = TRANSIENT_WRITE_ONLY
+    - $next-Node[CurrentStatus] = TRANSIENT_VALIDATED
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to TRANSIENT_ABSENT uphold 2-version invariant: VALIDATED->PUBLIC'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = TRANSIENT_ABSENT
+    - $prev-Node[CurrentStatus] = VALIDATED
+    - $next-Node[CurrentStatus] = PUBLIC
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
+- name: 'CheckConstraint transitions to TRANSIENT_ABSENT uphold 2-version invariant: WRITE_ONLY->VALIDATED'
+  from: prev-Node
+  kind: PreviousTransactionPrecedence
+  to: next-Node
+  query:
+    - $prev[Type] = '*scpb.CheckConstraint'
+    - $next[Type] = '*scpb.CheckConstraint'
+    - $prev[DescID] = $descID
+    - $prev[Self] = $next
+    - $prev-Target[Self] = $next-Target
+    - $prev-Target[TargetStatus] = TRANSIENT_ABSENT
+    - $prev-Node[CurrentStatus] = WRITE_ONLY
+    - $next-Node[CurrentStatus] = VALIDATED
+    - descriptorIsNotBeingDropped-24.1($prev)
+    - $descriptor-data[Type] = '*scpb.TableData'
+    - joinTargetNode($descriptor-data, $descriptor-data-Target, $descriptor-data-Node)
+    - $descriptor-data-Node[CurrentStatus] = PUBLIC
+    - $descriptor-data[DescID] = $descID
+    - descriptorIsDataNotBeingAdded-24.1($descID)
+    - joinTargetNode($prev, $prev-Target, $prev-Node)
+    - joinTargetNode($next, $next-Target, $next-Node)
 - name: 'Column transitions to ABSENT uphold 2-version invariant: DELETE_ONLY->ABSENT'
   from: prev-Node
   kind: PreviousTransactionPrecedence

diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go
@@ -970,7 +970,7 @@ func ResolveFK(
 		if s, t := originCols[i], referencedCols[i]; !s.GetType().Identical(t.GetType()) {
 			notice := pgnotice.Newf(
 				"type of foreign key column %q (%s) is not identical to referenced column %q.%q (%s)",
-				s.ColName(), s.GetType().String(), target.Name, t.GetName(), t.GetType().String())
+				s.ColName(), s.GetType().SQLString(), target.Name, t.GetName(), t.GetType().SQLString())
 			evalCtx.ClientNoticeSender.BufferClientNotice(ctx, notice)
 		}
 	}