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

Push down condition check to TiKV for prewrite request #9127

Merged
merged 35 commits into from
Feb 19, 2019

Conversation

zhangjinpeng87
Copy link
Contributor

@zhangjinpeng87 zhangjinpeng87 commented Jan 21, 2019

What problem does this PR solve?

Push down condition check to TiKV for Insert request.

What is changed and how it works?

Push down condition check to TiKV, and the batch_get for insert statement with unique constraint can be removed.

Check List

Tests

  • Unit test

Code changes

  • Has interface methods change

Related PRs

These two PRs should be merged first.

Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
store/tikv/2pc.go Outdated Show resolved Hide resolved
store/mockstore/mocktikv/mvcc_leveldb.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

I suggest handle prewriteMutation logic like this:

func prewriteMutation() {
    if mutation.Op == Op_Insert {
        Get() // Special handle for Op_insert, call the Get() operation
    }
    ....
}

This may result in worse performance, but the code is much simpler.
@zhangjinpeng1987

Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
store/mockstore/mocktikv/mvcc_leveldb.go Outdated Show resolved Hide resolved
store/tikv/2pc.go Outdated Show resolved Hide resolved
if err == nil {
panic(fmt.Sprintf("con:%d, precondition error for key:%s should not be nil", c.connID, key))
}
log.Errorf("con:%d key: %s already exists", c.connID, key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why log error here? key already exists is a normal business logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to log duplicated entry error in TiDB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with tiancaiamao, if we log the duplicated error, there will be too many useless logs.

kv/union_store.go Outdated Show resolved Hide resolved
@zhangjinpeng87
Copy link
Contributor Author

@disksing PTAL

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reset LGTM

if err == nil {
panic(fmt.Sprintf("con:%d, precondition error for key:%s should not be nil", c.connID, key))
}
log.Errorf("con:%d key: %s already exists", c.connID, key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with tiancaiamao, if we log the duplicated error, there will be too many useless logs.

kv/union_store.go Show resolved Hide resolved
store/tikv/2pc.go Outdated Show resolved Hide resolved
Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
@disksing
Copy link
Contributor

LGTM

@zhangjinpeng87
Copy link
Contributor Author

@winkyao PTAL again.

@zhangjinpeng87
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor

/run-integration-tests

@tiancaiamao
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/coprocessor status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants