Merge cockroachdb#125282

125282: scbuild: use proper checks when creating a virtual computed column r=Dedej-Bergin a=Dedej-Bergin This bug only existed in the declarative schema changer due to the declarative schema changer code not running checks with ReplaceColumnVars() and SanitizeVarFreeExp() like in the legacy schema changer. The fix was to add a call to b.ComputedColumnExpression() which calls schemaexpr.ValidateComputedColumnExpression which calls DequalifyAndValidateExpr which calls DequalifyAndValidateExprImpl, and that calls both ReplaceColumnVars and SanitizeVarFreeExpr. Fixes: cockroachdb#124546 Release note (bug fix): ALTER TABLE ... ADD CONSTRAINT UNIQUE will now fail with a well-formed error message and code 42601 if a statement tries to add a unique constraint on an expression. Co-authored-by: Bergin Dedej <[email protected]>
lyang24 · Jun 7, 2024 · 6032d53 · 6032d53
2 parents 626166f + f26cc49
commit 6032d53
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 40 deletions.
diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table
@@ -3957,3 +3957,14 @@ CREATE TABLE public.t_114316 (
 )
 
 subtest end
+
+subtest regression_test_alter_table_add_constraint_unique
+
+statement ok
+create table t_124546(a int);
+
+# Regression Test for https://github.com/cockroachdb/cockroach/issues/124546
+statement error pgcode 42601 variable sub-expressions are not allowed in EXPRESSION INDEX ELEMENT
+ALTER TABLE t_124546 ADD CONSTRAINT ident UNIQUE ( ( EXISTS ( TABLE error FOR READ ONLY ) ) DESC ) STORING ( ident , ident );
+
+subtest end
diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go
@@ -873,32 +873,7 @@ func maybeCreateVirtualColumnForIndex(
 	d.Nullable.Nullability = tree.Null
 	// Infer column type from expression.
 	{
-		colLookupFn := func(columnName tree.Name) (exists bool, accessible bool, id catid.ColumnID, typ *types.T) {
-			scpb.ForEachColumnName(elts, func(_ scpb.Status, target scpb.TargetStatus, cn *scpb.ColumnName) {
-				if target == scpb.ToPublic && tree.Name(cn.Name) == columnName {
-					id = cn.ColumnID
-				}
-			})
-			if id == 0 {
-				return false, false, 0, nil
-			}
-			scpb.ForEachColumn(elts, func(_ scpb.Status, target scpb.TargetStatus, col *scpb.Column) {
-				if target == scpb.ToPublic && col.ColumnID == id {
-					exists = true
-					accessible = !col.IsInaccessible
-				}
-			})
-			scpb.ForEachColumnType(elts, func(_ scpb.Status, target scpb.TargetStatus, col *scpb.ColumnType) {
-				if target == scpb.ToPublic && col.ColumnID == id {
-					typ = col.Type
-				}
-			})
-			return exists, accessible, id, typ
-		}
-		replacedExpr, _, err := schemaexpr.ReplaceColumnVars(expr, colLookupFn)
-		if err != nil {
-			panic(err)
-		}
+		replacedExpr := b.ComputedColumnExpression(tbl, d)
 		typedExpr, err := tree.TypeCheck(b, replacedExpr, b.SemaCtx(), types.Any)
 		if err != nil {
 			panic(err)

diff --git a/..._end/drop_index_partial_expression_index/drop_index_partial_expression_index.side_effects b/..._end/drop_index_partial_expression_index/drop_index_partial_expression_index.side_effects
@@ -60,7 +60,7 @@ upsert descriptor #104
   -    partitioning: {}
   -    predicate: i > 0:::INT8
   -    sharded: {}
-  -    version: 4
+  -    version: 3
   +  indexes: []
      modificationTime: {}
   +  mutations:
@@ -84,8 +84,8 @@ upsert descriptor #104
   +      partitioning: {}
   +      predicate: i > 0:::INT8
   +      sharded: {}
-  +      version: 4
-  +    mutationId: 1
+  +      version: 3
+  +    mutationId: 2
   +    state: WRITE_ONLY
   +  - column:
   +      computeExpr: lower(j)
@@ -98,7 +98,7 @@ upsert descriptor #104
   +        oid: 25
   +      virtual: true
   +    direction: DROP
-  +    mutationId: 1
+  +    mutationId: 2
   +    state: WRITE_ONLY
      name: t
      nextColumnId: 4
@@ -176,7 +176,7 @@ upsert descriptor #104
   -    partitioning: {}
   -    predicate: i > 0:::INT8
   -    sharded: {}
-  -    version: 4
+  -    version: 3
   +  indexes: []
      modificationTime: {}
   +  mutations:
@@ -200,8 +200,8 @@ upsert descriptor #104
   +      partitioning: {}
   +      predicate: i > 0:::INT8
   +      sharded: {}
-  +      version: 4
-  +    mutationId: 1
+  +      version: 3
+  +    mutationId: 2
   +    state: WRITE_ONLY
   +  - column:
   +      computeExpr: lower(j)
@@ -214,7 +214,7 @@ upsert descriptor #104
   +        oid: 25
   +      virtual: true
   +    direction: DROP
-  +    mutationId: 1
+  +    mutationId: 2
   +    state: WRITE_ONLY
      name: t
      nextColumnId: 4
@@ -243,15 +243,15 @@ upsert descriptor #104
          partitioning: {}
   -      predicate: i > 0:::INT8
          sharded: {}
-         version: 4
-       mutationId: 1
+         version: 3
+       mutationId: 2
   -    state: WRITE_ONLY
   +    state: DELETE_ONLY
      - column:
          computeExpr: lower(j)
   ...
        direction: DROP
-       mutationId: 1
+       mutationId: 2
   -    state: WRITE_ONLY
   +    state: DELETE_ONLY
      name: t
@@ -319,8 +319,8 @@ upsert descriptor #104
   -      name: crdb_internal_index_2_name_placeholder
   -      partitioning: {}
   -      sharded: {}
-  -      version: 4
-  -    mutationId: 1
+  -      version: 3
+  -    mutationId: 2
   -    state: DELETE_ONLY
   -  - column:
   -      computeExpr: lower(j)
@@ -333,7 +333,7 @@ upsert descriptor #104
   -        oid: 25
   -      virtual: true
   -    direction: DROP
-  -    mutationId: 1
+  -    mutationId: 2
   -    state: DELETE_ONLY
   +  mutations: []
      name: t