opt: add support for DELETE FK checks #39120
Conversation
|
This change is |
There was a problem hiding this comment.
Nice, that was fast! I only skimmed it (and it looks good!), I will take a more in-depth look on Monday.
I was aware that the code will need some restructuring but I didn't have a specific plan. When we do UPDATE we'll want to reuse most of the logic from insert and delete. We may also want to move the FK part to its own file.
We should add some tests with multiple column families (e.g. each column in a separate family), not entirely sure if those affect anything for deletes. I'm especially interested if there are cases where we wouldn't have scanned a column if it wasn't for a FK reference (not sure if it can happen, if we have an inbound FK we must have a unique index, and then we would need those columns to delete from the index).
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status:
pkg/sql/opt/optbuilder/testdata/fk-checks-delete, line 91 at r1 (raw file):
├── columns: p:12(int!null) ├── with-scan &1 │ ├── columns: p:12(int!null)
why is this not scanning other?
pkg/sql/opt/testutils/testcat/create_table.go, line 353 at r1 (raw file):
// pair is a one-to-one function from pairs of integers to integers. See // https://en.wikipedia.org/wiki/Pairing_function.
cool!
There was a problem hiding this comment.
Nice work!
Reviewable status:
pkg/sql/opt_catalog.go, line 1230 at r1 (raw file):
func (fk *optForeignKeyConstraint) ColumnCount() int { if fk.numCols == 0 { panic(errors.AssertionFailedf("cannot get column count from inbound FK constraint"))
Won't this cause crashes when we try to display the catalog (e.g. the execbuild catalog file I added)? Might need to temporarily adjust formatCatalogFKRef in cat/utils.go
pkg/sql/opt/optbuilder/mutation_builder.go, line 962 at r1 (raw file):
} inputCols := make(opt.ColList, numCols)
This is getting pretty confusing. We should at least have a comment here, since "input" is pretty generic. Perhaps better would be to have a little ascii art drawing of the plan we are building, putting things like inputCols in the corresponding box in the plan. Or maybe just renaming, e.g. deletedValuesCols
Also, this is only used to pass to makeFKInputScan so we might as well move it right before that call (maybe having the "scan" in-between is what was confusing to me)
pkg/sql/opt/optbuilder/mutation_builder.go, line 964 at r1 (raw file):
inputCols := make(opt.ColList, numCols) for j := 0; j < numCols; j++ { ord := fk.OriginColumnOrdinal(origTab, j)
should this be ReferencedColumnOrdinal?
pkg/sql/opt/testutils/testcat/create_table.go, line 353 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
cool!
I agree! Too bad it's temporary..
justinj
force-pushed
the
fk-delete
branch
2 times, most recently
from
afa792f
to
b4711dd
Compare
|
One more thing - I think we should check if the FK has a CASCADE, and not plan FKs in that case (to let the existing code handle it). |
|
How do you feel about that being a follow-up? I think that's part of a larger change, since it requires changing the FK interface and also probably threading a "was this FK check handled" flag up to execution? |
|
Aha, I missed some stuff in getting this wired up properly, hang on, fixing |
|
OK, fixed a bunch of stuff—PTAL when you get a chance! |
Sure. Note that for insert, "was this FK check handled" thing is there - if the |
There was a problem hiding this comment.
Oh, you already did all that.
Reviewable status:
There was a problem hiding this comment.
Reviewed 13 of 13 files at r2.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk_opt, line 67 at r2 (raw file):
DELETE FROM parent WHERE p = 2 statement error update or delete on table "parent" violates foreign key constraint on table "child"
is it easy to specify either update or delete instead of both here?
pkg/sql/sqlbase/structured.go, line 2994 at r2 (raw file):
// ForeignKeyReferenceActionType allows the conversion between a // tree.ReferenceAction and a ForeignKeyReference_Action.
did you mean to swap these two?
justinj
force-pushed
the
fk-delete
branch
2 times, most recently
from
4215de7
to
ad7fe77
Compare
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/sql/opt_catalog.go, line 1230 at r1 (raw file):
Previously, RaduBerinde wrote…
Won't this cause crashes when we try to display the catalog (e.g. the execbuild
catalogfile I added)? Might need to temporarily adjustformatCatalogFKRefincat/utils.go
Ended up removing this
pkg/sql/logictest/testdata/logic_test/fk_opt, line 67 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
is it easy to specify either update or delete instead of both here?
I think we can stuff it in the Check without too much trouble—I'll remove the or for now and make that change when we add update
pkg/sql/opt/optbuilder/mutation_builder.go, line 962 at r1 (raw file):
Previously, RaduBerinde wrote…
This is getting pretty confusing. We should at least have a comment here, since "input" is pretty generic. Perhaps better would be to have a little ascii art drawing of the plan we are building, putting things like
inputColsin the corresponding box in the plan. Or maybe just renaming, e.g.deletedValuesColsAlso, this is only used to pass to
makeFKInputScanso we might as well move it right before that call (maybe having the "scan" in-between is what was confusing to me)
Moved stuff around a bit—better now?
pkg/sql/opt/optbuilder/mutation_builder.go, line 964 at r1 (raw file):
Previously, RaduBerinde wrote…
should this be
ReferencedColumnOrdinal?
Yes, good catch! Didn't realize that old-style FK checks were still running, which is why this seemed to pass (will look into disabling them)
pkg/sql/opt/optbuilder/testdata/fk-checks-delete, line 91 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
why is this not scanning
other?
It was wrong! Good catch.
pkg/sql/sqlbase/structured.go, line 2994 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
did you mean to swap these two?
Done.
Build failed |
This commit adds FK checks for DELETE statements. It's very structurally similar to INSERT, I tried to abstract things to avoid having the duplicated structure here but there were enough differences between the two (referring to the Referenced table vs. the Origin table, etc) that it felt to me as though it was clearer to just extract some helpers where possible. We also introduce an ID field to the foreign key interface so that we can work around the fact that inbound FK descriptors do not contain the number of columns participating in the constraint, it's my hope that this method can be deleted when the descriptors are unified. Release note: None
|
Needed a rebase bors r+ |
39120: opt: add support for DELETE FK checks r=justinj a=justinj This commit adds FK checks for DELETE statements. It's very structurally similar to INSERT, I tried to abstract things to avoid having the duplicated structure here but there were enough differences between the two (referring to the Referenced table vs. the Origin table, etc) that it felt to me as though it was clearer to just extract some helpers where possible. @RaduBerinde The existing structure in the method looked to me like you were planning for it to be more unified than this so if you have thoughts on what that might look like I'm interested to hear them :) We also introduce an ID field to the foreign key interface so that we can work around the fact that inbound FK descriptors do not contain the number of columns participating in the constraint, it's my hope that this method can be deleted when the descriptors are unified. Release note: None Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
Build succeeded |
This commit adds FK checks for DELETE statements. It's very structurally
similar to INSERT, I tried to abstract things to avoid having the
duplicated structure here but there were enough differences between the
two (referring to the Referenced table vs. the Origin table, etc) that
it felt to me as though it was clearer to just extract some helpers
where possible.
@RaduBerinde The existing structure in the method looked to me like you were planning for it to be more unified than this so if you have thoughts on what that might look like I'm interested to hear them :)
We also introduce an ID field to the foreign key interface so that we
can work around the fact that inbound FK descriptors do not contain the
number of columns participating in the constraint, it's my hope that
this method can be deleted when the descriptors are unified.
Release note: None