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

Add integration test for the issue that stale pessimistic lock request with force-locking enabled may cause data inconsistency after resolving lock #773

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MyonKeminta
Copy link
Contributor

Ref: tikv/tikv#14551

This PR adds a integration test case to cover the issue stated in tikv/tikv#14551

It's confirmed that the test will fail when running on older TiKV versions without the fix

    lock_test.go:1483:
                Error Trace:    /home/tidb/misono/sync/client-go/integration_tests/lock_test.go:1483
                                                        /home/tidb/misono/sync/client-go/integration_tests/lock_test.go:1510
                                                        /home/tidb/misono/sync/client-go/integration_tests/lock_test.go:1513
                Error:          Received unexpected error:
                                not exist
                                github.com/tikv/client-go/v2/error.init
                                        /home/tidb/misono/sync/client-go/error/error.go:53
                                runtime.doInit
                                        /usr/local/go/src/runtime/proc.go:6506
                                runtime.doInit
                                        /usr/local/go/src/runtime/proc.go:6483
                                runtime.doInit
                                        /usr/local/go/src/runtime/proc.go:6483
                                runtime.doInit
                                        /usr/local/go/src/runtime/proc.go:6483
                                runtime.doInit
                                        /usr/local/go/src/runtime/proc.go:6483
                                runtime.main
                                        /usr/local/go/src/runtime/proc.go:233
                                runtime.goexit
                                        /usr/local/go/src/runtime/asm_amd64.s:1598
                Test:           TestLockWithTiKV/TestStaleForcePessimisticLockNotCommitted
    lock_test.go:1484:
                Error Trace:    /home/tidb/misono/sync/client-go/integration_tests/lock_test.go:1484
                                                        /home/tidb/misono/sync/client-go/integration_tests/lock_test.go:1510
                                                        /home/tidb/misono/sync/client-go/integration_tests/lock_test.go:1513
                Error:          Not equal:
                                expected: []byte{0x76, 0x32}
                                actual  : []byte(nil)

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,4 +1,2 @@
                                -([]uint8) (len=2) {
                                - 00000000  76 32                                             |v2|
                                -}
                                +([]uint8) <nil>

                Test:           TestLockWithTiKV/TestStaleForcePessimisticLockNotCommitted

Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
@MyonKeminta MyonKeminta requested review from cfzjywxk and ekexium April 19, 2023 12:10
// Try to make transaction t1 and t2 commit at the same commitTS.
// First, block at finishing getting minCommitTS from PD and before prewriting
s.NoError(failpoint.Enable("tikvclient/beforePrewrite", "pause"))
resCh := make(chan error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to use resCh := make(chan error, 2) so they two results would not block each other on the result channel?


// Trigger resolving locks.
// For normal resolve lock procedures, it always uses special path for resolving pessimistic locks, which
// avoids this problem. However, `BatchResolveLock` which is used by GC doesn't have that handling (and
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed maybe we need to fix BatchResolveLock first and make the resolving behaviour and interface consistent

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'll try to find some other way to do this test later.

@MyonKeminta
Copy link
Contributor Author

This case actually won't happen, since when we meets pessimistic lock, we always call resolvePessimisticLock which only pessimistic-rollback the lock instead of performing normal resolve-lock procedure, and when we meets optimistic lock we always know the real primary.

But perhaps we also need to consider the case that a optimistic lock points to the real primary, but the primary is locked by a stale pessimistic lock pointing to another key which is the invalidated primary...

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.

3 participants