-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix Foreign key fuzzer to ignore rows affected #15841
Conversation
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15841 +/- ##
==========================================
+ Coverage 68.40% 68.42% +0.01%
==========================================
Files 1556 1559 +3
Lines 195121 196511 +1390
==========================================
+ Hits 133479 134460 +981
- Misses 61642 62051 +409 ☔ View full report in Codecov by Sentry. |
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.
This is a nice refactoring, looks good to me
Description
After noticing the problem #15826 and running into the issues described in #15779 (comment) and #15779 (comment), the Vitess maintainers have decided not to fix the issue, and instead let the rows affected fields for Vitess and MySQL differ. The reasoning is that all the users of MySQL with foreign keys, should already be accustomed to having the rows affected being different from the total number of row deletions (because MySQL doesn't count the rows deleted via CASCADEs as they happen at the InnoDB level). Moreover, it will take significant code changes and performance penalty to match the MySQL behaviour. We would have to do multiple SELECT queries to find the number of qualifying rows for a DML operation upfront. This however, doesn't feel like the prudent way forward. Instead we are fixing the fuzzer to ignore the rows affected fields while testing and adding a test case for the queries listed in the bug to ensure everything else works.
Related Issue(s)
Checklist
Deployment Notes