Skip to content
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

prewrite only when keys are not exist #4085

Merged
merged 26 commits into from Feb 19, 2019

Conversation

@zhangjinpeng1987
Copy link
Member

commented Jan 17, 2019

Signed-off-by: zhangjinpeng1987 zhangjinpeng@pingcap.com

What have you changed? (mandatory)

Add support for Insert operation, prewrite will success only when all keys are not exist. With this feature, TiDB doesn't have to use a get/batch get to check if keys are exist when handling insert statement.

What are the type of the changes? (mandatory)

  • New feature

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Refer to a related PR or issue link (optional)

pingcap/kvproto#343 Should be merged first.

Performance test

Sysbench 32 tables, each table has 1000,000 rows, 32 threads insert
cpu-usage
grpc-count

support prewrite only when key not exist
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

@zhangjinpeng1987 zhangjinpeng1987 changed the title [DNM] support prewrite only when keys are not exist [DNM] prewrite only when keys are not exist Jan 17, 2019

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

any performance improved?

@rleungx rleungx added the S: DNM label Jan 17, 2019

zhangjinpeng1987 added some commits Jan 18, 2019

merge master
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
check exist when prewrite
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
@zhangjinpeng1987

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

any performance improved?

@siddontang Please check the performance test result.

zhangjinpeng1987 added some commits Jan 21, 2019

handle preconditon in prewrite
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

zhangjinpeng1987 added some commits Jan 21, 2019

update proto
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
update kvproto
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

@zhangjinpeng1987 zhangjinpeng1987 changed the title [DNM] prewrite only when keys are not exist prewrite only when keys are not exist Jan 23, 2019

@Connor1996
Copy link
Member

left a comment

Seems that it is not covered by any test

zhangjinpeng1987 added some commits Jan 23, 2019

add test for prewrite with exist checking
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
format
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
merge master
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
@zhangjinpeng1987

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

Seems that it is not covered by any test

@Connor1996 Done

@@ -80,7 +80,7 @@ pub fn is_short_value(value: &[u8]) -> bool {

#[derive(Debug, Clone)]
pub enum Mutation {
Put((Key, Value)),
Put((Key, Value, bool /*should not exist*/)),

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 24, 2019

Contributor

() is unnecessary. And struct is preferred in this case.

@ngaut

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

@coocood

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

@zhangjinpeng1987
Why not adding another mutation type like Mutation::Insert instead of adding a boolean field?

@zhangjinpeng1987

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

Why not adding another mutation type like Mutation::Insert instead of adding a boolean field?

After discussing with @tiancaiamao and @coocood , we add an Insert operation to keep Put operation pure. pingcap/kvproto#353

zhangjinpeng1987 added some commits Feb 1, 2019

use insert
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
merge master
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

@zhangjinpeng1987 zhangjinpeng1987 changed the title prewrite only when keys are not exist [DNM]prewrite only when keys are not exist Feb 10, 2019

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

Where is the Update command? @zhangjinpeng1987

@zhangjinpeng1987

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

Where is the Update command?

@tiancaiamao It should be in another PR.

@ngaut ngaut requested a review from disksing Feb 12, 2019

@ngaut

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

PTAL @disksing

zhangjinpeng1987 added some commits Feb 14, 2019

update kvproto
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
update kvproto
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
update kvproto
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
@zhangjinpeng1987

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2019

@zhangjinpeng1987 zhangjinpeng1987 changed the title [DNM]prewrite only when keys are not exist prewrite only when keys are not exist Feb 16, 2019

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

/test

@ngaut

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

/run-all-tests

zhangjinpeng1987 added some commits Feb 18, 2019

check conflict first
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
format code
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
@AndreMouche
Copy link
Member

left a comment

The logic looks good to me, also I can see the performance improvement for the cases meet the following conditions:

  1. auto commit(1 sql in the transaction)
  2. only including insert operates in one region

However, I'm still curious about the following transaction:

begin;
insert xxx; // if there is a duplicated key, will it return a duplicated key error directly here?
insert
select
update
....
commit;

Will the above case using this PR?
If yes, what if only one range has a duplicated key, will it create many locks in other regions? How about the performance of this case?

engine.prewrite(m, k, 1);
engine.commit(k, 1, 2);

engine.rollback(k, 5);

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Feb 18, 2019

Contributor

Don't you need to prewrite something before calling rollback?

This comment has been minimized.

Copy link
@zhangjinpeng1987
@zhangjinpeng1987

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@AndreMouche It will behavior the same as before.

@zhangjinpeng1987

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@disksing PTAL

if should_not_exist {
if write.write_type == WriteType::Put
|| (write.write_type != WriteType::Delete
&& self.key_exist(&key, write.start_ts - 1)?)

This comment has been minimized.

Copy link
@disksing

disksing Feb 19, 2019

Collaborator

How about just if should_not_exist && self.key_exist(&key, ts-1)?

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Feb 19, 2019

Author Member

We have got a latest write here, it may don't need to call key_exist.

@zhangjinpeng1987 zhangjinpeng1987 merged commit 43a9789 into tikv:master Feb 19, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

dcalvin added a commit to dcalvin/tikv that referenced this pull request Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.