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

RFC: coprocessor write #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sticnarf
Copy link
Contributor

@sticnarf sticnarf commented Oct 13, 2020

Update or pessimistic lock push-downs have been mentioned a lot of times but there are only rough ideas.

This RPC intends to make clear the protocol and how this feature will be implemented in TiKV.

Rendered

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
Copy link
Contributor Author

cc @breeswish @zhangjinpeng1987 @coocood @cfzjywxk @youjiali1995


This optimization needs some more work. We can create a `PointPrewrite` executor which is a specialized `TableScan` as the bottom executor of DAG. It yields results just like `TableScan` but also does constraint checks of prewrite. After the execution, we are safe to send the mutations to the underlying raft store without checking write conflicts again.

### DAG_PESSIMISTIC_LOCK
Copy link

Choose a reason for hiding this comment

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

I think this feature doesn't have much benefit as DAG_PREWRITE.
It's only useful for UPDATE statements with primary key range condition.

Copy link
Contributor Author

@sticnarf sticnarf Oct 13, 2020

Choose a reason for hiding this comment

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

Could you clarify it a bit more? I don't see why it's limited to primary key range conditions.
If it only has a selection exectuor, I think the filtered rows are exactly the rows that need to be locked.

Copy link

Choose a reason for hiding this comment

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

Sorry, you're right.

Copy link

Choose a reason for hiding this comment

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

But still, it's much more complicated than DAG_PREWRITE which only needs to handle the auto-commit single key non-index update.

It is unlikely to be implement in 5.0 release.

Copy link
Contributor Author

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 more complicated than DAG_PREWRITE.

This document is only an RFC for technical discussion. I'm not going to decide which version will contain these features here.

I think it should be the product managers who decide it after the RFC is mostly accepted.

Copy link

Choose a reason for hiding this comment

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

How about we separate the two features into different RFCs?


However, in multiple cases, we can move some of the logic from the client to TiKV to reduce one RPC and save encoding/decoding cost.

### Simple updates

Choose a reason for hiding this comment

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

I think we could make it more detailed about what scenarios we are about to solve with this RFC, how should we extend it if we want to solve more.


We can find the result set contains sufficient information to construct the row keys we want to lock. It would be better if we can use only one RPC to do the query and lock these keys. Coprocessor can take the result set of the query as the input of `AcquirePessimisticLock`. This can save one RPC.

## Detailed design

Choose a reason for hiding this comment

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

We could add some execution flow to explain how the optimized scenarios will work. From the SQL execution perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants