-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
executor: fill extra partition ID column in UnionScan executor #28666
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3089,3 +3089,83 @@ func (s *partitionTableSuite) TestIssue26251(c *C) { | |
c.Fail() | ||
} | ||
} | ||
|
||
func (s *partitionTableSuite) TestLeftJoinForUpdate(c *C) { | ||
tk1 := testkit.NewTestKit(c, s.store) | ||
tk1.MustExec("use test") | ||
tk2 := testkit.NewTestKit(c, s.store) | ||
tk2.MustExec("use test") | ||
tk3 := testkit.NewTestKit(c, s.store) | ||
tk3.MustExec("use test") | ||
|
||
tk1.MustExec("drop table if exists nt, pt") | ||
tk1.MustExec("create table nt (id int, col varchar(32), primary key (id))") | ||
tk1.MustExec("create table pt (id int, col varchar(32), primary key (id)) partition by hash(id) partitions 4") | ||
|
||
resetData := func() { | ||
tk1.MustExec("truncate table nt") | ||
tk1.MustExec("truncate table pt") | ||
tk1.MustExec("insert into nt values (1, 'hello')") | ||
tk1.MustExec("insert into pt values (2, 'test')") | ||
} | ||
|
||
// ========================== First round of test ================== | ||
// partition table left join normal table. | ||
// ================================================================= | ||
resetData() | ||
ch := make(chan int, 10) | ||
tk1.MustExec("begin pessimistic") | ||
// No union scan | ||
tk1.MustQuery("select * from pt left join nt on pt.id = nt.id for update").Check(testkit.Rows("2 test <nil> <nil>")) | ||
go func() { | ||
// Check the key is locked. | ||
tk2.MustExec("update pt set col = 'xxx' where id = 2") | ||
ch <- 2 | ||
}() | ||
|
||
// Union scan | ||
tk1.MustExec("insert into pt values (1, 'world')") | ||
tk1.MustQuery("select * from pt left join nt on pt.id = nt.id for update").Sort().Check(testkit.Rows("1 world 1 hello", "2 test <nil> <nil>")) | ||
go func() { | ||
// Check the key is locked. | ||
tk3.MustExec("update nt set col = 'yyy' where id = 1") | ||
ch <- 3 | ||
}() | ||
|
||
// Give chance for the goroutines to run first. | ||
time.Sleep(80 * time.Millisecond) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be unstable in the CI environment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be stable. 2, 3 blocked by 1 means the partition pessimistic lock works as expected, the partition key is constructed correctly. |
||
ch <- 1 | ||
tk1.MustExec("rollback") | ||
|
||
checkOrder := func() { | ||
c.Assert(<-ch, Equals, 1) | ||
v1 := <-ch | ||
v2 := <-ch | ||
c.Assert((v1 == 2 && v2 == 3) || (v1 == 3 && v2 == 2), IsTrue) | ||
} | ||
checkOrder() | ||
|
||
// ========================== Another round of test ================== | ||
// normal table left join partition table. | ||
// =================================================================== | ||
resetData() | ||
tk1.MustExec("begin pessimistic") | ||
// No union scan | ||
tk1.MustQuery("select * from nt left join pt on pt.id = nt.id for update").Check(testkit.Rows("1 hello <nil> <nil>")) | ||
|
||
// Union scan | ||
tk1.MustExec("insert into pt values (1, 'world')") | ||
tk1.MustQuery("select * from nt left join pt on pt.id = nt.id for update").Check(testkit.Rows("1 hello 1 world")) | ||
go func() { | ||
tk2.MustExec("replace into pt values (1, 'aaa')") | ||
ch <- 2 | ||
}() | ||
go func() { | ||
tk3.MustExec("update nt set col = 'bbb' where id = 1") | ||
ch <- 3 | ||
}() | ||
time.Sleep(80 * time.Millisecond) | ||
ch <- 1 | ||
tk1.MustExec("rollback") | ||
checkOrder() | ||
} |
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.
There could be some unexpected errors if the
physicalID
is zero and this condition is a bit confusing. Do we have some other ways do check the left join result situation?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.
Yes, I agree it's confusing here.
physicalID == 0
may be casue by left join, or maybe it's caused by bugs.Distinguish those cases is unrealistic, because left join have several implementations: hash join / merge join / nest loop join / index join etc... and left join is one of the case we found (that will generate empty or null row), there might be other cases that fill empty row ... It's hard to find out all.
So ... let's look a step back.
In the past, we have bug for lock on partition ... (that's bad)
Then, we fix it ... #14921
Then, we find more bug (that's bad)
Then, we try to fix it ... #21148
And the the solution caused more serious problems and introduced more critical bugs ... (wow! worse)
After change, we come back from worse to bad, that's a big progress!
I mean, we fix some problems and make the solution (at least) not bad than before.
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.
@qw4990
Do you have any ideas about this?
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 thing I'm worried about is though the former bugs will make the query panic it will not have future impact on the data in storage. If we could not verify which is expected in some write statement, there could be some wrong data writting into storage, just like the issue listed above an invalid key is locked and the lock record is persisted.
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.
Yes... I have the same worries that bugs break the data.
If there are some better ways to fix this problem, I'd like to choose that solution. But I can't come up with better ideas.
So we have to fix the current problem and add tests to cover more scenarios.
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's quite necessary to add more tests by now seems there could be more unknown issues. BTW do we have bandwidth for the coverage enhancement or our QA team?
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.
Can we let all
OuterJoins
set this column to a specified value (e.g. -1) explicitly when mismatching?Then we can define
pid=0
as the uninitialized state and we know it must be caused by some bug;And then we can return an error like
pid is not uninitialized
in this case.We don't have to find all
OuterJoins
at once; We can find them by our best effort this time, and then just wait for theuninitialized
error and fix them.