Fix the issue that primary pessimistic lock may be left not cleared after GC #866

MyonKeminta · 2023-07-03T12:57:04Z

The problem is that BatchResolveLock calls resolvePessimsticLock without calling getTxnStatus, but resolvePessimsticLock assumes getTxnStatus (in which CheckTxnStatus RPC is called) is done so it doesn't do anything to primary pessimistic lock. This may result in residue pessimistic locks in some rare cases.

This PR moves resolvePessimsticLock calling to after getTxnStatus to avoid the problem.

…fter GC Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk · 2023-07-04T03:49:36Z

txnkv/transaction/txn.go

@@ -556,10 +556,26 @@ func (txn *KVTxn) Rollback() error {
 		txn.CancelAggressiveLocking(context.Background())
 	}

+	skipPessimisticRollback := false


Better to add comments if it's used just in tests.

cfzjywxk · 2023-07-04T03:54:37Z

txnkv/txnlock/lock_resolver.go

+		if err != nil {
+			return false, err
+		}
+
 		if l.LockType == kvrpcpb.Op_PessimisticLock {


Should we add some comments on resolvePessimisticLock that it should be called only after the getTxnStatus is called?

…ore strict checks Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…ared after GC (#866) (#867) * Fix the issue that primary pessimistic lock may be left not cleared after GC Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Address comments Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> --------- Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…fter GC (tikv#866) * Fix the issue that primary pessimistic lock may be left not cleared after GC Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Fix mysteriously shown up thing that makes compilation failed Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Fix test effectiveness (forgot to set txn2 to pessimistic txn); add more strict checks Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Address comments Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> --------- Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…fter GC (#866) (#869) * Fix the issue that primary pessimistic lock may be left not cleared after GC * Fix mysteriously shown up thing that makes compilation failed * Fix test effectiveness (forgot to set txn2 to pessimistic txn); add more strict checks * Address comments --------- Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…fter GC (#866) (#870) * Fix the issue that primary pessimistic lock may be left not cleared after GC * Fix mysteriously shown up thing that makes compilation failed * Fix test effectiveness (forgot to set txn2 to pessimistic txn); add more strict checks * Address comments --------- Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

* client-go: add some key range info to error when PD returned no region (#862) Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> * *: refine non-global stale-read request retry logic (#863) Signed-off-by: crazycs520 <crazycs520@gmail.com> * Fix the issue that primary pessimistic lock may be left not cleared after GC (#866) * Fix the issue that primary pessimistic lock may be left not cleared after GC Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Fix mysteriously shown up thing that makes compilation failed Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Fix test effectiveness (forgot to set txn2 to pessimistic txn); add more strict checks Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Address comments Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> --------- Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * add explicit request source type to label the external request like lightning/br (#868) Signed-off-by: nolouch <nolouch@gmail.com> * use '%d' instead of '%q' for some int values in error message (#875) Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> * format key in error message in method `scanRegions` (#876) Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> * make cop request timeout a config paramter (#865) * update Signed-off-by: Spade A <u6748471@anu.edu.au> * update Signed-off-by: Spade A <u6748471@anu.edu.au> * update Signed-off-by: Spade A <u6748471@anu.edu.au> * update Signed-off-by: Spade A <u6748471@anu.edu.au> --------- Signed-off-by: Spade A <u6748471@anu.edu.au> * region_cache: support check pending tiflash peer (#821) Signed-off-by: guo-shaoge <shaoge1994@163.com> Co-authored-by: disksing <i@disksing.com> * *: add `SnapshotIterReverse` and make `iterReverse` supports `lowerBound` (#883) Signed-off-by: Jason Mo <mohangjie1995@gmail.com> * *: fix stale read ops metric (#878) (#889) Signed-off-by: crazycs520 <crazycs520@gmail.com> Co-authored-by: disksing <i@disksing.com> * add gc options (#828) Signed-off-by: weedge <weege007@gmail.com> Co-authored-by: disksing <i@disksing.com> * reload region cache when store is resolved from invalid status (#843) Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: disksing <i@disksing.com> * ci: update setup-go action (#904) Signed-off-by: disksing <i@disksing.com> * fix unexpected slow query during GC running after stop 1 tikv-server (#899) (#909) * fix unexpected slow query during GC running after stop 1 tikv-server Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix test Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * resource_manager: ignore ru metrics for background request (#872) Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: disksing <i@disksing.com> * add more log for diagnose (#915) * add more log for diagnose Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix Signed-off-by: crazycs520 <crazycs520@gmail.com> * add more log for diagnose Signed-off-by: crazycs520 <crazycs520@gmail.com> * add more log Signed-off-by: crazycs520 <crazycs520@gmail.com> * address comment Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * use context logger as much as possible (#908) * use context logger as much as possible Signed-off-by: crazycs520 <crazycs520@gmail.com> * refine Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * Resume max retry time check for stale read retry with leader option(#903) (#911) * Resume max retry time check for stale read retry with leader option Signed-off-by: cfzjywxk <lsswxrxr@163.com> * add cancel Signed-off-by: cfzjywxk <lsswxrxr@163.com> --------- Signed-off-by: cfzjywxk <lsswxrxr@163.com> * request_source: remove default label (#890) * request_source: remove default label Signed-off-by: nolouch <nolouch@gmail.com> * add a function to set request source task type (#925) * add a function to set request source task type Signed-off-by: glorv <glorvs@163.com> * ci: update go version (#936) * ci: update go version Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix test Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * use tidb_kv_read_timeout as first kv request timeout (#919) * support tidb_kv_read_timeout as first round kv request timeout Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * update comment Signed-off-by: crazycs520 <crazycs520@gmail.com> * refine test Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * [pick] resource_control: bypass some internal urgent request (#938) * resource_control: bypass some internal urgent request (#884) Signed-off-by: nolouch <nolouch@gmail.com> * resourcecontrol: fix nil pointer (#900) Signed-off-by: nolouch <nolouch@gmail.com> --------- Signed-off-by: nolouch <nolouch@gmail.com> --------- Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> Signed-off-by: crazycs520 <crazycs520@gmail.com> Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Signed-off-by: nolouch <nolouch@gmail.com> Signed-off-by: Spade A <u6748471@anu.edu.au> Signed-off-by: guo-shaoge <shaoge1994@163.com> Signed-off-by: Jason Mo <mohangjie1995@gmail.com> Signed-off-by: weedge <weege007@gmail.com> Signed-off-by: you06 <you1474600@gmail.com> Signed-off-by: disksing <i@disksing.com> Signed-off-by: husharp <jinhao.hu@pingcap.com> Signed-off-by: cfzjywxk <lsswxrxr@163.com> Signed-off-by: glorv <glorvs@163.com> Signed-off-by: iosmanthus <myosmanthustree@gmail.com> Co-authored-by: 王超 <cclcwangchao@hotmail.com> Co-authored-by: crazycs <crazycs520@gmail.com> Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: ShuNing <nolouch@gmail.com> Co-authored-by: Spade A <71589810+SpadeA-Tang@users.noreply.github.com> Co-authored-by: guo-shaoge <shaoge1994@163.com> Co-authored-by: disksing <i@disksing.com> Co-authored-by: Hangjie Mo <mohangjie1995@gmail.com> Co-authored-by: weedge <weege007@gmail.com> Co-authored-by: you06 <you1474600@gmail.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: cfzjywxk <lsswxrxr@163.com> Co-authored-by: glorv <glorvs@163.com>

Signed-off-by: Ping Yu <yuping@pingcap.com>

MyonKeminta added 2 commits July 3, 2023 20:54

Fix the issue that primary pessimistic lock may be left not cleared a…

032c8f5

…fter GC Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix mysteriously shown up thing that makes compilation failed

2acf9f3

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta requested review from cfzjywxk, zyguan and ekexium and removed request for cfzjywxk July 4, 2023 03:28

cfzjywxk reviewed Jul 4, 2023

View reviewed changes

MyonKeminta added 2 commits July 4, 2023 12:08

Fix test effectiveness (forgot to set txn2 to pessimistic txn); add m…

0a10498

…ore strict checks Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Address comments

3df6d24

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ekexium approved these changes Jul 4, 2023

View reviewed changes

cfzjywxk merged commit c0cf773 into tikv:master Jul 4, 2023

MyonKeminta mentioned this pull request Jul 4, 2023

(6.1) Fix the issue that primary pessimistic lock may be left not cleared after GC (#866) #867

Merged

MyonKeminta mentioned this pull request Jul 4, 2023

txn: Update client-go to fix the issue that GC BatchResolveLcok may miss primary pessimistic locks pingcap/tidb#45143

Merged

12 tasks

MyonKeminta mentioned this pull request Jul 4, 2023

(6.5) Fix the issue that primary pessimistic lock may be left not cleared after GC (#866) #869

Merged

MyonKeminta mentioned this pull request Jul 4, 2023

(7.1) Fix the issue that primary pessimistic lock may be left not cleared after GC (#866) #870

Merged

MyonKeminta deleted the m/fix-batch-resolve-lock-miss-primary-pessimistic-lock branch July 4, 2023 09:15

pingyu added a commit to pingyu/client-go that referenced this pull request Dec 13, 2023

try revert tikv#866

ffd48a1

Signed-off-by: Ping Yu <yuping@pingcap.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the issue that primary pessimistic lock may be left not cleared after GC #866

Fix the issue that primary pessimistic lock may be left not cleared after GC #866

MyonKeminta commented Jul 3, 2023 •

edited

Loading

cfzjywxk Jul 4, 2023

cfzjywxk Jul 4, 2023

Fix the issue that primary pessimistic lock may be left not cleared after GC #866

Fix the issue that primary pessimistic lock may be left not cleared after GC #866

Conversation

MyonKeminta commented Jul 3, 2023 • edited Loading

cfzjywxk Jul 4, 2023

Choose a reason for hiding this comment

cfzjywxk Jul 4, 2023

Choose a reason for hiding this comment

MyonKeminta commented Jul 3, 2023 •

edited

Loading