-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Update foreign key handling to be smarter around when to skip foreign key emulation #18618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Update foreign key handling to be smarter around when to skip foreign key emulation #18618
Conversation
…kip) foreign key emulation. Signed-off-by: Arthur Schreiber <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Arthur Schreiber <[email protected]>
…/smarter-fk-handling Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18618 +/- ##
==========================================
- Coverage 67.52% 67.50% -0.02%
==========================================
Files 1608 1608
Lines 263611 263656 +45
==========================================
- Hits 177994 177988 -6
- Misses 85617 85668 +51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
…/smarter-fk-handling Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
harshit-gangal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes looks good.
We should add some self referential fuzzer testing scenario in e2e test.
…/smarter-fk-handling Signed-off-by: Arthur Schreiber <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me, although I'm not sure why we can't passthrough regardless of the action type.
@arthurschreiber please create a feature/enhancement request and link it to this PR. It doesn't have to be much, but describing what the desired improvement is and the use case (you largely did this in the PR description).
Thanks!
| // If the action is other than RESTRICT / NO ACTION / DEFAULT, we need to verify. | ||
| if !childFk.OnUpdate.IsRestrict() { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we care about this part if it's scoped to 1 shard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess because we need to know about all changes made...
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
Description
This improves the foreign key implementation by being more clever about when we can push down foreign key handling to MySQL and when we need to handle things on the vtgate side.
We can completely push down foreign key handling to MySQL for queries that involve foreign keys if all the foreign keys match the following conditions:
RESTRICT(or none has been specified).Related Issue(s)
N/A
Checklist
Deployment Notes