sql: multiple foreign key cascades bug fixes #42624
Conversation
|
This change is |
rohany
force-pushed
the
fk-cascade-bug
branch
from
42fc2a0
to
b907a4a
Compare
rohany
requested review from
BramGruneir,
RaduBerinde,
jordanlewis and
a team
|
I want to get a couple eyes on these changes, as some of the bugs were pretty subtle. |
rohany
added this to Open PRs
in SQL Features (Deprecated - use SQL Experience board)
via automation
There was a problem hiding this comment.
Thank you very much for working on this. It's some very complicated code :(
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @BramGruneir, @jordanlewis, @RaduBerinde, and @rohany)
pkg/sql/row/cascader.go, line 766 at r1 (raw file):
origValues := values.originalValues.At(i) updatedValues := values.updatedValues.At(i) if len(origValues) == len(updatedValues) {
I don't have a good understanding of this code, but this seems sketchy. What if we need to stop the cascade when the lengths aren't different?
But.. I bet this is a net improvement to the status-quo :)
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/updater.go, line 305 at r1 (raw file):
} // Prevent people from using b directly, as it does not get flushed.
This is strange, why don't we just name the argument batch?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/cascader.go, line 766 at r1 (raw file):
Previously, RaduBerinde wrote…
I don't have a good understanding of this code, but this seems sketchy. What if we need to stop the cascade when the lengths aren't different?
But.. I bet this is a net improvement to the status-quo :)
from what I can understand the lengths are different when either a delete happens (on which updatedValues is length 0), or on the original update that starts the cascade. In both cases this check isn't needed
pkg/sql/row/updater.go, line 305 at r1 (raw file):
Previously, RaduBerinde wrote…
This is strange, why don't we just name the argument
batch?
oh thats really big brain -- i'll update that.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/cascader.go, line 766 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
from what I can understand the lengths are different when either a delete happens (on which updatedValues is length 0), or on the original update that starts the cascade. In both cases this check isn't needed
Oh, that's great, please add that info to the comment!
|
pkg/sql/row/cascader.go, line 766 at r1 (raw file): Previously, RaduBerinde wrote…
Is the converse true? If it's a delete or an update that's not the original one, is it guaranteed that the lengths are different? If no, it seems strange than if the lengths happen to match we check something and otherwise we don't.. |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/cascader.go, line 766 at r1 (raw file):
Previously, RaduBerinde wrote…
Is the converse true? If it's a delete or an update that's not the original one, is it guaranteed that the lengths are different? If no, it seems strange than if the lengths happen to match we check something and otherwise we don't..
i believe so. The cascader always reads the full row from the table when performing updates, so if the cascader itself pushes something onto the queue here, then the original row and the updated row have the same length, equal to the number of columns in the table
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/cascader.go, line 766 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
i believe so. The cascader always reads the full row from the table when performing updates, so if the cascader itself pushes something onto the queue here, then the original row and the updated row have the same length, equal to the number of columns in the table
Actually, let me specify this -- i think the lengths are only different in the case of deletions.
rohany
force-pushed
the
fk-cascade-bug
branch
from
b907a4a
to
631c1b4
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/cascader.go, line 766 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
Actually, let me specify this -- i think the lengths are only different in the case of deletions.
Thanks for the insightful question radu! I thought about it more and i think its true that the lengths are always going to be the same. Its just in the case of deletes that the updatedValues row container is nil. I removed the length check there.
pkg/sql/row/updater.go, line 305 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
oh thats really big brain -- i'll update that.
done
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
With some very minor changes.
Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/cascade, line 3771 at r2 (raw file):
3 4 2 2 4 1 2 3 5 2 3 4
Please add some cleanup here (and the earlier subtest). I know it's not required but I tried to make sure that each test was self contained.
pkg/sql/row/cascader.go, line 762 at r2 (raw file):
// We skip this check when values.updatedValues is nil. This happens when a cascade is started // because of a delete, upon which a row container is not created because there is not an // updated value when a row is deleted.
I still think it might be worthwhile to consider adding a mask might make this check significantly faster and not have to run compare so often. This whole check could be a logical and.
I wouldn't put that work in now, since the optimizer should be taken over this code.
Perhaps a todo just in case?
This PR fixes multiple existing foreign key cascade bugs. * Fixes a panic on performing cascade updates on tables with multiple column families. * Fixes a bug where a self referential foreign key constraint with set default would not be maintained on a cascading update. * Fixes a bug where multiple self referential foreign key constraints would cause all the rows in the referenced constraint columns to be set to null or default on a cascading update. The panic was caused by the updater having a mismatch of fetched columns when only certain column families were fetched by the get requset. The first cascade bug was caused by usage of an invalid batch, causing updates made by the updater to not be visible to reads made by the cascader. The final bug was a logic error that was caused by rows being enqueued in the cascader to be updatedeven when there wasn't a change *on the foreign key column that it was being checked for*. This caused rows to be updated when constrained values weren't even changed, leading to cascades that could change large numbers of unintended rows in the table. Fixes cockroachdb#42120. Release note (bug fix): This PR fixes multiple existing bugs: * Fixes a panic on performing cascade updates on tables with multiple column families. * Fixes a bug where a self referential foreign key constraint with set default would not be maintained on a cascading update. * Fixes a bug where multiple self referential foreign key constraints would cause all the rows in the referenced constraint columns to be set to null or default on a cascading update.
rohany
force-pushed
the
fk-cascade-bug
branch
from
631c1b4
to
a31f5af
Compare
|
TFTR! nits have been addressed. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status:
|
bors r+ |
|
@RaduBerinde, i think #42231 needs to be backported as well for me to backport this. |
Build failed (retrying...) |
Build failed (retrying...) |
42624: sql: multiple foreign key cascades bug fixes r=rohany a=rohany This PR fixes multiple existing foreign key cascade bugs. * Fixes a panic on performing cascade updates on tables with multiple column families. * Fixes a bug where a self referential foreign key constraint with set default would not be maintained on a cascading update. * Fixes a bug where multiple self referential foreign key constraints would cause all the rows in the referenced constraint columns to be set to null or default on a cascading update. The panic was caused by the updater having a mismatch of fetched columns when only certain column families were fetched by the get requset. The first cascade bug was caused by usage of an invalid batch, causing updates made by the updater to not be visible to reads made by the cascader. The final bug was a logic error that was caused by rows being enqueued in the cascader to be updatedeven when there wasn't a change *on the foreign key column that it was being checked for*. This caused rows to be updated when constrained values weren't even changed, leading to cascades that could change large numbers of unintended rows in the table. Fixes #42120. Release note (bug fix): This PR fixes multiple existing bugs: * Fixes a panic on performing cascade updates on tables with multiple column families. * Fixes a bug where a self referential foreign key constraint with set default would not be maintained on a cascading update. * Fixes a bug where multiple self referential foreign key constraints would cause all the rows in the referenced constraint columns to be set to null or default on a cascading update. Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Build succeeded |
This PR fixes multiple existing foreign key cascade bugs.
multiple column families.
would not be maintained on a cascading update.
would cause all the rows in the referenced constraint columns
to be set to null or default on a cascading update.
The panic was caused by the updater having a mismatch of
fetched columns when only certain column families were
fetched by the get requset.
The first cascade bug was caused by usage of an invalid batch, causing updates
made by the updater to not be visible to reads made by the cascader.
The final bug was a logic error that was caused by rows being
enqueued in the cascader to be updatedeven when there wasn't a change
on the foreign key column that it was being checked for.
This caused rows to be updated when constrained values
weren't even changed, leading to cascades that could change large numbers
of unintended rows in the table.
Fixes #42120.
Release note (bug fix): This PR fixes multiple existing bugs:
multiple column families.
would not be maintained on a cascading update.
would cause all the rows in the referenced constraint columns
to be set to null or default on a cascading update.