RFC: In-memory Pessimistic Locks

[sticnarf]

This is a more aggresive optimization than pipelined pessimistic lock. It tries to put pessimistic locks only in memory and not replicate them through Raft while not decreasing the success rate of pessimistic transactions.

According to preliminary tests, this optimization reduces disk write flow and raftstore CPU by about 20%.

cc @HunDunDM @youjiali1995 @gengliqi @cfzjywxk @MyonKeminta @longfangsong

[BusyJay]

Can you write a section to reasoning correctness explicitly?

[youjiali1995]

There is a notable difference between pipelined pessimistic lock and in memory pessimistic lock -- if a pessimistic lock is written, it's never lost in pipelined pessimistic lock but in memory pessimistic lock may be lost. Can you elaborate on that?

[sticnarf]

The new commit adds some more detailed handlings about leader transfer and some explanations about correctness.

[zhangjinpeng87]

How about tikv store level failure?

[zhangjinpeng87]

How about the leader encounters network isolation?

[sticnarf]

The behavior under these store-level failures is not different from using pipelined pessimistic lock. The pessimisitc transactions are not guaranteed to commit successfully, but they can commit if no write conflicts have happen.

[zhangjinpeng87]

pipelined pessimistic lock is off by default? so this feature will not open by default?

[sticnarf]

Pipielined pessimistic lock is enabled by default from 5.0. So, this feature will be enabled by default as well.

[Little-Wallace]

I can not understand. Because raft will only append and apply log in order as they are send, we just need to propose the entry MsgTransferLeader after lock. Just like that.

fn propose_transfer_leader(&mut self, msg: RaftMessage) {
    if !self.locks.is_empty() {
           let lock_msg = RaftMessage::new();
           for lock in self.locks.take() {
                 lock_msg.append(lock);
                 if lock_msg.size() > 1024 * 1024 {   // 1MB
                     self.propose(lock_msg);
                 }
           }
           self.propose(lock_msg);
     }
     self.propose(msg);
}

[sticnarf]

Well, my initial thought was to not change the transfer leader procedure. Using a proposal is simpler.

[cfzjywxk]

Another difference is the impact of leader transafer on ongoing pessimistic transactions such as the fallback processing described in this document. If the impact is ok we could use the simpler way and make it a whole, it's easier to maintain.

[youjiali1995]

The timeout mechanism is still needed. The leader transfer timeout is election timeout in raft. We can check it on tick.

[youjiali1995]

The timeout mechanism is still needed. The leader transfer timeout is election timeout in raft. We can check it on tick.

[youjiali1995]

LGTM

[cfzjywxk]

LGTM

[moonsphere]

This operation must be replicated through Raft.

maybe this can get some inspiration from CRDB... (every read cmd need to go through raft is a little expensive.

it seems they also can have similar push-txn mechanics, but they only update timestamp in memory tsCache instead of updating a field in the lock(write-intent), the new leader also couldn't get old pushed timestamp info because of lost memory.

in the previous version, it seems they choose to update tcCache low water ts to new lease time to let conflict write txn report error & retry...https://github.com/cockroachdb/cockroach/blob/df826cdf700a79948d083827ca67967016a1a1af/pkg/kv/kvserver/replica_proposal.go#L383... (it's unfriendly to user but safe..

but it seems in this year, they take some new improvements cockroachdb/cockroach@a7472e3, to transfer ts cache during leader transfer and range merge, maybe have litter chance to force the user to retry in commit phase.. (ps: I'm not carefully read this new code, maybe wrong 😄

[sticnarf]

This does not mean every read cmd will go through Raft. It is rare in practice to update the min_commit_ts in the pessimistic lock. Keys are prewritten in parallel, modifying the pessimistic lock only happens when secondary keys are prewritten and the primary key is not prewritten.

I haven't understood the new CRDB approach yet. We will get the pessimistic lock lost under unexpected crashes. But then, the ts cache cannot be transferred as well... Currently, I don't get how it will decrease the failure rate...

[moonsphere]

This does not mean every read cmd

thanks for explaining 😄 ~

I don't get how it will decrease the failure rate

I mean it will help decrease failure rate for "transfer leader", it seems previous version transfer leader also will set low water as newLease.Start just like crash - -

but for real "unexpected crashes", the pr should have no improvement for fail rate (

[Little-Wallace]

When TiKV add a learner for this region, it will send a snapshot to learner node. But how we could keep the consistency for both lockcf and writecf?

[Little-Wallace]

I means that this memory structure need give a snapshot-like result, so that not transaction will effect this result, which is scanning data to send to learner node.

[sticnarf]

Pessimistic locks are totally ignored for all reading requests. So, we can care only about the leader.