store: fix potential panic in GC worker #14403

jackysp · 2020-01-09T03:33:05Z

Signed-off-by: Shuaipeng Yu [email protected]

What problem does this PR solve?

Try to fix

goroutine 5109074295 [running]:
github.com/pingcap/tidb/executor.(*ExecStmt).Exec.func1(0xc01b732830, 0x23b8c60, 0xc000138000, 0xc7ef2ca7e0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/adapter.go:229 +0x48c
panic(0x1ebe500, 0x31c4a60)
	/usr/local/go/src/runtime/panic.go:679 +0x1b2
github.com/pingcap/tidb/session.(*TxnState).LockKeys(0xc2e81aa1f0, 0x23b8c60, 0xc000138000, 0xc03018ab30, 0x5bd729875b001b6, 0xc350, 0xc817a17540, 0x1, 0x1, 0xc0638be8c0, ...)
	<autogenerated>:1 +0x36
github.com/pingcap/tidb/executor.doLockKeys(0x23b8c60, 0xc000138000, 0x23effa0, 0xc2e81aa1e0, 0xc350, 0xc817a17540, 0x1, 0x1, 0x13, 0xc2bdd5e300)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/executor.go:800 +0x185
github.com/pingcap/tidb/executor.(*SelectLockExec).Next(0xc33d69de60, 0x23b8c60, 0xc000138000, 0xca6dc03680, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/executor.go:789 +0x5a8
github.com/pingcap/tidb/executor.Next(0x23b8c60, 0xc000138000, 0x23bf820, 0xc33d69de60, 0xca6dc03680, 0x0, 0x203000)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/executor.go:192 +0xbd
github.com/pingcap/tidb/executor.(*ProjectionExec).unParallelExecute(0xc0148a7100, 0x23b8c60, 0xc000138000, 0xca27605ce0, 0xc3a2a6, 0xc013663180)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/projection.go:182 +0x8d
github.com/pingcap/tidb/executor.(*ProjectionExec).Next(0xc0148a7100, 0x23b8c60, 0xc000138000, 0xca27605ce0, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/projection.go:169 +0xf4
github.com/pingcap/tidb/executor.Next(0x23b8c60, 0xc000138000, 0x23bf660, 0xc0148a7100, 0xca27605ce0, 0xca2698aa98, 0xc00b0e9080)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/executor.go:192 +0xbd
github.com/pingcap/tidb/executor.(*recordSet).Next(0xc01b732578, 0x23b8c60, 0xc000138000, 0xca27605ce0, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/adapter.go:113 +0xc7
github.com/pingcap/tidb/executor.(*ExecStmt).runPessimisticSelectForUpdate(0xc7ef2ca7e0, 0x23b8c60, 0xc000138000, 0x23bf660, 0xc0148a7100, 0x0, 0x0, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/adapter.go:387 +0x303
github.com/pingcap/tidb/executor.(*ExecStmt).handlePessimisticSelectForUpdate(0xc7ef2ca7e0, 0x23b8c60, 0xc000138000, 0x23bf660, 0xc0148a7100, 0x355a9c0, 0xc1a4df2b00, 0x0, 0xc03018a800)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/adapter.go:362 +0x5d
github.com/pingcap/tidb/executor.(*ExecStmt).Exec(0xc7ef2ca7e0, 0x23b8c60, 0xc000138000, 0x0, 0x0, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/executor/adapter.go:287 +0x70c
github.com/pingcap/tidb/session.runStmt(0x23b8c60, 0xc000138000, 0x23effa0, 0xc2e81aa1e0, 0x23bf1e0, 0xc7ef2ca7e0, 0x0, 0x0, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/session/tidb.go:249 +0x1cb
github.com/pingcap/tidb/session.(*session).executeStatement(0xc2e81aa1e0, 0x23b8c60, 0xc000138000, 0x0, 0x23beba0, 0xc0183f58c0, 0x23bf1e0, 0xc7ef2ca7e0, 0x0, 0x0, ...)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/session/session.go:976 +0x1ce
github.com/pingcap/tidb/session.(*session).execute(0xc2e81aa1e0, 0x23b8c60, 0xc000138000, 0xc1a4df2b60, 0x63, 0xc9c65c, 0x31c8260, 0xc0176dc880, 0x11, 0xc01b733190)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/session/session.go:1091 +0x96a
github.com/pingcap/tidb/session.(*session).Execute(0xc2e81aa1e0, 0x23b8c60, 0xc000138000, 0xc1a4df2b60, 0x63, 0x0, 0x0, 0x0, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/session/session.go:1014 +0xdb
github.com/pingcap/tidb/store/tikv/gcworker.(*GCWorker).loadValueFromSysTable(0xc002626200, 0x2104528, 0xc, 0x0, 0x0, 0x0, 0x0)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/store/tikv/gcworker/gc_worker.go:1274 +0x119
github.com/pingcap/tidb/store/tikv/gcworker.(*GCWorker).checkUseDistributedGC(0xc002626200, 0x23b8c20, 0xc003ae2400, 0x5bd722345b00000)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/store/tikv/gcworker/gc_worker.go:749 +0x55
github.com/pingcap/tidb/store/tikv/gcworker.(*GCWorker).runGCJob(0xc002626200, 0x23b8c20, 0xc003ae2400, 0x5bd722345b00000, 0xc)
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/store/tikv/gcworker/gc_worker.go:534 +0x1451
created by github.com/pingcap/tidb/store/tikv/gcworker.(*GCWorker).leaderTick
	/home/jenkins/agent/workspace/tidb_release-3.0/go/src/github.com/pingcap/tidb/store/tikv/gcworker/gc_worker.go:281 +0xd96

What is changed and how it works?

Do not create a private session for checkLeader.

Check List

Tests

Unit test

Code changes

Has exported function/method change

Related changes

Need to cherry-pick to the release branch

jackysp · 2020-01-09T03:38:58Z

/run-all-tests

lysu · 2020-01-09T05:46:12Z

store/tikv/gcworker/gc_worker.go

@@ -1067,42 +1067,39 @@ func (w *GCWorker) doGC(ctx context.Context, safePoint uint64, concurrency int)

 func (w *GCWorker) checkLeader() (bool, error) {
 	metrics.GCWorkerCounter.WithLabelValues("check_leader").Inc()
-	se := createSession(w.store)
-	defer se.Close()


@hicqu #4453 PTAL

jackysp · 2020-01-09T12:12:33Z

/run-all-tests

Signed-off-by: Shuaipeng Yu <[email protected]>

jackysp · 2020-01-10T02:12:05Z

/run-all-tests

MyonKeminta

Rest LGTM

MyonKeminta · 2020-01-10T06:09:56Z

store/tikv/gcworker/gc_worker.go

+	se := createSession(w.store)
+	defer se.Close()


It seems the session is not used in this function

Signed-off-by: Shuaipeng Yu <[email protected]>

coocood · 2020-01-10T06:49:55Z

LGTM

coocood · 2020-01-10T06:50:59Z

/merge

sre-bot · 2020-01-10T06:53:16Z

/run-all-tests

sre-bot · 2020-01-10T06:56:13Z

@jackysp merge failed.

jackysp · 2020-01-10T07:18:38Z

/merge

sre-bot · 2020-01-10T07:21:15Z

/run-all-tests

sre-bot · 2020-01-10T07:25:26Z

cherry pick to release-3.0 failed

Conflicts: store/tikv/gcworker/gc_worker.go store/tikv/gcworker/gc_worker_test.go

bb7133 · 2020-01-16T16:10:05Z

Hi @jackysp , please update the release note in the PR description, thanks!

jackysp added type/bugfix This PR fixes a bug. component/GC needs-cherry-pick-3.0 labels Jan 9, 2020

jackysp requested a review from lysu January 9, 2020 03:35

jackysp marked this pull request as ready for review January 9, 2020 03:50

jackysp requested review from coocood and MyonKeminta January 9, 2020 03:50

lysu reviewed Jan 9, 2020

View reviewed changes

lysu requested a review from hicqu January 9, 2020 05:46

jackysp added 2 commits January 10, 2020 10:11

store: fix potential panic in GC worker

dfe5d0b

Signed-off-by: Shuaipeng Yu <[email protected]>

use function local session instead of object one

29b1e92

Signed-off-by: Shuaipeng Yu <[email protected]>

jackysp force-pushed the fix_gc_worker branch from ea1735c to 29b1e92 Compare January 10, 2020 02:11

jackysp added the status/all tests passed label Jan 10, 2020

MyonKeminta reviewed Jan 10, 2020

View reviewed changes

address comments

6f976d6

Signed-off-by: Shuaipeng Yu <[email protected]>

coocood approved these changes Jan 10, 2020

View reviewed changes

coocood added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 10, 2020

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 10, 2020

Merge branch 'master' into fix_gc_worker

4e172ad

Merge branch 'master' into fix_gc_worker

de12210

sre-bot merged commit 27c7264 into pingcap:master Jan 10, 2020

jackysp added a commit to jackysp/tidb that referenced this pull request Jan 10, 2020

store: fix potential panic in GC worker (pingcap#14403)

350194d

Conflicts: store/tikv/gcworker/gc_worker.go store/tikv/gcworker/gc_worker_test.go

jackysp mentioned this pull request Jan 10, 2020

store: fix potential panic in GC worker (#14403) #14439

Merged

jackysp added a commit that referenced this pull request Jan 16, 2020

store: fix potential panic in GC worker (#14403) (#14439)

50963e7

imtbkcat pushed a commit to imtbkcat/tidb that referenced this pull request Jan 16, 2020

store: fix potential panic in GC worker (pingcap#14403)

b29826a

imtbkcat mentioned this pull request Jan 16, 2020

store: fix potential panic in GC worker (#14403) #14501

Merged

bb7133 added the needs-cherry-pick-2.1 label Jan 16, 2020

imtbkcat pushed a commit to imtbkcat/tidb that referenced this pull request Jan 17, 2020

store: fix potential panic in GC worker (pingcap#14403)

66c23c9

jackysp pushed a commit that referenced this pull request Jan 19, 2020

store: fix potential panic in GC worker (#14403) (#14501)

7a60c80

jackysp deleted the fix_gc_worker branch February 27, 2020 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store: fix potential panic in GC worker #14403

store: fix potential panic in GC worker #14403

jackysp commented Jan 9, 2020

jackysp commented Jan 9, 2020

lysu Jan 9, 2020

jackysp commented Jan 9, 2020

jackysp commented Jan 10, 2020

MyonKeminta left a comment

MyonKeminta Jan 10, 2020

jackysp Jan 10, 2020

coocood commented Jan 10, 2020

coocood commented Jan 10, 2020

sre-bot commented Jan 10, 2020

sre-bot commented Jan 10, 2020

jackysp commented Jan 10, 2020

sre-bot commented Jan 10, 2020

sre-bot commented Jan 10, 2020

bb7133 commented Jan 16, 2020

store: fix potential panic in GC worker #14403

store: fix potential panic in GC worker #14403

Conversation

jackysp commented Jan 9, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

jackysp commented Jan 9, 2020

lysu Jan 9, 2020

Choose a reason for hiding this comment

jackysp commented Jan 9, 2020

jackysp commented Jan 10, 2020

MyonKeminta left a comment

Choose a reason for hiding this comment

MyonKeminta Jan 10, 2020

Choose a reason for hiding this comment

jackysp Jan 10, 2020

Choose a reason for hiding this comment

coocood commented Jan 10, 2020

coocood commented Jan 10, 2020

sre-bot commented Jan 10, 2020

sre-bot commented Jan 10, 2020

jackysp commented Jan 10, 2020

sre-bot commented Jan 10, 2020

sre-bot commented Jan 10, 2020

bb7133 commented Jan 16, 2020