Merge pull request cockroachdb#93068 from Xiang-Gu/implement-add-cons…

…traint-foreign-key

docs/generated/http/BUILD.bazel

@@ -25,6 +25,7 @@ genrule(
         "//pkg/sql/catalog/fetchpb:fetchpb_proto",
         "//pkg/sql/contentionpb:contentionpb_proto",
         "//pkg/sql/execinfrapb:execinfrapb_proto",
+        "//pkg/sql/sem/semenumpb:semenumpb_proto",
         "//pkg/sql/inverted:inverted_proto",
         "//pkg/sql/lex:lex_proto",
         "//pkg/sql/pgwire/pgerror:pgerror_proto",

pkg/BUILD.bazel

@@ -1799,6 +1799,7 @@ GO_TARGETS = [
     "//pkg/sql/sem/eval:eval_test",
     "//pkg/sql/sem/normalize:normalize",
     "//pkg/sql/sem/normalize:normalize_test",
+    "//pkg/sql/sem/semenumpb:semenumpb",
     "//pkg/sql/sem/transform:transform",
     "//pkg/sql/sem/tree/evalgen:evalgen",
     "//pkg/sql/sem/tree/evalgen:evalgen_lib",
@@ -2931,6 +2932,7 @@ GET_X_DATA_TARGETS = [
     "//pkg/sql/sem/eval/cast_test:get_x_data",
     "//pkg/sql/sem/eval/eval_test:get_x_data",
     "//pkg/sql/sem/normalize:get_x_data",
+    "//pkg/sql/sem/semenumpb:get_x_data",
     "//pkg/sql/sem/transform:get_x_data",
     "//pkg/sql/sem/tree:get_x_data",
     "//pkg/sql/sem/tree/evalgen:get_x_data",

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -9,8 +9,8 @@ exp,benchmark
 12,AlterTableAddColumn/alter_table_add_2_columns
 12,AlterTableAddColumn/alter_table_add_3_columns
 13,AlterTableAddForeignKey/alter_table_add_1_foreign_key
-17,AlterTableAddForeignKey/alter_table_add_2_foreign_keys
-21,AlterTableAddForeignKey/alter_table_add_3_foreign_keys
+13,AlterTableAddForeignKey/alter_table_add_2_foreign_keys
+13,AlterTableAddForeignKey/alter_table_add_3_foreign_keys
 13,AlterTableAddForeignKey/alter_table_add_foreign_key_with_3_columns
 8,AlterTableConfigureZone/alter_table_configure_zone_5_replicas
 8,AlterTableConfigureZone/alter_table_configure_zone_7_replicas_

pkg/gen/protobuf.bzl

@@ -62,6 +62,7 @@ PROTOBUF_SRCS = [
     "//pkg/sql/protoreflect/test:protoreflecttest_go_proto",
     "//pkg/sql/rowenc/rowencpb:rowencpb_go_proto",
     "//pkg/sql/schemachanger/scpb:scpb_go_proto",
+    "//pkg/sql/sem/semenumpb:semenumpb_go_proto",
     "//pkg/sql/sessiondatapb:sessiondatapb_go_proto",
     "//pkg/sql/sqlstats/insights:insights_go_proto",
     "//pkg/sql/sqlstats/persistedsqlstats:persistedsqlstats_go_proto",

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/BUILD.bazel

@@ -434,6 +434,7 @@ go_library(
         "//pkg/sql/sem/catconstants",
         "//pkg/sql/sem/catid",
         "//pkg/sql/sem/eval",
+        "//pkg/sql/sem/semenumpb",
         "//pkg/sql/sem/transform",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sem/tree/treebin",

pkg/sql/alter_table.go

@@ -39,6 +39,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/semenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
@@ -923,8 +924,8 @@ func applyColumnMutation(
 		for _, fk := range tableDesc.OutboundFKs {
 			for _, colID := range fk.OriginColumnIDs {
 				if colID == col.GetID() &&
-					fk.OnUpdate != catpb.ForeignKeyAction_NO_ACTION &&
-					fk.OnUpdate != catpb.ForeignKeyAction_RESTRICT {
+					fk.OnUpdate != semenumpb.ForeignKeyAction_NO_ACTION &&
+					fk.OnUpdate != semenumpb.ForeignKeyAction_RESTRICT {
 					return pgerror.Newf(
 						pgcode.InvalidColumnDefinition,
 						"column %s(%d) cannot have both an ON UPDATE expression and a foreign"+

pkg/sql/backfill.go

@@ -39,6 +39,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"
@@ -1502,18 +1503,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
@@ -1530,10 +1532,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")
+		}
 	})
 }
 
@@ -2661,7 +2688,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/catalog/BUILD.bazel

@@ -36,6 +36,8 @@ go_library(
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/privilege",
         "//pkg/sql/schemachanger/scpb",
+        "//pkg/sql/sem/catconstants",
+        "//pkg/sql/sem/semenumpb",
         "//pkg/sql/sem/tree",
         "//pkg/sql/types",
         "//pkg/util",

pkg/sql/catalog/catpb/BUILD.bazel

@@ -9,6 +9,7 @@ proto_library(
     name = "catpb_proto",
     srcs = [
         "catalog.proto",
+        "enum.proto",
         "function.proto",
         "privilege.proto",
     ],
@@ -33,7 +34,6 @@ go_library(
     name = "catpb",
     srcs = [
         "catalog.go",
-        "constraint.go",
         "default_privilege.go",
         "doc.go",
         "expression.go",
@@ -53,7 +53,6 @@ go_library(
         "//pkg/sql/sem/catconstants",
         "//pkg/sql/sem/catid",
         "@com_github_cockroachdb_errors//:errors",
-        "@com_github_cockroachdb_redact//:redact",
     ],
 )
 

pkg/sql/catalog/catpb/catalog.proto

@@ -18,17 +18,6 @@ option go_package = "catpb";
 
 import "gogoproto/gogo.proto";
 
-// ForeignKeyAction describes the action which should be taken when a foreign
-// key constraint reference is acted upon.
-enum ForeignKeyAction {
-  option (gogoproto.goproto_enum_stringer) = false;
-  NO_ACTION = 0;
-  RESTRICT = 1;
-  SET_NULL = 2;
-  SET_DEFAULT = 3;
-  CASCADE = 4;
-}
-
 // LocalityConfig is used to figure the locality of a table.
 message LocalityConfig {
   option (gogoproto.equal) = true;
@@ -66,18 +55,6 @@ message LocalityConfig {
   }
 }
 
-// SystemColumnKind is an enum representing the different kind of system
-// columns that can be synthesized by the execution engine.
-enum SystemColumnKind {
-  // Default value, unused.
-  NONE = 0;
-  // A system column containing the value of the MVCC timestamp associated
-  // with the kv's corresponding to the row.
-  MVCCTIMESTAMP = 1;
-  // A system column containing the OID of the table that the row came from.
-  TABLEOID = 2;
-}
-
 // GeneratedAsIdentityType is an enum representing how the creation of
 // a column is associated with the GENERATED {ALWAYS | BY DEFAULT} AS IDENTITY
 // syntax.
@@ -234,23 +211,3 @@ message AutoStatsSettings {
   // FractionStaleRows is table setting sql_stats_automatic_collection_fraction_stale_rows.
   optional double fraction_stale_rows = 3;
 }
-
-// InvertedIndexColumnKind is the kind of the inverted index on a column. The
-// reason this needs to be stored is that we need to be able to check that the
-// "opclass" passed into an inverted index declaration (for example,
-// gin_trgm_ops) is compatible with the datatype of a particular column
-// (gin_tgrm_ops is only valid on text). A future reason is that it's possible
-// to desire having more than one type of inverted index on a particular
-// datatype - for example, you might want to create a "stemming" inverted index
-// on text. And without this extra kind, it wouldn't be possible to distinguish
-// a text inverted index that uses trigrams, vs a text inverted index that uses
-// stemming.
-enum InvertedIndexColumnKind {
-  // DEFAULT is the default kind of inverted index column. JSON, Array, and
-  // geo inverted indexes all are DEFAULT, though prior to 22.2 they had no
-  // kind at all.
-  DEFAULT = 0;
-  // TRIGRAM is the trigram kind of inverted index column. It's only valid on
-  // text columns.
-  TRIGRAM = 1;
-}

pkg/sql/catalog/catpb/enum.proto

@@ -0,0 +1,51 @@
+// Copyright 2022 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.
+
+// This file should contain only EMUN definitions for concepts that
+// are internal and not visible to the SQL layer.
+// It uses proto3 so other packages can import those enum definitions
+// when needed.
+syntax = "proto3";
+package cockroach.sql.catalog.catpb;
+option go_package = "catpb";
+
+import "gogoproto/gogo.proto";
+
+// SystemColumnKind is an enum representing the different kind of system
+// columns that can be synthesized by the execution engine.
+enum SystemColumnKind {
+  // Default value, unused.
+  NONE = 0;
+  // A system column containing the value of the MVCC timestamp associated
+  // with the kv's corresponding to the row.
+  MVCCTIMESTAMP = 1;
+  // A system column containing the OID of the table that the row came from.
+  TABLEOID = 2;
+}
+
+// InvertedIndexColumnKind is the kind of the inverted index on a column. The
+// reason this needs to be stored is that we need to be able to check that the
+// "opclass" passed into an inverted index declaration (for example,
+// gin_trgm_ops) is compatible with the datatype of a particular column
+// (gin_tgrm_ops is only valid on text). A future reason is that it's possible
+// to desire having more than one type of inverted index on a particular
+// datatype - for example, you might want to create a "stemming" inverted index
+// on text. And without this extra kind, it wouldn't be possible to distinguish
+// a text inverted index that uses trigrams, vs a text inverted index that uses
+// stemming.
+enum InvertedIndexColumnKind {
+  // DEFAULT is the default kind of inverted index column. JSON, Array, and
+  // geo inverted indexes all are DEFAULT, though prior to 22.2 they had no
+  // kind at all.
+  DEFAULT = 0;
+  // TRIGRAM is the trigram kind of inverted index column. It's only valid on
+  // text columns.
+  TRIGRAM = 1;
+}

pkg/sql/catalog/descpb/BUILD.bazel

@@ -9,7 +9,6 @@ go_library(
     name = "descpb",
     srcs = [
         "column.go",
-        "constraint.go",
         "descriptor.go",
         "index.go",
         "join_type.go",
@@ -23,7 +22,6 @@ go_library(
     deps = [
         "//pkg/keys",
         "//pkg/sql/catalog/catenumpb",
-        "//pkg/sql/catalog/catpb",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/protoreflect",
@@ -64,6 +62,7 @@ proto_library(
         "//pkg/sql/catalog/catenumpb:catenumpb_proto",
         "//pkg/sql/catalog/catpb:catpb_proto",
         "//pkg/sql/schemachanger/scpb:scpb_proto",
+        "//pkg/sql/sem/semenumpb:semenumpb_proto",
         "//pkg/sql/types:types_proto",
         "//pkg/util/hlc:hlc_proto",
         "@com_github_gogo_protobuf//gogoproto:gogo_proto",
@@ -84,6 +83,7 @@ go_proto_library(
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/catalog/catpb",
         "//pkg/sql/schemachanger/scpb",
+        "//pkg/sql/sem/semenumpb",
         "//pkg/sql/types",
         "//pkg/util/hlc",
         "@com_github_gogo_protobuf//gogoproto",