opt: add FK checks for UPDATE statements #39751
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 21 of 21 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @justinj and @RaduBerinde)
pkg/sql/opt_catalog.go, line 597 at r1 (raw file):
deleteAction: fk.OnDelete, updateAction: fk.OnUpdate, id: cat.StableID(pair(int(fk.Index), int(idxDesc.ID))),
did you mean for these elements in the pair to be in the opposite order as the one above?
(and is this id field even being used anymore)?
pkg/sql/logictest/testdata/logic_test/fk_opt, line 173 at r1 (raw file):
# This case illustrates why it's insufficient to just check the "old" values, # even in the absence of temporary unique constraint violations.
I don't understand this case....
pkg/sql/opt/optbuilder/mutation_builder.go, line 932 at r1 (raw file):
deleteCols := make(opt.ColList, fk.ColumnCount()) for i := range deleteCols {
[nit] i is shadowing the outer variable
pkg/sql/opt/optbuilder/mutation_builder.go, line 948 at r1 (raw file):
input, cols := mb.makeFKInputScan(deletedFKCols) abort := mb.addDeletionCheck(i, input, cols, fk.DeleteReferenceAction())
it's a bit odd to return "abort" as a bool -- seems more idiomatic to return "ok" and check for "!ok"
pkg/sql/opt/optbuilder/mutation_builder.go, line 974 at r1 (raw file):
oldRowCols := mb.fetchOrds // newRowCols is an array mapping column ordinals in each the effective new
each the -> each of the
pkg/sql/opt/optbuilder/mutation_builder.go, line 1022 at r1 (raw file):
numCols := fk.ColumnCount() oldRowFKColOrdinals := make([]scopeOrdinal, fk.ColumnCount())
can use numCols here
pkg/sql/opt/optbuilder/mutation_builder.go, line 1024 at r1 (raw file):
oldRowFKColOrdinals := make([]scopeOrdinal, fk.ColumnCount()) newRowFKColOrdinals := make([]scopeOrdinal, fk.ColumnCount()) for i := 0; i < numCols; i++ {
[nit] shadowing outer variable
pkg/sql/opt/optbuilder/mutation_builder.go, line 1025 at r1 (raw file):
newRowFKColOrdinals := make([]scopeOrdinal, fk.ColumnCount()) for i := 0; i < numCols; i++ { oldRowFKColOrdinals[i] = oldRowCols[fk.ReferencedColumnOrdinal(mb.tab, i)]
you could save this in a variable to avoid calling it twice
pkg/sql/opt/optbuilder/mutation_builder.go, line 1053 at r1 (raw file):
outCols := make(opt.ColList, len(deletedFKCols)) proj := memo.ProjectionsExpr{} for i := 0; i < len(deletedFKCols); i++ {
[nit] shadowing
pkg/sql/opt/optbuilder/mutation_builder.go, line 1075 at r1 (raw file):
// addInsertionCheck adds a FK check for rows which are added to a table. // The input is the input to the mutation operator, and the insertCols is a
What input? I just see fkOrdinal
pkg/sql/opt/optbuilder/mutation_builder.go, line 1076 at r1 (raw file):
// addInsertionCheck adds a FK check for rows which are added to a table. // The input is the input to the mutation operator, and the insertCols is a // of the columns for the rows being inserted, indexed by their ordinal
a list of?
pkg/sql/opt/optbuilder/mutation_builder.go, line 1083 at r1 (raw file):
var notNullInputCols opt.ColSet insertedFKCols := make(opt.ColList, numCols) for j := 0; j < numCols; j++ {
[nit] this can be i instead of j
pkg/sql/opt/optbuilder/testdata/fk-checks-update, line 73 at r1 (raw file):
│ └── variable: parent.p [type=int] └── f-k-checks-item: grandchild(c) -> child(c) └── semi-join (hash)
Is this check necessary since we're not updating c?
There was a problem hiding this comment.
Really cool to see this, and it looks less complicated than I expected.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk_opt, line 173 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I don't understand this case....
Each input row to the update contains both the new and the old value. In most cases you can assume that the old value is being removed (unless you're in one of the questionable cases above where different rows interact with each other). But in this case the old and the new value are the same.
Nice case Justin!
pkg/sql/opt/ops/mutation.opt, line 227 at r1 (raw file):
# CreatorOp is the operator that generated this check, used to provide # better error messages. CreatorOp Operator
I'm not a fan of using opt operators as clues to what the SQL statement in an error should be. In fact, I have a branch where I remove mutationBuilder.op because I find it confusing - it's not always the op that ends up being built. I am replacing it with a statementTag string (which is usually the SQL StatementTag(), modulo some exception with UPSERT).
I think this field should similarly be a string too.
I was waiting for this PR to go in first so that I don't cause conflicts, but if it helps I can get that change out right now.
pkg/sql/opt/optbuilder/mutation_builder.go, line 966 at r1 (raw file):
// Update we can emit both semi-joins and anti-joins. // The input to the Update operator consists of both the existing values for
[nit] could be more clear, eg "Each row input to the Update operator contains both the existing and the new value for each updated column"
pkg/sql/opt/optbuilder/mutation_builder.go, line 1034 at r1 (raw file):
// of being updated _from_, minus the ones that were "added" by virtue of // being updated _to_. deletions := mb.b.factory.ConstructExcept(
Maybe mention that this could also be ExceptAll, it's not 100% clear to me which would be better in practice.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1216 at r1 (raw file):
} func (mb *mutationBuilder) addDeletionCheck(
[nit] a comment would help
pkg/sql/opt/optbuilder/mutation_builder.go, line 1286 at r1 (raw file):
} func (mb *mutationBuilder) projectOrdinals(
This definitely needs a comment
pkg/sql/opt/optbuilder/testdata/fk-checks-update, line 232 at r1 (raw file):
└── eq [type=bool] ├── variable: c [type=int] └── variable: grandchild2.c [type=int]
We need more tests here from a bunch of angles:
-
cases like the one above where we don't want to check FKs that aren't being modified
-
multi-column FKs, where only some of the columns are updated, including cases where the columns are in different families. By the way, we have an interesting TODO here: I can't find any code that figures out that if you modify column
aand you have an outgoing FK on(a,b)you also need to scanb; I believe this currently happens automatically because we always have an index ona,b, but when we remove that requirement this won't be the case anymore. This can wait until 19.2 but it should be spelled out somewhere. -
a self-referencing FK
justinj
force-pushed
the
fk-update
branch
3 times, most recently
from
da29b9a
to
712f48f
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt_catalog.go, line 597 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
did you mean for these elements in the pair to be in the opposite order as the one above?
(and is this id field even being used anymore)?
I think this is the right order—they're both in the order of (referencing, referenced), but this isn't necessary any more with the descriptor changes.
pkg/sql/logictest/testdata/logic_test/fk_opt, line 173 at r1 (raw file):
Previously, RaduBerinde wrote…
Each input row to the update contains both the new and the old value. In most cases you can assume that the old value is being removed (unless you're in one of the questionable cases above where different rows interact with each other). But in this case the old and the new value are the same.
Nice case Justin!
Thanks! I elaborated on the comment a bit.
pkg/sql/opt/ops/mutation.opt, line 227 at r1 (raw file):
Previously, RaduBerinde wrote…
I'm not a fan of using opt operators as clues to what the SQL statement in an error should be. In fact, I have a branch where I remove
mutationBuilder.opbecause I find it confusing - it's not always the op that ends up being built. I am replacing it with astatementTagstring (which is usually the SQLStatementTag(), modulo some exception withUPSERT).I think this field should similarly be a string too.
I was waiting for this PR to go in first so that I don't cause conflicts, but if it helps I can get that change out right now.
Hm, ok, in the meantime I guess I'll leave this and then we can rebase your change onto it? Unsure what the best intermediate change would be.
pkg/sql/opt/optbuilder/mutation_builder.go, line 932 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit]
iis shadowing the outer variable
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 948 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
it's a bit odd to return "abort" as a bool -- seems more idiomatic to return "ok" and check for "!ok"
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 966 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] could be more clear, eg "Each row input to the Update operator contains both the existing and the new value for each updated column"
Yeah that's much better! Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 974 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
each the -> each of the
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1022 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
can use
numColshere
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1024 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] shadowing outer variable
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1025 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
you could save this in a variable to avoid calling it twice
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1034 at r1 (raw file):
Previously, RaduBerinde wrote…
Maybe mention that this could also be
ExceptAll, it's not 100% clear to me which would be better in practice.
Interesting point. I guess in general if we were excepting on a key in some circumstances we could normalize ExceptAll to Except, (or vice versa) if we decided that one was preferable to the other.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1053 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] shadowing
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1075 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
What input? I just see
fkOrdinal
Meant to imply that the input to the check here is the same as the input to the mutation operator, tried to make that clearer.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1076 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
a list of?
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1083 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] this can be
iinstead ofj
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1216 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] a comment would help
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1286 at r1 (raw file):
Previously, RaduBerinde wrote…
This definitely needs a comment
Done.
pkg/sql/opt/optbuilder/testdata/fk-checks-update, line 73 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Is this check necessary since we're not updating
c?
Good call, not necessary! I added some code to not add checks when no relevant columns are updated.
pkg/sql/opt/optbuilder/testdata/fk-checks-update, line 232 at r1 (raw file):
Previously, RaduBerinde wrote…
We need more tests here from a bunch of angles:
cases like the one above where we don't want to check FKs that aren't being modified
multi-column FKs, where only some of the columns are updated, including cases where the columns are in different families. By the way, we have an interesting TODO here: I can't find any code that figures out that if you modify column
aand you have an outgoing FK on(a,b)you also need to scanb; I believe this currently happens automatically because we always have an index ona,b, but when we remove that requirement this won't be the case anymore. This can wait until 19.2 but it should be spelled out somewhere.a self-referencing FK
That's a very interesting case indeed! Added a couple cases that should break when that changes and added a comment.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/ops/mutation.opt, line 227 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Hm, ok, in the meantime I guess I'll leave this and then we can rebase your change onto it? Unsure what the best intermediate change would be.
You could make this field a string in this change (basically move up the case that converts the op to "insert", "delete", or whatever). Either way is fine, I'll rebase my change after yours and figure it out.
There was a problem hiding this comment.
Reviewed 14 of 14 files at r2.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk_opt, line 2796 at r2 (raw file):
# This case illustrates why it's insufficient to just check the "old" values, # even in the absence of temporary unique constraint violations. In this case, # the "new" and "old" values are the same, so we have to remove the new values
[nit] remove -> access?
pkg/sql/opt/optbuilder/mutation_builder.go, line 1231 at r2 (raw file):
} // addDeletionCheck adds a FK check for rows which are removed from a tabel.
tabel -> table
There was a problem hiding this comment.
Reviewable status:
justinj
force-pushed
the
fk-update
branch
3 times, most recently
from
13ed8f4
to
d0d49ba
Compare
There was a problem hiding this comment.
Spent a while trying to understand what was going wrong with the test failure, turned out that disabling FK checks for cascading actions only refers to outbound references, and inbound ones still need to be checked. Turns out to be a simple fix and I just needed to remove one of the ifs I added. There's probably a cleaner way to fit this in but hopefully this code path will be simplified further by the removal of old-style FK checks in the near future. Would appreciate a quick look!
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk_opt, line 2796 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] remove -> access?
tried to make this clearer
pkg/sql/opt/ops/mutation.opt, line 227 at r1 (raw file):
Previously, RaduBerinde wrote…
You could make this field a string in this change (basically move up the
casethat converts the op to "insert", "delete", or whatever). Either way is fine, I'll rebase my change after yours and figure it out.
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1231 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
tabel -> table
Done.
There was a problem hiding this comment.
LGTM (though I'm not particularly familiar with the row/ code)
Reviewable status:
|
Thanks! bors r+ |
There was a problem hiding this comment.
Probably need to rebase on 40214 - will be cool to see the updates to the plans.
Reviewable status:
Build failed (retrying...) |
Build failed (retrying...) |
|
bors r- |
Canceled |
Release note: None
|
Turns out none of the updates require FK checks, the test failure was because we were still buffering the input if we ended up without any checks, which isn't a problem for correctness, but it's a little nicer if we don't, so I've made a change to not buffer up if we end up without checks. bors r+ |
39751: opt: add FK checks for UPDATE statements r=justinj a=justinj Been trying to get this in a reviewable state for a while—still not quite perfect but gotta get it out the door, I think. Going to have to rebase when #39462 goes in to get some better testing coverage. Release note: None Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
There was a problem hiding this comment.
Reviewable status:
Build succeeded |
Been trying to get this in a reviewable state for a while—still not quite perfect but gotta get it out the door, I think. Going to have to rebase when #39462 goes in to get some better testing coverage.
Release note: None