-
Notifications
You must be signed in to change notification settings - Fork 6k
executor: fix the issue that INSERT IGNORE
doesn't lock the parent table
#56661
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
Conversation
|
||
func TestLockKeysInInsertIgnore(t *testing.T) { | ||
// FIXME: this test will fail in the unistore. After fixing the issue in unistore, please move | ||
// this test to `foreign_key_test.go`. |
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 still don't know why this case will fail in the unistore. Maybe the transaction implementation has some bugs (or just different behaviors).
I tried this case with/without IGNORE
. It seems that the foreign key behaves strangely in unistore 🤔 :
Prepare:
create table parent (id int primary key);
create table child(id int primary key, ref int, foreign key (ref) references parent(id));
insert into parent values (1);
Execute:
Transaction 1 | Transaction 2 |
---|---|
BEGIN; | BEGIN; |
INSERT INTO child VALUES (1, 1); | |
UPDATE parent SET id = 2 WHERE id = 1 | |
COMMIT; | |
COMMIT; |
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 record this issue in #56663. It's confirmed as a transaction bug in unistore.
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.
It'll be fixed in #56821
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #56661 +/- ##
=================================================
- Coverage 73.2689% 56.2956% -16.9733%
=================================================
Files 1650 1776 +126
Lines 455646 634548 +178902
=================================================
+ Hits 333847 357223 +23376
- Misses 101279 253233 +151954
- Partials 20520 24092 +3572
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/retest |
7f2ab9a
to
8e9e9ee
Compare
INSERT IGNORE
doesn't lock the parent tableINSERT IGNORE
doesn't lock the parent table
/retest |
This test failure cannot be reproduced locally 🤔 . Investigating. |
/retest |
/retest |
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, crazycs520 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
/retest |
1 similar comment
/retest |
b7807d3
to
7aeba7b
Compare
/retest |
Signed-off-by: Yang Keao <[email protected]>
What problem does this PR solve?
Issue Number: close #56655
Problem Summary:
INSERT IGNORE
cannot lock the rows in parent, so the constraint may be broken.What changed and how does it work?
Try to lock the rows even when the
toBeCheckedKeys
andtoBeCheckedPrefixKeys
are empty.Check List
Tests
Release note