sql: legacy schema changer handles add/drop constraint and validate c…

…onstraint in one stmt correctly

Previously, CRDB has incompatible behavior when it comes to adding and
validating constraints in one statment.
Consider
```
1. ALTER TABLE .. ADD CONSTRAINT .. [NOT VALID], VALIDATE CONSTRAINT ..;
2. ALTER TABLE .. DROP CONSTRAINT .. [NOT VALID], VALIDATE CONSTRAINT ..;
```
This PR ensures
1. The VALIDATE CONSTRAINT is either a no-op (for vanilla constraint)
   or validates the newly added constraint (for not-valid constraint)

2. The VALIDATE CONSTRAINT errors with a Constraint-Not-Found error.

This is consistent with PG's behavior.

This also result in behavior changes in schema changes in txns. E.g.
```
BEGIN;

ALTER TABLE .. ADD CONSTRAINT .. [NOT VALID]
Or
ALTER TABLE .. DROP CONSTRAINT .. [NOT VALID]

ALTER TABLE .. VALIDATE CONSTRAINT ..

COMMIT;
```
The VALIDATE CONSTRAINT will have the same behavior here as above.
This is also consistent with PG's behavior.

Release note (bug fix): Fixed bugs for
 `ALTER TABLE .. ADD/DROP CONSTRAINT ..; VALIDATE CONSTRAINT ..`
so that its behavior is consistent with PG.

pkg/sql/alter_table.go

@@ -523,12 +523,9 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name)
 			}
 			switch c.GetConstraintValidity() {
-			case descpb.ConstraintValidity_Validated:
+			case descpb.ConstraintValidity_Validated, descpb.ConstraintValidity_Validating:
 				// Nothing to do.
 				continue
-			case descpb.ConstraintValidity_Validating:
-				return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
-					"constraint %q in the middle of being added, try again later", t.Constraint)
 			case descpb.ConstraintValidity_Dropping:
 				return sqlerrors.NewUndefinedConstraintError(string(t.Constraint), n.tableDesc.Name)
 			}

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,17 @@ 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 constraint mutation's validity being VALIDATED is strange but could happen
+					// in the legacy schema changer when we do
+					// `ALTER TABLE ... ADD CONSTRAINT ... NOT VALID, VALIDATE CONSTRAINT ...`
+					// where `ADD CONSTRAINT ... NOT VALID` enqueues a mutation and
+					// `VALIDATE CONSTRAINT` change this mutation's validity to VALIDATED.
+					// We thus will run into this case later in the job for
+					// `ADD CONSTRAINT ... NOT VALID`, at which point we should just append it
+					// to the public slice and return.
 					desc.Checks = append(desc.Checks, &t.Constraint.Check)
 				default:
 					return errors.AssertionFailedf("invalid constraint validity state: %d", t.Constraint.Check.Validity)
@@ -1409,7 +1417,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 +1435,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/logictest/testdata/logic_test/alter_table

@@ -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 ok
+ALTER TABLE t_96648 ADD CONSTRAINT check_i CHECK (i > 0), VALIDATE CONSTRAINT check_i;
+
+statement ok
+ALTER TABLE t_96648 ADD CONSTRAINT check_i1 CHECK (i > 0) NOT VALID, VALIDATE CONSTRAINT check_i1;
+
+query TTTTB colnames
+SHOW CONSTRAINTS FROM t_96648
+----
+table_name  constraint_name  constraint_type  details              validated
+t_96648     check_i          CHECK            CHECK ((i > 0))      true
+t_96648     check_i1         CHECK            CHECK ((i > 0))      true
+t_96648     t_96648_pkey     PRIMARY KEY      PRIMARY KEY (i ASC)  true
+
+statement ok
+ALTER TABLE t_96648 ADD CONSTRAINT check_i2 CHECK (i > 0) NOT VALID;
+
+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_i2" of relation "t_96648" does not exist
+ALTER TABLE t_96648 DROP CONSTRAINT check_i2, VALIDATE CONSTRAINT check_i2;
+
+statement ok
+ALTER TABLE t_96648 DROP CONSTRAINT check_i, DROP CONSTRAINT check_i1, DROP CONSTRAINT check_i2;
+
+statement ok
+DROP TABLE t_96648

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn

@@ -831,14 +831,14 @@ CREATE TABLE orders2 (
   INDEX (product)
 )
 
-# The constraint can't be validated with VALIDATE CONSTRAINT in the same transaction
+# VALIDATE CONSTRAINT when a vanilla constraint is being added is a no-op.
 statement ok
 BEGIN
 
 statement ok
 ALTER TABLE orders2 ADD FOREIGN KEY (product) REFERENCES products
 
-statement error constraint "orders2_product_fkey" in the middle of being added, try again later
+statement ok
 ALTER TABLE orders2 VALIDATE CONSTRAINT orders2_product_fkey
 
 statement ok
@@ -851,7 +851,7 @@ BEGIN
 statement ok
 ALTER TABLE orders2 ADD FOREIGN KEY (product) REFERENCES products
 
-statement error constraint "orders2_product_fkey" in the middle of being added, try again later
+statement error pq: referencing constraint "orders2_product_fkey_1" in the middle of being added, try again later
 ALTER TABLE orders2 DROP COLUMN product
 
 statement ok
@@ -864,7 +864,7 @@ BEGIN
 statement ok
 ALTER TABLE orders2 ADD FOREIGN KEY (product) REFERENCES products
 
-statement error constraint "orders2_product_fkey" in the middle of being added, try again later
+statement error pq: referencing constraint "orders2_product_fkey_1" in the middle of being added, try again later
 DROP INDEX orders2@orders2_product_idx
 
 statement ok
@@ -1051,7 +1051,7 @@ ALTER TABLE check_table ADD h INT
 statement ok
 ALTER TABLE check_table ADD CONSTRAINT h_0 CHECK (h > 0)
 
-statement error constraint "h_0" in the middle of being added
+statement ok
 ALTER TABLE check_table VALIDATE CONSTRAINT h_0
 
 statement ok

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 {