backfill: Add logic for foreign key validation

This commit refactored some of the check constraint validation code
to implement a unified function for validating all kinds of constraints.

pkg/server/server_sql.go

@@ -1095,7 +1095,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 		execCfg.ProtectedTimestampManager,
 		sql.ValidateForwardIndexes,
 		sql.ValidateInvertedIndexes,
-		sql.ValidateCheckConstraint,
+		sql.ValidateConstraint,
 		sql.NewFakeSessionData,
 	)
 	execCfg.InternalExecutorFactory = ieFactory

pkg/sql/backfill.go

@@ -40,6 +40,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/rowexec"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowinfra"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -1503,18 +1504,19 @@ func (e InvalidIndexesError) Error() string {
 	return fmt.Sprintf("found %d invalid indexes", len(e.Indexes))
 }
 
-// ValidateCheckConstraint validates the check constraint against all rows
-// in index `indexIDForValidation` in table `tableDesc`.
-func ValidateCheckConstraint(
+// ValidateConstraint validates the constraint against all rows
+// in `tbl`.
+//
+// TODO (xiang): Support validating UNIQUE_WITHOUT_INDEX constraint in this function.
+func ValidateConstraint(
 	ctx context.Context,
 	tableDesc catalog.TableDescriptor,
-	checkConstraint catalog.CheckConstraint,
+	constraint catalog.Constraint,
 	indexIDForValidation descpb.IndexID,
 	sessionData *sessiondata.SessionData,
 	runHistoricalTxn descs.HistoricalInternalExecTxnRunner,
 	execOverride sessiondata.InternalExecutorOverride,
 ) (err error) {
-
 	tableDesc, err = tableDesc.MakeFirstMutationPublic(catalog.IgnoreConstraints)
 	if err != nil {
 		return err
@@ -1531,10 +1533,35 @@ func ValidateCheckConstraint(
 		semaCtx.TableNameResolver = resolver
 		defer func() { descriptors.ReleaseAll(ctx) }()
 
-		return ie.WithSyntheticDescriptors([]catalog.Descriptor{tableDesc}, func() error {
-			return validateCheckExpr(ctx, &semaCtx, txn, sessionData, checkConstraint.GetExpr(),
-				tableDesc.(*tabledesc.Mutable), ie, indexIDForValidation)
-		})
+		switch catalog.GetConstraintType(constraint) {
+		case catconstants.ConstraintTypeCheck:
+			ck := constraint.AsCheck()
+			return ie.WithSyntheticDescriptors(
+				[]catalog.Descriptor{tableDesc},
+				func() error {
+					return validateCheckExpr(ctx, &semaCtx, txn, sessionData, ck.GetExpr(),
+						tableDesc.(*tabledesc.Mutable), ie, indexIDForValidation)
+				},
+			)
+		case catconstants.ConstraintTypeFK:
+			fk := constraint.AsForeignKey()
+			targetTable, err := descriptors.ByID(txn).Get().Table(ctx, fk.GetReferencedTableID())
+			if err != nil {
+				return err
+			}
+			if targetTable.GetID() == tableDesc.GetID() {
+				targetTable = tableDesc
+			}
+			return ie.WithSyntheticDescriptors(
+				[]catalog.Descriptor{tableDesc},
+				func() error {
+					return validateForeignKey(ctx, tableDesc.(*tabledesc.Mutable), targetTable, fk.ForeignKeyDesc(),
+						indexIDForValidation, txn, ie)
+				},
+			)
+		default:
+			return errors.AssertionFailedf("validation of unsupported constraint type")
+		}
 	})
 }
 
@@ -2662,7 +2689,7 @@ func validateFkInTxn(
 	return ie.WithSyntheticDescriptors(
 		syntheticDescs,
 		func() error {
-			return validateForeignKey(ctx, srcTable, targetTable, fk, ie, txn)
+			return validateForeignKey(ctx, srcTable, targetTable, fk, 0 /* indexIDForValidation */, txn, ie)
 		})
 }
 

pkg/sql/check.go

@@ -171,12 +171,18 @@ func matchFullUnacceptableKeyQuery(
 // WHERE t.a IS NULL
 // LIMIT 1  -- if limitResults is set
 //
+// It is possible to force FK validation query to perform against a particular
+// index, as specified in `indexIDForValidation` when it's non zero. This is necessary
+// if we are validating a FK constraint on a primary index that's being added (e.g.
+// `ADD COLUMN ... REFERENCES other_table(...)`).
+//
 // TODO(radu): change this to a query which executes as an anti-join when we
 // remove the heuristic planner.
 func nonMatchingRowQuery(
 	srcTbl catalog.TableDescriptor,
 	fk *descpb.ForeignKeyConstraint,
 	targetTbl catalog.TableDescriptor,
+	indexIDForValidation descpb.IndexID,
 	limitResults bool,
 ) (sql string, originColNames []string, _ error) {
 	originColNames, err := srcTbl.NamesForColumnIDs(fk.OriginColumnIDs)
@@ -229,7 +235,7 @@ func nonMatchingRowQuery(
 	if limitResults {
 		limit = " LIMIT 1"
 	}
-	return fmt.Sprintf(
+	query := fmt.Sprintf(
 		`SELECT %[1]s FROM 
 		  (SELECT %[2]s FROM [%[3]d AS src]@{IGNORE_FOREIGN_KEYS} WHERE %[4]s) AS s
 			LEFT OUTER JOIN
@@ -245,7 +251,28 @@ func nonMatchingRowQuery(
 		// Sufficient to check the first column to see whether there was no matching row
 		targetCols[0], // 7
 		limit,         // 8
-	), originColNames, nil
+	)
+	if indexIDForValidation != 0 {
+		query = fmt.Sprintf(
+			`SELECT %[1]s FROM 
+		  (SELECT %[2]s FROM [%[3]d AS src]@{IGNORE_FOREIGN_KEYS, FORCE_INDEX=[%[4]d]} WHERE %[5]s) AS s
+			LEFT OUTER JOIN
+			[%[6]d AS target] AS t
+			ON %[7]s
+		 WHERE %[8]s IS NULL %[9]s`,
+			strings.Join(qualifiedSrcCols, ", "), // 1
+			strings.Join(srcCols, ", "),          // 2
+			srcTbl.GetID(),                       // 3
+			indexIDForValidation,                 // 4
+			strings.Join(srcWhere, " AND "),      // 5
+			targetTbl.GetID(),                    // 6
+			strings.Join(on, " AND "),            // 7
+			// Sufficient to check the first column to see whether there was no matching row
+			targetCols[0], // 8
+			limit,         // 9
+		)
+	}
+	return query, originColNames, nil
 }
 
 // validateForeignKey verifies that all the rows in the srcTable
@@ -258,8 +285,9 @@ func validateForeignKey(
 	srcTable *tabledesc.Mutable,
 	targetTable catalog.TableDescriptor,
 	fk *descpb.ForeignKeyConstraint,
-	ie sqlutil.InternalExecutor,
+	indexIDForValidation descpb.IndexID,
 	txn *kv.Txn,
+	ie sqlutil.InternalExecutor,
 ) error {
 	nCols := len(fk.OriginColumnIDs)
 
@@ -299,10 +327,7 @@ func validateForeignKey(
 			), fk.Name)
 		}
 	}
-	query, colNames, err := nonMatchingRowQuery(
-		srcTable, fk, targetTable,
-		true, /* limitResults */
-	)
+	query, colNames, err := nonMatchingRowQuery(srcTable, fk, targetTable, indexIDForValidation, true /* limitResults */)
 	if err != nil {
 		return err
 	}

pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go

@@ -999,15 +999,28 @@ func (s *TestState) Validator() scexec.Validator {
 	return s
 }
 
-// ValidateCheckConstraint implements the validator interface.
-func (s *TestState) ValidateCheckConstraint(
+// ValidateConstraint implements the validator interface.
+func (s *TestState) ValidateConstraint(
 	ctx context.Context,
 	tbl catalog.TableDescriptor,
-	constraint catalog.CheckConstraint,
+	constraint catalog.Constraint,
 	indexIDForValidation descpb.IndexID,
 	override sessiondata.InternalExecutorOverride,
 ) error {
-	s.LogSideEffectf("validate check constraint %v in table #%d", constraint.GetName(), tbl.GetID())
+	s.LogSideEffectf("validate %v constraint %v in table #%d",
+		catalog.GetConstraintType(constraint), constraint.GetName(), tbl.GetID())
+	return nil
+}
+
+func (s *TestState) ValidateForeignKeyConstraint(
+	ctx context.Context,
+	out catalog.TableDescriptor,
+	in catalog.TableDescriptor,
+	constraint catalog.Constraint,
+	override sessiondata.InternalExecutorOverride,
+) error {
+	s.LogSideEffectf("validate foreign key constraint %v from table #%d to table #%d",
+		constraint.GetName(), out.GetID(), in.GetID())
 	return nil
 }
 

pkg/sql/schemachanger/scdeps/validator.go

@@ -53,11 +53,11 @@ type ValidateInvertedIndexesFn func(
 	protectedTSProvider scexec.ProtectedTimestampManager,
 ) error
 
-// ValidateCheckConstraintFn callback function for validting check constraints.
-type ValidateCheckConstraintFn func(
+// ValidateConstraintFn callback function for validating constraints.
+type ValidateConstraintFn func(
 	ctx context.Context,
 	tbl catalog.TableDescriptor,
-	constraint catalog.CheckConstraint,
+	constraint catalog.Constraint,
 	indexIDForValidation descpb.IndexID,
 	sessionData *sessiondata.SessionData,
 	runHistoricalTxn descs.HistoricalInternalExecTxnRunner,
@@ -75,7 +75,7 @@ type validator struct {
 	ieFactory                  sqlutil.InternalExecutorFactory
 	validateForwardIndexes     ValidateForwardIndexesFn
 	validateInvertedIndexes    ValidateInvertedIndexesFn
-	validateCheckConstraint    ValidateCheckConstraintFn
+	validateConstraint         ValidateConstraintFn
 	newFakeSessionData         NewFakeSessionDataFn
 	protectedTimestampProvider scexec.ProtectedTimestampManager
 }
@@ -113,14 +113,14 @@ func (vd validator) ValidateInvertedIndexes(
 	)
 }
 
-func (vd validator) ValidateCheckConstraint(
+func (vd validator) ValidateConstraint(
 	ctx context.Context,
 	tbl catalog.TableDescriptor,
-	constraint catalog.CheckConstraint,
+	constraint catalog.Constraint,
 	indexIDForValidation descpb.IndexID,
 	override sessiondata.InternalExecutorOverride,
 ) error {
-	return vd.validateCheckConstraint(ctx, tbl, constraint, indexIDForValidation, vd.newFakeSessionData(&vd.settings.SV),
+	return vd.validateConstraint(ctx, tbl, constraint, indexIDForValidation, vd.newFakeSessionData(&vd.settings.SV),
 		vd.makeHistoricalInternalExecTxnRunner(), override)
 }
 
@@ -151,7 +151,7 @@ func NewValidator(
 	protectedTimestampProvider scexec.ProtectedTimestampManager,
 	validateForwardIndexes ValidateForwardIndexesFn,
 	validateInvertedIndexes ValidateInvertedIndexesFn,
-	validateCheckConstraint ValidateCheckConstraintFn,
+	validateCheckConstraint ValidateConstraintFn,
 	newFakeSessionData NewFakeSessionDataFn,
 ) scexec.Validator {
 	return validator{
@@ -161,7 +161,7 @@ func NewValidator(
 		ieFactory:                  ieFactory,
 		validateForwardIndexes:     validateForwardIndexes,
 		validateInvertedIndexes:    validateInvertedIndexes,
-		validateCheckConstraint:    validateCheckConstraint,
+		validateConstraint:         validateCheckConstraint,
 		newFakeSessionData:         newFakeSessionData,
 		protectedTimestampProvider: protectedTimestampProvider,
 	}

pkg/sql/schemachanger/scexec/dependencies.go

@@ -234,10 +234,10 @@ type Validator interface {
 		override sessiondata.InternalExecutorOverride,
 	) error
 
-	ValidateCheckConstraint(
+	ValidateConstraint(
 		ctx context.Context,
 		tbl catalog.TableDescriptor,
-		constraint catalog.CheckConstraint,
+		constraint catalog.Constraint,
 		indexIDForValidation descpb.IndexID,
 		override sessiondata.InternalExecutorOverride,
 	) error

pkg/sql/schemachanger/scexec/exec_validation.go

@@ -50,8 +50,8 @@ func executeValidateUniqueIndex(
 	return nil
 }
 
-func executeValidateCheckConstraint(
-	ctx context.Context, deps Dependencies, op *scop.ValidateCheckConstraint,
+func executeValidateConstraint(
+	ctx context.Context, deps Dependencies, op *scop.ValidateConstraint,
 ) error {
 	descs, err := deps.Catalog().MustReadImmutableDescriptors(ctx, op.TableID)
 	if err != nil {
@@ -66,15 +66,10 @@ func executeValidateCheckConstraint(
 	if err != nil {
 		return err
 	}
-	check := constraint.AsCheck()
-	if check == nil {
-		return errors.Newf("constraint ID %v does not identify a check constraint in table %v.",
-			op.ConstraintID, op.TableID)
-	}
 
 	// Execute the validation operation as a root user.
 	execOverride := sessiondata.RootUserSessionDataOverride
-	err = deps.Validator().ValidateCheckConstraint(ctx, table, check, op.IndexIDForValidation, execOverride)
+	err = deps.Validator().ValidateConstraint(ctx, table, constraint, op.IndexIDForValidation, execOverride)
 	if err != nil {
 		return scerrors.SchemaChangerUserError(err)
 	}
@@ -99,8 +94,8 @@ func executeValidationOp(ctx context.Context, deps Dependencies, op scop.Op) (er
 			}
 			return err
 		}
-	case *scop.ValidateCheckConstraint:
-		if err = executeValidateCheckConstraint(ctx, deps, op); err != nil {
+	case *scop.ValidateConstraint:
+		if err = executeValidateConstraint(ctx, deps, op); err != nil {
 			if !scerrors.HasSchemaChangerUserError(err) {
 				return errors.Wrapf(err, "%T: %v", op, op)
 			}

pkg/sql/schemachanger/scexec/executor_external_test.go

@@ -469,10 +469,10 @@ func (noopValidator) ValidateInvertedIndexes(
 	return nil
 }
 
-func (noopValidator) ValidateCheckConstraint(
+func (noopValidator) ValidateConstraint(
 	ctx context.Context,
 	tbl catalog.TableDescriptor,
-	constraint catalog.CheckConstraint,
+	constraint catalog.Constraint,
 	indexIDForValidation descpb.IndexID,
 	override sessiondata.InternalExecutorOverride,
 ) error {

pkg/sql/schemachanger/scop/validation.go

@@ -28,8 +28,8 @@ type ValidateIndex struct {
 	IndexID descpb.IndexID
 }
 
-// ValidateCheckConstraint validates a check constraint on a table's columns.
-type ValidateCheckConstraint struct {
+// ValidateConstraint validates a check constraint on a table's columns.
+type ValidateConstraint struct {
 	validationOp
 	TableID              descpb.ID
 	ConstraintID         descpb.ConstraintID

pkg/sql/schemachanger/scpb/uml/table.puml

@@ -92,10 +92,6 @@ ForeignKeyConstraint :  ConstraintID
 ForeignKeyConstraint : []ColumnIDs
 ForeignKeyConstraint :  ReferencedTableID
 ForeignKeyConstraint : []ReferencedColumnIDs
-ForeignKeyConstraint :  OnUpdateAction
-ForeignKeyConstraint :  OnDeleteAction
-ForeignKeyConstraint :  CompositeKeyMatchMethod
-ForeignKeyConstraint :  IndexIDForValidation
 
 object TableComment
 

pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint.go

@@ -49,8 +49,8 @@ func init() {
 				}),
 			),
 			to(scpb.Status_VALIDATED,
-				emit(func(this *scpb.CheckConstraint) *scop.ValidateCheckConstraint {
-					return &scop.ValidateCheckConstraint{
+				emit(func(this *scpb.CheckConstraint) *scop.ValidateConstraint {
+					return &scop.ValidateConstraint{
 						TableID:              this.TableID,
 						ConstraintID:         this.ConstraintID,
 						IndexIDForValidation: this.IndexIDForValidation,

pkg/sql/schemachanger/scplan/testdata/alter_table_add_check

@@ -32,7 +32,7 @@ PostCommitPhase stage 1 of 2 with 1 ValidationType op
   transitions:
     [[CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2}, PUBLIC], WRITE_ONLY] -> VALIDATED
   ops:
-    *scop.ValidateCheckConstraint
+    *scop.ValidateConstraint
       ConstraintID: 2
       TableID: 104
 PostCommitPhase stage 2 of 2 with 4 MutationType ops

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_vanilla

@@ -81,7 +81,7 @@ begin transaction #2
 commit transaction #2
 begin transaction #3
 ## PostCommitPhase stage 1 of 2 with 1 ValidationType op
-validate check constraint crdb_internal_constraint_2_name_placeholder in table #104
+validate CHECK constraint crdb_internal_constraint_2_name_placeholder in table #104
 commit transaction #3
 begin transaction #4
 ## PostCommitPhase stage 2 of 2 with 4 MutationType ops