schemachanger: implement ADD CONSTRAINT ... FOREIGN KEY
#93068
Conversation
|
This change is |
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
13 times, most recently
from
95f925a to
969a674
Compare
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
6 times, most recently
from
52fa6ba to
671ab31
Compare
|
This is ready for a look! Overall, It follows the same recipe as |
Xiang-Gu
changed the title
ADD CONSTRAINT ... FOREIGN KEYADD CONSTRAINT ... FOREIGN KEY
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
2 times, most recently
from
fbdc5ae to
d9ae4e5
Compare
pkg/sql/catalog/table_elements.go
Outdated
| @@ -440,6 +440,9 @@ type Constraint interface { | |||
| // GetName returns the name of this constraint update mutation. | |||
| GetName() string | |||
|
|
|||
| // GetConstraintType returns the constraint type. | |||
| GetConstraintType() descpb.ConstraintType | |||
There was a problem hiding this comment.
Rather than make this a method on the Constraint interface, can you make this a function instead? This isn't terribly important in this case, but a useful general principle is to keep the set of object methods to a minimum and to keep separate any logic which can be expressed using method calls and doesn't need any state that's internal to the object.
There was a problem hiding this comment.
FYI, I looked into where this new method is used and it doesn't seem like it's particularly necessary. There's a bunch of type checks such as c.ConstraintType == descpb.ConstraintToUpdate_FOREIGN_KEY which can (or rather, should) be rewritten as c.AsForeignKey() != nil and there's a format string parameter which could be replaced by a %T or something.
| ) | ||
| } else { | ||
| return errors.AssertionFailedf("validation of unsupported constraint type") | ||
| } |
There was a problem hiding this comment.
very minor nit: is it possible to switch constraint.(type) here instead? I'm not actually sure, in any case these if-elses feel kinda clunky.
There was a problem hiding this comment.
I don't think that's possible bc constraint.(type) will be *tabledesc.checkConstraint, *tabledesc.foreignKeyConstraint, etc., all of which are unexported.
FYI you can case catalog.CheckConstraint: etc but this is fine too of course.
| return errors.AssertionFailedf("foreign key with ID %d not found in origin table %q (%d)", | ||
| op.ConstraintID, out.GetName(), out.GetID()) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
nit: fyi you could probably do away with found and break etc. with a defer, same applies to the ops below; not sure if it's worth it though. This is fine btw.
| @@ -17,6 +17,7 @@ go_library( | |||
| deps = [ | |||
| "//pkg/sql/catalog/catpb", # keep | |||
| "//pkg/sql/sem/catid", # keep | |||
| "//pkg/sql/sem/tree", # keep | |||
There was a problem hiding this comment.
No, please don't import tree! In fact, the enums you need are already in catpb, use those instead.
| if originalTableNamespaceElem.DatabaseID != referencedTableNamespaceElem.DatabaseID { | ||
| // TODO (xiang): Refactor new schema changer to be able to retrieve cluster settings (sql.allowCrossDatabaseFKs) | ||
| // here in the builder. | ||
| panic(scerrors.NotImplementedErrorf(t, "foreign key references cross databases are not supported yet")) |
There was a problem hiding this comment.
Cross-DB references are a deprecated feature, effectively already unsupported, so you can do away with this and simply throw a regular error.
| if e.ColumnID == columnID { | ||
| columnDefaultExpression = e | ||
| } | ||
| }) |
There was a problem hiding this comment.
Don't we have filters which filter element result sets by attribute value? Seems like you could use an existing one or define a new one.
| func mustRetrieveNamespaceElem(b BuildCtx, tableID catid.DescID) *scpb.Namespace { | ||
| _, _, ns := scpb.FindNamespace(b.QueryByID(tableID)) | ||
| if ns == nil { | ||
| panic(errors.AssertionFailedf("programming error: cannot find a Namespace element for table %v", tableID)) |
There was a problem hiding this comment.
This is overly defensive. If the table exists then scdecomp will always produce a namespace element.
|
Should I wait for CI to pass before doing another review pass? |
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
2 times, most recently
from
56c1e88 to
75a2d63
Compare
|
@postamar Sorry for the delayed response. I was trying to make the first commit work where I did a little bit of refactoring work. What I essentially did is that I pulled the FkReferenceActio and FKCompositeMatchMethod from catpb and descpb into a new proto file (called
Yes, while the CI is running and I'm actively on top of it in case it fails, please give me another round of review when you get a chance! Thank you very much! |
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
from
75a2d63 to
2d1c50d
Compare
|
Looks like the build is broken, |
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
2 times, most recently
from
7872aa3 to
12eac6b
Compare
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
4 times, most recently
from
546024a to
9f490c8
Compare
|
@postamar I'm glad that I finally made the CI green again. I spent some time trying to separate some of the enum msg definitions (written in proto2) into a new file (written in proto3). It will be a low-level package so package |
| ) | ||
| } else { | ||
| return errors.AssertionFailedf("validation of unsupported constraint type") | ||
| } |
There was a problem hiding this comment.
I don't think that's possible bc constraint.(type) will be *tabledesc.checkConstraint, *tabledesc.foreignKeyConstraint, etc., all of which are unexported.
FYI you can case catalog.CheckConstraint: etc but this is fine too of course.
| ForeignKeyConstraint : OnUpdateAction | ||
| ForeignKeyConstraint : OnDeleteAction | ||
| ForeignKeyConstraint : CompositeKeyMatchMethod | ||
| ForeignKeyConstraint : IndexIDForValidation |
There was a problem hiding this comment.
nit: this change seems like it doesn't belong in this commit (presently ee82237)
| @@ -321,6 +321,8 @@ message UniqueWithoutIndexConstraint { | |||
| uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; | |||
| uint32 constraint_id = 2 [(gogoproto.customname) = "ConstraintID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ConstraintID"]; | |||
| repeated uint32 column_ids = 3 [(gogoproto.customname) = "ColumnIDs", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"]; | |||
| // Predicate, if non-nil, means a partial uniqueness constraint. | |||
| Expression predicate = 4 [(gogoproto.customname) = "Predicate"]; | |||
There was a problem hiding this comment.
This doesn't seem like it should be in commit b4eabaf
| // 5. Resolve `t.(*ForeignKeyConstraintTableDef).Table` (i.e. referenced table name) and check whether it's in the same | ||
| // database as the originTable (i.e. tbl). Cross database FK references is a deprecated feature and is in practice | ||
| // no longer supported. We will return an error here directly. | ||
| referencedTableID := mustGetTableIDFromTableName(b, fkDef.Table) |
There was a problem hiding this comment.
mustGetTableIDFromTableName requires privilege.CREATE to resolve the table, which seems odd for a table referenced in a FK. Can you double-check that this is the existing behaviour, please?
|
|
||
| // 5. Resolve `t.(*ForeignKeyConstraintTableDef).Table` (i.e. referenced table name) and check whether it's in the same | ||
| // database as the originTable (i.e. tbl). Cross database FK references is a deprecated feature and is in practice | ||
| // no longer supported. We will return an error here directly. |
There was a problem hiding this comment.
style nit: Comments shouldn't go beyond the 80th column. Applies to other comments in this file also.
| originalTableNamespaceElem := mustRetrieveNamespaceElem(b, tbl.TableID) | ||
| referencedTableNamespaceElem := mustRetrieveNamespaceElem(b, referencedTableID) | ||
| if originalTableNamespaceElem.DatabaseID != referencedTableNamespaceElem.DatabaseID { | ||
| panic(scerrors.NotImplementedErrorf(t, "cross DB FK reference is a deprecating feature and won't be supported"+ |
There was a problem hiding this comment.
s/deprecating/deprecated/
s/won't be supported in declarative schema changer./is no longer supported/
|
|
||
| // 7. Compute referencedColumnNames, which is usually provided in `t.(*ForeignKeyConstraintTableDef).ToCols`. But if | ||
| // it's empty, then we attempt to add this FK on the PK of the referenced table, excluding implicit columns. | ||
| if fkDef.ToCols == nil || len(fkDef.ToCols) == 0 { |
There was a problem hiding this comment.
just len(fkDef.ToCols) == 0
| } | ||
| } | ||
|
|
||
| // 10. Check the name of this to-be-added FK constraint hasn't been used; Or, give it one if no name is specified. |
| )) | ||
| } | ||
|
|
||
| // 12. (Finally!) Add a ForeignKey_Constraint, ConstraintName element to builder state. |
There was a problem hiding this comment.
Thanks these comments are very helpful!
| @@ -178,6 +178,7 @@ var elementSchemaOptions = []rel.SchemaOption{ | |||
| rel.EntityAttr(DescID, "TableID"), | |||
| rel.EntityAttr(ReferencedDescID, "ReferencedTableID"), | |||
| rel.EntityAttr(ConstraintID, "ConstraintID"), | |||
| rel.EntityAttr(IndexID, "IndexIDForValidation"), | |||
There was a problem hiding this comment.
I just noticed something: is this actually needed, can you check? if not, please remove this binding.
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
from
9f490c8 to
d1b334c
Compare
There was a problem hiding this comment.
Thanks for the review! I've addressed the feedback and will merge it once CI is green.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @postamar)
pkg/sql/backfill.go line 1586 at r2 (raw file):
Previously, postamar (Marius Posta) wrote…
I don't think that's possible bc constraint.(type) will be *tabledesc.checkConstraint, *tabledesc.foreignKeyConstraint, etc., all of which are unexported.
FYI you can
case catalog.CheckConstraint:etc but this is fine too of course.
Done.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go line 227 at r34 (raw file):
Previously, postamar (Marius Posta) wrote…
style nit: Comments shouldn't go beyond the 80th column. Applies to other comments in this file also.
Done
Code quote:
originTable (i.e. tbl). Cross database FK referpkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go line 228 at r34 (raw file):
Previously, postamar (Marius Posta) wrote…
mustGetTableIDFromTableNamerequiresprivilege.CREATEto resolve the table, which seems odd for a table referenced in a FK. Can you double-check that this is the existing behaviour, please?
I don't think there is an existing behaviour per se; It's just that we need to know a table ID from its name here, and the only utility we have in the builder is ResolveTable which requires a privilege. I put privilege.CREATE here bc ADD FOREIGN KEY requires CREATE privilege.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go line 232 at r34 (raw file):
Previously, postamar (Marius Posta) wrote…
s/deprecating/deprecated/
s/won't be supported in declarative schema changer./is no longer supported/
Done.
Code quote:
ference is a deppkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go line 255 at r34 (raw file):
Previously, postamar (Marius Posta) wrote…
just
len(fkDef.ToCols) == 0
done
Code quote:
f.ToCols == nil || len(fkDef.ToCopkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go line 310 at r34 (raw file):
Previously, postamar (Marius Posta) wrote…
s/Check/Check that/
Done.
Code quote:
eck the name of thispkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go line 336 at r34 (raw file):
Previously, postamar (Marius Posta) wrote…
Thanks these comments are very helpful!
Good to hear!
pkg/sql/schemachanger/scpb/elements.proto line 325 at r34 (raw file):
Previously, postamar (Marius Posta) wrote…
This doesn't seem like it should be in commit b4eabaf
This I believe it does! When implementing ADD FOREIGN KEY in the builder, we need to ensure that the referenced table guarantees uniqueness on the referenced columns. One way to guarantee uniqueness is via a non-partial unique_without_index constraint on the referenced columns.
pkg/sql/schemachanger/screl/attr.go line 181 at r36 (raw file):
Previously, postamar (Marius Posta) wrote…
I just noticed something: is this actually needed, can you check? if not, please remove this binding.
Yes, I think so. Just like CheckConstraint, it's possible that a FK constraint validation needs to perform on a new primary index that's yet not public. Having this rel attribute allows the dep-rule "don't validate until the index is backfilled" to apply to FK constraints.
pkg/sql/logictest/testdata/logic_test/fk line 2719 at r18 (raw file):
Previously, postamar (Marius Posta) wrote…
Use a regexp that matches both schema changer implementations.
Done.
Previously, we had enum protobuf in catpb for FK reference actions but written in proto2, making it impossible to re-use in other proto3 files (e.g. scpb). This commit solved this problem by moving the following two enum definitions into a new package `sql/sem/semenumpb/constraint.proto`, written in proto3: - catpb.proto: ForeignKeyAction - descpb.proto: ForeignKeyConstraint_Match The idea of this package is that it will be imported by tree and possibly other XXpb packages, so that all the other XXpb packages do not need to import the heavy tree package. It also did some cleanup work to move some other enum definitions out of `catpb` into a new package `sql/catalog/catpb/enum.proto`, also in proto3, for future use: - catpb.proto: SystemColumnKind - catpb.proto: InvertedIndexColumnKind The general rule of thumb to determine which proto file an enum should go to is this: If it's a concept that is visible to the SQL layer (e.g. ON UPDATE, MATCH, etc. that has keyword and can be configured in a SQL query) should go to `sql/sem/semenumpb/xxx.proto`. Other concepts that are internal (e.g. SystemColumnKind) should be put in `sql/catalog/catpb/enum.proto`.
This commit refactored some of the check constraint validation code to implement a unified function for validating all kinds of constraints.
Xiang-Gu
force-pushed
the
implement-add-constraint-foreign-key
branch
from
d1b334c to
d05c969
Compare
This PR implements
ADD CONSTRAINT ... FOREIGN KEYin the declarative schema changer.One notable change is the behavioral difference compared to legacy schema changer: we only
add the FK constraint onto the mutation slice, and when it's finally ready to get published after
validation succeeded, we remove it off the mutation slice, add it to
OutboundFKsslice,and add the back-reference in the referenced table's
InboundFKsslice.See each commit message for more details.
Epic: None