txn: Error handling for pessimistic locks

[pingyu]

Close: #313

What's changed

Automatic rollback successful locks when pessimistic_lock is failed.

How it is done

Add a new error PessimisticLockError holding successful locks return by TiKV server.
On failure, pessimistic_lock acquires the successful keys from PessimisticLockError and do the rollback.

Tests

• Manual
• Unit test
• Integrated test

[pingyu]

/cc @ekexium

[ekexium]

Great job! I just have a few suggestions.

And I noticed we didn't set the MULTI_REGION correctly in Github actions. Please do me a favor to fix it :). Simply change the last command in ci.yml to MULTI_REGION=1 make integration-test

[ekexium]

Had better print some logs here.

[ekexium]

I think we need to appropriately change the buffer state. In the current PR it doesn't affect at all, but I'm afriad the function might be misused. Or we can add a comment to explicitly warn such behavior.

[pingyu]

Had better print some logs here.

En... I print a debug msg "rollback pessimistic lock" at the beginning.
What more logs do we need ? (since there is also a simple debug log at the beginning of other methods)

[andylokandy]

@ekexium Do we allow the user to retry pessimistic lock? If we do, we should not eagerly rollback the successfully locked keys.

[ekexium]

@ekexium Do we allow the user to retry pessimistic lock? If we do, we should not eagerly rollback the successfully locked keys.

Users can retry, but we cannot make assumptions on that. The lock request may already have retried, subject to the backoff option.

[ekexium]

Had better print some logs here.

En... I print a debug msg "rollback pessimistic lock" at the beginning. What more logs do we need ? (since there is also a simple debug log at the beginning of other methods)

I didn't see it 🤦 . The logging is a mess for now. Let's just keep it as is in this PR.

[andylokandy]

Users can retry, but we cannot make assumptions on that. The lock request may already have retried, subject to the backoff option.

Let me explain my point again. If we allow the user to retry the pessimistic lock when the client returns errors, the client should remember which keys are locked, but not eagerly roll back them. Besides, the client should roll back the locked key when client.rollback() is called, or panic if get dropped without rolling back or committing.

[ekexium]

I think the major drawback of your idea is that if the user doesn't retry the same set of keys, then the locks will remain and may block other txns for some time.

If users want to retry the same set of keys, they can use a different backoff strategy (and we may consider enhance the retry of pessimistic locks to use a newer for_update_ts).

Reference: client-go immediately rollback the successful pessimistic locks, but I think part of the reason is to cooperate with TiDB, client-rust doesn't have to bahave in the same way.

[pingyu]

I agree with @ekexium . I also think that it would be better to provide an "atomic" method to developers, which will do nothing when return error.

[pingyu]

@ekexium
Comments addressed. I'm sorry for the late. PTAL, thanks~

[pingyu]

The integration test case txn_optimistic_heartbeat is stably failed. Let me see see.

[pingyu]

The integration test case txn_optimistic_heartbeat is stably failed. Let me see see.

The error is raised from clear_tikv in first tests::common::init(), and should be caused by the 500 ms timeout of DEFAULT_REGION_BACKOFF is not enough for CI environment.

I fixed it by prolonging the max delay to 10 seconds. Now all checks are passed.

[andylokandy]

@ekexium PTAL

[ekexium]

LGTM. We should consider support optional backoffs for all raw requests.