schemachanger: implement ALTER TABLE .. VALIDATE CONSTRAINT ..

pkg/sql/alter_table.go

@@ -504,8 +504,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 				if t.IfExists {
 					continue
 				}
-				return pgerror.Newf(pgcode.UndefinedObject,
-					"constraint %q of relation %q does not exist", t.Constraint, n.tableDesc.Name)
+				return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name)
 			}
 			if err := n.tableDesc.DropConstraint(c, func(backRef catalog.ForeignKeyConstraint) error {
 				return params.p.removeFKBackReference(params.ctx, n.tableDesc, backRef.ForeignKeyDesc())
@@ -521,8 +520,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 			name := string(t.Constraint)
 			c := catalog.FindConstraintByName(n.tableDesc, name)
 			if c == nil {
-				return pgerror.Newf(pgcode.UndefinedObject,
-					"constraint %q of relation %q does not exist", t.Constraint, n.tableDesc.Name)
+				return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name)
 			}
 			switch c.GetConstraintValidity() {
 			case descpb.ConstraintValidity_Validated:
@@ -532,8 +530,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
 					"constraint %q in the middle of being added, try again later", t.Constraint)
 			case descpb.ConstraintValidity_Dropping:
-				return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
-					"constraint %q in the middle of being dropped", t.Constraint)
+				return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name)
 			}
 			if ck := c.AsCheck(); ck != nil {
 				if err := validateCheckInTxn(
@@ -745,8 +742,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 		case *tree.AlterTableRenameConstraint:
 			constraint := catalog.FindConstraintByName(n.tableDesc, string(t.Constraint))
 			if constraint == nil {
-				return pgerror.Newf(pgcode.UndefinedObject,
-					"constraint %q of relation %q does not exist", tree.ErrString(&t.Constraint), n.tableDesc.Name)
+				return sqlerrors.NewUndefinedConstraintError(tree.ErrString(&t.Constraint), n.tableDesc.Name)
 			}
 			if t.Constraint == t.NewName {
 				// Nothing to do.

pkg/sql/backfill.go

@@ -2667,10 +2667,9 @@ func getTargetTablesAndFk(
 	if srcTable.Version > srcTable.ClusterVersion().Version {
 		syntheticTable = srcTable
 	}
-	for i := range srcTable.OutboundFKs {
-		def := &srcTable.OutboundFKs[i]
-		if def.Name == fkName {
-			fk = def
+	for _, outbounfFK := range srcTable.OutboundForeignKeys() {
+		if outbounfFK.GetName() == fkName {
+			fk = outbounfFK.ForeignKeyDesc()
 			break
 		}
 	}
@@ -2741,10 +2740,9 @@ func validateUniqueWithoutIndexConstraintInTxn(
 		syntheticDescs = append(syntheticDescs, tableDesc)
 	}
 	var uc *descpb.UniqueWithoutIndexConstraint
-	for i := range tableDesc.UniqueWithoutIndexConstraints {
-		def := &tableDesc.UniqueWithoutIndexConstraints[i]
-		if def.Name == constraintName {
-			uc = def
+	for _, uwi := range tableDesc.UniqueConstraintsWithoutIndex() {
+		if uwi.GetName() == constraintName {
+			uc = uwi.UniqueWithoutIndexDesc()
 			break
 		}
 	}

pkg/sql/catalog/tabledesc/structured.go

@@ -1391,9 +1391,19 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error {
 							break
 						}
 					}
-				case descpb.ConstraintValidity_Unvalidated:
+				case descpb.ConstraintValidity_Unvalidated, descpb.ConstraintValidity_Validated:
 					// add the constraint to the list of check constraints on the table
 					// descriptor.
+					//
+					// The "validated" validity seems strange -- why would we ever complete
+					// a constraint mutation whose validity is already "validated"?
+					// This is because of how legacy schema changer is implemented and will
+					// occur for the following case:
+					// `ALTER TABLE .. ADD CONSTRAINT .. NOT VALID, VALIDATE CONSTRAINT ..`
+					// where the constraint mutation's validity is changed by `VALIDATE CONSTRAINT`
+					// to "validated", and in the job of `ADD CONSTRAINT .. NOT VALID` we
+					// came to mark the constraint mutation as completed.
+					// The same is true for FK and UWI constraints.
 					desc.Checks = append(desc.Checks, &t.Constraint.Check)
 				default:
 					return errors.AssertionFailedf("invalid constraint validity state: %d", t.Constraint.Check.Validity)
@@ -1409,7 +1419,7 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error {
 							break
 						}
 					}
-				case descpb.ConstraintValidity_Unvalidated:
+				case descpb.ConstraintValidity_Unvalidated, descpb.ConstraintValidity_Validated:
 					// Takes care of adding the Foreign Key to the table index. Adding the
 					// backreference to the referenced table index must be taken care of
 					// in another call.
@@ -1427,7 +1437,7 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error {
 							break
 						}
 					}
-				case descpb.ConstraintValidity_Unvalidated:
+				case descpb.ConstraintValidity_Unvalidated, descpb.ConstraintValidity_Validated:
 					// add the constraint to the list of unique without index constraints
 					// on the table descriptor.
 					desc.UniqueWithoutIndexConstraints = append(

pkg/sql/comment_on_constraint.go

@@ -15,10 +15,9 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
-	"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/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 )
 
@@ -60,8 +59,7 @@ func (n *commentOnConstraintNode) startExec(params runParams) error {
 	constraintName := string(n.n.Constraint)
 	constraint := catalog.FindConstraintByName(n.tableDesc, constraintName)
 	if constraint == nil {
-		return pgerror.Newf(pgcode.UndefinedObject,
-			"constraint %q of relation %q does not exist", constraintName, n.tableDesc.GetName())
+		return sqlerrors.NewUndefinedConstraintError(constraintName, n.tableDesc.GetName())
 	}
 
 	var err error

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -1571,7 +1571,7 @@ ALTER TABLE unique_without_index ADD CONSTRAINT my_unique_e UNIQUE WITHOUT INDEX
 ALTER TABLE unique_without_index ADD CONSTRAINT my_unique_e2 UNIQUE WITHOUT INDEX (e) NOT VALID
 
 # Trying to validate one of the constraints will fail.
-statement error pgcode 23505 pq: could not create unique constraint "my_unique_e"\nDETAIL: Key \(e\)=\(1\) is duplicated\.
+statement error pgcode 23505 pq: could not create unique constraint ".*"\nDETAIL: Key \(e\)=\(1\) is duplicated\.
 ALTER TABLE unique_without_index VALIDATE CONSTRAINT my_unique_e
 
 # But after we delete a row, validation should succeed.
@@ -1632,7 +1632,7 @@ statement ok
 ALTER TABLE unique_without_index_partial ADD CONSTRAINT uniq_a_1 UNIQUE WITHOUT INDEX (a) WHERE b > 0 OR c > 0 NOT VALID
 
 # Trying to validate the constraint will fail.
-statement error pgcode 23505 pq: could not create unique constraint "uniq_a_1"\nDETAIL: Key \(a\)=\(1\) is duplicated\.
+statement error pgcode 23505 pq: could not create unique constraint ".*"\nDETAIL: Key \(a\)=\(1\) is duplicated\.
 ALTER TABLE unique_without_index_partial VALIDATE CONSTRAINT uniq_a_1
 
 # But after we delete a row, validation should succeed.
@@ -2903,3 +2903,40 @@ t_96115     t_96115_pkey     PRIMARY KEY      true
 
 statement ok
 DROP TABLE t_96115;
+
+# This subtest tests the behavior of adding/dropping a constraint and validating
+# the constraint in one ALTER TABLE statement.
+
+subtest 96648
+
+statement ok
+CREATE TABLE t_96648 (i INT PRIMARY KEY);
+
+statement error pq: constraint "check_i" in the middle of being added, try again later
+ALTER TABLE t_96648 ADD CONSTRAINT check_i CHECK (i > 0), VALIDATE CONSTRAINT check_i;
+
+statement ok
+ALTER TABLE t_96648 ADD CONSTRAINT check_i CHECK (i > 0);
+
+# Ensure check "check_i" is successfully added.
+statement error pq: failed to satisfy CHECK constraint \(i > 0:::INT8\)
+INSERT INTO t_96648 VALUES (0);
+
+statement ok
+ALTER TABLE t_96648 ADD CONSTRAINT check_i1 CHECK (i > 10) NOT VALID, VALIDATE CONSTRAINT check_i1;
+
+# Ensure check "check_i2" is successfully added.
+statement error pq: failed to satisfy CHECK constraint \(i > 10:::INT8\)
+INSERT INTO t_96648 VALUES (5)
+
+statement error pgcode 42704 constraint "check_i" of relation "t_96648" does not exist
+ALTER TABLE t_96648 DROP CONSTRAINT check_i, VALIDATE CONSTRAINT check_i;
+
+statement error pgcode 42704 constraint "check_i1" of relation "t_96648" does not exist
+ALTER TABLE t_96648 DROP CONSTRAINT check_i1, VALIDATE CONSTRAINT check_i1;
+
+statement ok
+ALTER TABLE t_96648 DROP CONSTRAINT check_i, DROP CONSTRAINT check_i1;
+
+statement ok
+DROP TABLE t_96648

pkg/sql/opt/exec/execbuilder/testdata/unique

@@ -1775,6 +1775,13 @@ ALTER TABLE uniq_overlaps_pk VALIDATE CONSTRAINT unique_a
 
 # Same test as the previous, but now that the constraint has been validated, it
 # can be treated as a key. This allows the joins to be more efficient.
+#
+# The implementation detail for VALIDATE CONSTRAINT is slightly different between
+# legacy and declarative schema changer, causing the output of the following
+# EXPLAIN UPDATE to be slightly different: the ordering of three same-level
+# constraint checks is different. We thus test on the same EXPLAIN UPDATE twice
+# with slightly different expected output.
+skipif config local
 query T
 EXPLAIN UPDATE uniq_overlaps_pk SET a = 1, b = 2, c = 3, d = 4 WHERE a = 5
 ----
@@ -1844,6 +1851,76 @@ vectorized: true
             └── • scan buffer
                   label: buffer 1
 
+onlyif config local
+query T
+EXPLAIN UPDATE uniq_overlaps_pk SET a = 1, b = 2, c = 3, d = 4 WHERE a = 5
+----
+distribution: local
+vectorized: true
+·
+• root
+│
+├── • update
+│   │ table: uniq_overlaps_pk
+│   │ set: a, b, c, d
+│   │
+│   └── • buffer
+│       │ label: buffer 1
+│       │
+│       └── • render
+│           │
+│           └── • scan
+│                 missing stats
+│                 table: uniq_overlaps_pk@uniq_overlaps_pk_pkey
+│                 spans: [/5 - /5]
+│                 locking strength: for update
+│
+├── • constraint-check
+│   │
+│   └── • error if rows
+│       │
+│       └── • hash join (right semi)
+│           │ equality: (b, c) = (b_new, c_new)
+│           │ right cols are key
+│           │ pred: a_new != a
+│           │
+│           ├── • scan
+│           │     missing stats
+│           │     table: uniq_overlaps_pk@uniq_overlaps_pk_pkey
+│           │     spans: FULL SCAN
+│           │
+│           └── • scan buffer
+│                 label: buffer 1
+│
+├── • constraint-check
+│   │
+│   └── • error if rows
+│       │
+│       └── • hash join (right semi)
+│           │ equality: (c, d) = (c_new, d_new)
+│           │ right cols are key
+│           │ pred: (a_new != a) OR (b_new != b)
+│           │
+│           ├── • scan
+│           │     missing stats
+│           │     table: uniq_overlaps_pk@uniq_overlaps_pk_pkey
+│           │     spans: FULL SCAN
+│           │
+│           └── • scan buffer
+│                 label: buffer 1
+│
+└── • constraint-check
+    │
+    └── • error if rows
+        │
+        └── • lookup join (semi)
+            │ table: uniq_overlaps_pk@uniq_overlaps_pk_pkey
+            │ equality: (a_new) = (a)
+            │ pred: b_new != b
+            │
+            └── • scan buffer
+                  label: buffer 1
+
 # Update with non-constant input.
 # No need to add a check for b,c since those columns weren't updated.
 # Add inequality filters for the hidden primary key column.

pkg/sql/schema_changer.go

@@ -1413,7 +1413,8 @@ func (sc *SchemaChanger) done(ctx context.Context) error {
 				}
 			}
 			if fk := m.AsForeignKey(); fk != nil && fk.Adding() &&
-				fk.GetConstraintValidity() == descpb.ConstraintValidity_Unvalidated {
+				(fk.GetConstraintValidity() == descpb.ConstraintValidity_Unvalidated ||
+					fk.GetConstraintValidity() == descpb.ConstraintValidity_Validated) {
 				// Add backreference on the referenced table (which could be the same table)
 				backrefTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID())
 				if err != nil {

pkg/sql/schemachanger/scbuild/builder_state.go

@@ -1053,8 +1053,7 @@ func (b *builderState) ResolveConstraint(
 		if p.IsExistenceOptional {
 			return nil
 		}
-		panic(pgerror.Newf(pgcode.UndefinedObject,
-			"constraint %q of relation %q does not exist", constraintName, rel.GetName()))
+		panic(sqlerrors.NewUndefinedConstraintError(string(constraintName), rel.GetName()))
 	}
 
 	return c.ers.Filter(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) bool {

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel

@@ -11,6 +11,7 @@ go_library(
         "alter_table_alter_primary_key.go",
         "alter_table_drop_column.go",
         "alter_table_drop_constraint.go",
+        "alter_table_validate_constraint.go",
         "comment_on.go",
         "create_function.go",
         "create_index.go",

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go

@@ -72,7 +72,8 @@ var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{
 
 		return false
 	}},
-	reflect.TypeOf((*tree.AlterTableDropConstraint)(nil)): {fn: alterTableDropConstraint, on: true, minSupportedClusterVersion: clusterversion.V23_1},
+	reflect.TypeOf((*tree.AlterTableDropConstraint)(nil)):     {fn: alterTableDropConstraint, on: true, minSupportedClusterVersion: clusterversion.V23_1},
+	reflect.TypeOf((*tree.AlterTableValidateConstraint)(nil)): {fn: alterTableValidateConstraint, on: true, minSupportedClusterVersion: clusterversion.V23_1},
 }
 
 // alterTableAddConstraintMinSupportedClusterVersion tracks the minimal supported cluster version

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go

@@ -592,9 +592,9 @@ func validateConstraintNameIsNotUsed(
 
 	var isInUse bool
 	scpb.ForEachConstraintWithoutIndexName(b.QueryByID(tbl.TableID), func(
-		_ scpb.Status, _ scpb.TargetStatus, e *scpb.ConstraintWithoutIndexName,
+		_ scpb.Status, target scpb.TargetStatus, e *scpb.ConstraintWithoutIndexName,
 	) {
-		if e.Name == string(name) {
+		if target == scpb.ToPublic && e.Name == string(name) {
 			isInUse = true
 		}
 	})