sql: schema changer not to validate shard column constraint

Fixes cockroachdb#67613
The shard column constraint is created internally and should
be automatically upheld. So not need to verify it when backfilling
hash sharded indexes.

Release not: None

pkg/sql/BUILD.bazel

@@ -446,6 +446,7 @@ go_test(
         "ambiguous_commit_test.go",
         "as_of_test.go",
         "backfill_num_ranges_in_span_test.go",
+        "backfill_test.go",
         "builtin_mem_usage_test.go",
         "builtin_test.go",
         "comment_on_column_test.go",

pkg/sql/backfill.go

@@ -250,21 +250,16 @@ func (sc *SchemaChanger) runBackfill(ctx context.Context) error {
 				addedIndexSpans = append(addedIndexSpans, tableDesc.IndexSpan(sc.execCfg.Codec, idx.GetID()))
 				addedIndexes = append(addedIndexes, idx.GetID())
 			} else if c := m.AsConstraint(); c != nil {
-				isValidating := false
-				if c.IsCheck() {
-					isValidating = c.Check().Validity == descpb.ConstraintValidity_Validating
-				} else if c.IsForeignKey() {
-					isValidating = c.ForeignKey().Validity == descpb.ConstraintValidity_Validating
-				} else if c.IsUniqueWithoutIndex() {
-					isValidating = c.UniqueWithoutIndex().Validity == descpb.ConstraintValidity_Validating
-				} else if c.IsNotNull() {
-					// NOT NULL constraints are always validated before they can be added
-					isValidating = true
+				isValidating, isAdding, err := shouldValidateOrAddConstraint(tableDesc, c)
+				if err != nil {
+					return err
 				}
 				if isValidating {
-					constraintsToAddBeforeValidation = append(constraintsToAddBeforeValidation, c)
 					constraintsToValidate = append(constraintsToValidate, c)
 				}
+				if isAdding {
+					constraintsToAddBeforeValidation = append(constraintsToAddBeforeValidation, c)
+				}
 			} else if mvRefresh := m.AsMaterializedViewRefresh(); mvRefresh != nil {
 				viewToRefresh = mvRefresh
 			} else if m.AsPrimaryKeySwap() != nil || m.AsComputedColumnSwap() != nil {
@@ -361,6 +356,53 @@ func (sc *SchemaChanger) runBackfill(ctx context.Context) error {
 	return nil
 }
 
+// shouldValidateCheckConstraint checks if a Check Constraint should be
+// validated during backfill. A Check Constraint can skip validation if it's
+// created for a shard column internally.
+func shouldValidateCheckConstraint(
+	tableDesc *tabledesc.Mutable, checkConstraint descpb.TableDescriptor_CheckConstraint,
+) (bool, error) {
+	// The check constraint on shard column is always on the shard column itself.
+	if len(checkConstraint.ColumnIDs) != 1 {
+		return checkConstraint.Validity == descpb.ConstraintValidity_Validating, nil
+	}
+
+	checkCol, err := tableDesc.FindColumnWithID(checkConstraint.ColumnIDs[0])
+	if err != nil {
+		return false, err
+	}
+
+	if tableDesc.IsShardColumn(checkCol) && checkCol.Adding() {
+		return false, nil
+	}
+	return checkConstraint.Validity == descpb.ConstraintValidity_Validating, nil
+}
+
+func shouldValidateOrAddConstraint(
+	tableDesc *tabledesc.Mutable, constraint catalog.ConstraintToUpdate,
+) (shouldValidate bool, shouldAdd bool, err error) {
+	if constraint.IsCheck() {
+		shouldValidate, err = shouldValidateCheckConstraint(tableDesc, constraint.Check())
+		if err != nil {
+			return
+		}
+	} else if constraint.IsForeignKey() {
+		shouldValidate = constraint.ForeignKey().Validity == descpb.ConstraintValidity_Validating
+	} else if constraint.IsUniqueWithoutIndex() {
+		shouldValidate = constraint.UniqueWithoutIndex().Validity == descpb.ConstraintValidity_Validating
+	} else if constraint.IsNotNull() {
+		// NOT NULL constraints are always validated before they can be added
+		shouldValidate = true
+	}
+
+	if constraint.IsCheck() {
+		shouldAdd = constraint.Check().Validity == descpb.ConstraintValidity_Validating
+	} else {
+		shouldAdd = shouldValidate
+	}
+	return
+}
+
 // dropConstraints publishes a new version of the given table descriptor with
 // the given constraint removed from it, and waits until the entire cluster is
 // on the new version of the table descriptor. It returns the new table descs.

pkg/sql/backfill_test.go

@@ -0,0 +1,163 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package sql
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/stretchr/testify/require"
+)
+
+func TestShouldValidateCheckConstraint(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	tableDesc := &tabledesc.Mutable{}
+	tableDesc.TableDescriptor = descpb.TableDescriptor{
+		ID:            2,
+		ParentID:      1,
+		Name:          "foo",
+		FormatVersion: descpb.InterleavedFormatVersion,
+		Columns: []descpb.ColumnDescriptor{
+			{ID: 1, Name: "c1"},
+		},
+		Families: []descpb.ColumnFamilyDescriptor{
+			{ID: 0, Name: "primary", ColumnIDs: []descpb.ColumnID{1, 2}, ColumnNames: []string{"c1", "c2"}},
+		},
+		PrimaryIndex: descpb.IndexDescriptor{
+			ID: 1, Name: "pri", KeyColumnIDs: []descpb.ColumnID{1},
+			KeyColumnNames:      []string{"c1"},
+			KeyColumnDirections: []descpb.IndexDescriptor_Direction{descpb.IndexDescriptor_ASC},
+			EncodingType:        descpb.PrimaryIndexEncoding,
+			Version:             descpb.LatestPrimaryIndexDescriptorVersion,
+		},
+		Mutations: []descpb.DescriptorMutation{
+			{
+				Descriptor_: &descpb.DescriptorMutation_Index{
+					Index: &descpb.IndexDescriptor{
+						ID: 2, Name: "new_hash_index", KeyColumnIDs: []descpb.ColumnID{2, 3},
+						KeyColumnNames: []string{"c2", "c3"},
+						KeyColumnDirections: []descpb.IndexDescriptor_Direction{
+							descpb.IndexDescriptor_ASC,
+							descpb.IndexDescriptor_ASC,
+						},
+						EncodingType: descpb.PrimaryIndexEncoding,
+						Version:      descpb.LatestPrimaryIndexDescriptorVersion,
+						Sharded: descpb.ShardedDescriptor{
+							IsSharded:    true,
+							Name:         "c3",
+							ShardBuckets: 8,
+							ColumnNames:  []string{"c2"},
+						},
+					},
+				},
+				Direction: descpb.DescriptorMutation_ADD,
+			},
+			{
+				Descriptor_: &descpb.DescriptorMutation_Column{
+					Column: &descpb.ColumnDescriptor{
+						ID:      2,
+						Name:    "c2",
+						Virtual: true,
+					},
+				},
+				Direction: descpb.DescriptorMutation_ADD,
+			},
+			{
+				Descriptor_: &descpb.DescriptorMutation_Column{
+					Column: &descpb.ColumnDescriptor{
+						ID:      3,
+						Name:    "c3",
+						Virtual: true,
+					},
+				},
+				Direction: descpb.DescriptorMutation_ADD,
+			},
+		},
+	}
+
+	type testCase struct {
+		name            string
+		checkConstraint descpb.TableDescriptor_CheckConstraint
+		expectedResult  bool
+	}
+
+	tcs := []testCase{
+		{
+			name: "test_adding_shard_col_check_constraint",
+			checkConstraint: descpb.TableDescriptor_CheckConstraint{
+				Expr:      "some fake expr",
+				Name:      "some fake name",
+				Validity:  descpb.ConstraintValidity_Validating,
+				ColumnIDs: []descpb.ColumnID{3},
+				Hidden:    true,
+			},
+			expectedResult: false,
+		},
+		{
+			name: "test_adding_non_shard_col_check_constraint",
+			checkConstraint: descpb.TableDescriptor_CheckConstraint{
+				Expr:      "some fake expr",
+				Name:      "some fake name",
+				Validity:  descpb.ConstraintValidity_Validating,
+				ColumnIDs: []descpb.ColumnID{2},
+				Hidden:    false,
+			},
+			expectedResult: true,
+		},
+		{
+			name: "test_adding_non_shard_col_check_constraint_not_validating",
+			checkConstraint: descpb.TableDescriptor_CheckConstraint{
+				Expr:      "some fake expr",
+				Name:      "some fake name",
+				Validity:  descpb.ConstraintValidity_Validated,
+				ColumnIDs: []descpb.ColumnID{2},
+				Hidden:    false,
+			},
+			expectedResult: false,
+		},
+		{
+			name: "test_adding_multi_col_check_constraint",
+			checkConstraint: descpb.TableDescriptor_CheckConstraint{
+				Expr:      "some fake expr",
+				Name:      "some fake name",
+				Validity:  descpb.ConstraintValidity_Validating,
+				ColumnIDs: []descpb.ColumnID{2, 3},
+				Hidden:    false,
+			},
+			expectedResult: true,
+		},
+		{
+			name: "test_adding_multi_col_check_constraint_not_validating",
+			checkConstraint: descpb.TableDescriptor_CheckConstraint{
+				Expr:      "some fake expr",
+				Name:      "some fake name",
+				Validity:  descpb.ConstraintValidity_Validated,
+				ColumnIDs: []descpb.ColumnID{2, 3},
+				Hidden:    false,
+			},
+			expectedResult: false,
+		},
+	}
+
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			isValidating, err := shouldValidateCheckConstraint(tableDesc, tc.checkConstraint)
+			if err != nil {
+				t.Fatal("Failed to run function being tested:", err)
+			}
+			require.Equal(t, tc.expectedResult, isValidating)
+		})
+	}
+
+}