*: calculate GC safe point based on global min start timestamp #12223

jackysp · 2019-09-17T09:14:17Z

What problem does this PR solve?

We do not need to increase GC life time or GC run interval to avoid the transaction aborted when it took too much time.

What is changed and how it works?

calculate the new safe point using the minimum of (now - GC life time) and the global min transaction start timestamp.
remove the configurable max-txn-time-use and use a fixed 24 hours instead, which is the longest transaction life time.
remove the relation of GC life time and the max-txn-time-use.

Check List

Tests

Manual test (add detailed scripts or steps below)

start 2 TiDB's cluster
connect to one TiDB, then begin; and get its start ts by information_schema.processlist
checkout /tidb/store/gcworker/saved_safe_point, it will pin to the min start ts
commit the transaction
checkout /tidb/store/gcworker/saved_safe_point again, and the safe point will update again.

Code changes

Has exported function/method change

Related changes

Need to update the documentation

jackysp · 2019-09-17T09:14:44Z

/run-all-tests

ngaut · 2019-09-17T09:25:45Z

Great job.

coocood · 2019-09-17T10:21:00Z

store/tikv/gcworker/gc_worker.go

+	globalMinStartTime := time.Unix(sec, nsec)
+
+	diff := safePoint.Sub(globalMinStartTime)
+	maxTxnTimeUse := time.Duration(tikv.MaxTxnTimeUse)*time.Millisecond + 10*time.Second


I think + 10secs is not needed.

coocood · 2019-09-17T10:24:32Z

store/tikv/gcworker/gc_worker.go

+	}
+	physical := oracle.ExtractPhysical(globalMinStartTS)
+	sec, nsec := physical/1e3, (physical%1e3)*1e6
+	globalMinStartTime := time.Unix(sec, nsec)


We can construct time with ns only,
time.Unix(0, physical * 1e6)

coocood · 2019-09-17T10:30:07Z

store/tikv/gcworker/gc_worker.go

+		return globalMinStartTime
+	}
+
+	return safePoint


It's is possible that the diff > maxTxnTimeUse but there is another transaction startTS before the safepoint.
So we need to check maxTxnTimeUse during kvs iteration.

I think we needn't, since for these transactions which run longer than maxTxnTimeUse should be failed finally

codecov · 2019-09-17T11:26:21Z

Codecov Report

Merging #12223 into master will increase coverage by 1.2613%.
The diff coverage is 63.2653%.

@@               Coverage Diff                @@
##             master     #12223        +/-   ##
================================================
+ Coverage   79.8138%   81.0751%   +1.2613%     
================================================
  Files           460        454         -6     
  Lines        102268      99150      -3118     
================================================
- Hits          81624      80386      -1238     
+ Misses        14746      12952      -1794     
+ Partials       5898       5812        -86

AndreMouche

I think if the minStartTs is not updated or not exist, we needn't start GC

AndreMouche · 2019-09-17T12:00:50Z

store/tikv/gcworker/gc_worker.go

+	globalMinStartTime := time.Unix(0, oracle.ExtractPhysical(math.MaxUint64)*1e6)
+	maxTxnTimeUse := time.Duration(tikv.MaxTxnTimeUse)*time.Millisecond + 10*time.Second
+
+	for _, v := range kvs {


Is it possible the kvs is empty?

AndreMouche · 2019-09-17T12:14:04Z

store/tikv/gcworker/gc_worker.go

+		return globalMinStartTime
+	}
+
+	return safePoint


I think we needn't, since for these transactions which run longer than maxTxnTimeUse should be failed finally

coocood · 2019-09-17T12:35:46Z

LGTM

AndreMouche

LGTM

AndreMouche · 2019-09-18T02:09:22Z

@MyonKeminta PTAL

crazycs520

LGTM

MyonKeminta · 2019-09-18T03:48:47Z

Will it work for clusters without TiDB?

MyonKeminta · 2019-09-18T07:05:35Z

store/tikv/gcworker/gc_worker.go

+		return safePoint
+	}
+
+	maxTxnTimeUse := time.Duration(tikv.MaxTxnTimeUse)*time.Millisecond + 10*time.Second


Why plus 10s?

I think this is your previous suggestion. Leave 10s for commit, otherwise, the transaction may not be committed yet, and GC has already started.

Sorry I didn't get it. I think it's not necessary.

jackysp · 2019-09-19T13:19:36Z

Will it work for clusters without TiDB?

No, I don't think the original way will not work either.

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

jackysp · 2019-09-24T07:39:22Z

Fixed a bug that could cause other long transactions of the same TiDB to fail. And added a test case. PTAL @coocood @AndreMouche @MyonKeminta

coocood · 2019-09-24T11:06:12Z

LGTM

MyonKeminta

Rest LGTM

MyonKeminta · 2019-09-26T02:38:03Z

store/tikv/gcworker/gc_worker.go

+		return safePoint
+	}
+
+	safePointTS := variable.GoTimeToTS(safePoint)


Why not use oracle.ComposeTS? The same to other usages of variable.GoTimeToTS

It does not need to GetPhysical().

MyonKeminta · 2019-09-26T04:00:19Z

store/tikv/gcworker/gc_worker_test.go

+	err = spkv.Put(fmt.Sprintf("%s/%s", domain.ServerMinStartTSPath, "b"), "1")
+	c.Assert(err, IsNil)
+	sp = s.gcWorker.calSafePointByMinStartTS(now)
+	c.Assert(sp, Equals, zeroTime)


I think you may at least need to add a case that saves a normal time to etcd, to check if calSafePointByMinStartTs parses the time correctly.

Let's add some integration tests.

MyonKeminta · 2019-09-26T04:01:18Z

domain/info.go

 	for _, info := range pl {
-		if info.CurTxnStartTS < minStartTS {
+		if info.CurTxnStartTS > startTSLowerLimit && info.CurTxnStartTS < minStartTS {


Is there any test that covers these changes?

noop... Any suggestions? Maybe we need some intagration test.

Sorry but I'm not familiar with that part of code. Just thinking it will be nice if this is covered by tests.

test case added, PTAL @MyonKeminta .

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

MyonKeminta

LGTM

sre-bot · 2019-09-30T05:25:50Z

Your auto merge job has been accepted, waiting for 12397

sre-bot · 2019-09-30T05:31:36Z

/run-all-tests

jackysp added the type/enhancement The issue or PR belongs to an enhancement. label Sep 17, 2019

jackysp requested review from lysu, coocood, MyonKeminta and crazycs520 September 17, 2019 09:14

jackysp added the sig/transaction SIG:Transaction label Sep 17, 2019

jackysp requested a review from disksing September 17, 2019 09:15

coocood reviewed Sep 17, 2019

View reviewed changes

AndreMouche reviewed Sep 17, 2019

View reviewed changes

AndreMouche previously approved these changes Sep 18, 2019

View reviewed changes

crazycs520 reviewed Sep 18, 2019

View reviewed changes

MyonKeminta reviewed Sep 18, 2019

View reviewed changes

jackysp added the status/DNM label Sep 21, 2019

jackysp dismissed AndreMouche’s stale review via c5c0414 September 21, 2019 16:07

jackysp added 9 commits September 24, 2019 13:22

*: calculate GC safe point based on global min start timestamp

ce7ae55

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

address comments

d48f0fb

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

update etcd key

901c95f

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

remove max-txn-time-use

7dee370

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

add some debug logs

8d890a2

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

fix bitSize

c1e4feb

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

skip startts == 0

dec2132

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

update the debug log

8b58e2f

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

fix import

b6d018f

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

jackysp added 4 commits September 24, 2019 13:24

address comments

d0bff40

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

fix a bug

c318f66

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

fix compile

aaa6323

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

avoid the second oldest transactions aborted in the same tidb-server

5ae29fd

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

jackysp force-pushed the update_gc_now branch from c5c0414 to 5ae29fd Compare September 24, 2019 05:24

add test cases

259ca92

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

jackysp requested review from coocood, MyonKeminta, crazycs520 and AndreMouche September 24, 2019 07:37

jackysp removed the status/DNM label Sep 24, 2019

MyonKeminta reviewed Sep 26, 2019

View reviewed changes

jackysp added 6 commits September 29, 2019 13:16

add test cases

54acf4f

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

address comments

e523497

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

fix data race in test cases

7ddd22d

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

fix race

a98140b

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

revert the wrong fix

32be307

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

Merge branch 'master' into update_gc_now

d3e6928

MyonKeminta reviewed Sep 30, 2019

View reviewed changes

coocood approved these changes Sep 30, 2019

View reviewed changes

coocood added the status/can-merge Indicates a PR has been approved by a committer. label Sep 30, 2019

sre-bot merged commit f146185 into pingcap:master Sep 30, 2019

jackysp deleted the update_gc_now branch February 27, 2020 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: calculate GC safe point based on global min start timestamp #12223

*: calculate GC safe point based on global min start timestamp #12223

jackysp commented Sep 17, 2019 •

edited

Loading

jackysp commented Sep 17, 2019

ngaut commented Sep 17, 2019

coocood Sep 17, 2019

coocood Sep 17, 2019

coocood Sep 17, 2019

AndreMouche Sep 17, 2019

codecov bot commented Sep 17, 2019 •

edited

Loading

AndreMouche left a comment

AndreMouche Sep 17, 2019

jackysp Sep 17, 2019

AndreMouche Sep 17, 2019

coocood commented Sep 17, 2019

AndreMouche left a comment

AndreMouche commented Sep 18, 2019

crazycs520 left a comment

MyonKeminta commented Sep 18, 2019

MyonKeminta Sep 18, 2019

jackysp Sep 19, 2019

MyonKeminta Sep 23, 2019

jackysp commented Sep 19, 2019

jackysp commented Sep 24, 2019

coocood commented Sep 24, 2019

MyonKeminta left a comment

MyonKeminta Sep 26, 2019

jackysp Sep 26, 2019

MyonKeminta Sep 26, 2019

jackysp Sep 26, 2019

MyonKeminta Sep 26, 2019

jackysp Sep 26, 2019

MyonKeminta Sep 26, 2019

jackysp Sep 29, 2019

MyonKeminta left a comment

sre-bot commented Sep 30, 2019

sre-bot commented Sep 30, 2019

*: calculate GC safe point based on global min start timestamp #12223

*: calculate GC safe point based on global min start timestamp #12223

Conversation

jackysp commented Sep 17, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

jackysp commented Sep 17, 2019

ngaut commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2019 • edited Loading

Codecov Report

AndreMouche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Sep 17, 2019

AndreMouche left a comment

Choose a reason for hiding this comment

AndreMouche commented Sep 18, 2019

crazycs520 left a comment

Choose a reason for hiding this comment

MyonKeminta commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp commented Sep 19, 2019

jackysp commented Sep 24, 2019

coocood commented Sep 24, 2019

MyonKeminta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta left a comment

Choose a reason for hiding this comment

sre-bot commented Sep 30, 2019

sre-bot commented Sep 30, 2019

jackysp commented Sep 17, 2019 •

edited

Loading

codecov bot commented Sep 17, 2019 •

edited

Loading