-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
*: calculate GC safe point based on global min start timestamp #12223
Changes from all commits
ce7ae55
d48f0fb
901c95f
7dee370
8d890a2
c1e4feb
dec2132
8b58e2f
b6d018f
d0bff40
c318f66
aaa6323
5ae29fd
259ca92
54acf4f
e523497
7ddd22d
a98140b
32be307
d3e6928
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,12 +29,13 @@ import ( | |
"github.com/pingcap/kvproto/pkg/metapb" | ||
"github.com/pingcap/parser/terror" | ||
pd "github.com/pingcap/pd/client" | ||
"github.com/pingcap/tidb/config" | ||
"github.com/pingcap/tidb/ddl/util" | ||
"github.com/pingcap/tidb/domain" | ||
"github.com/pingcap/tidb/kv" | ||
"github.com/pingcap/tidb/metrics" | ||
"github.com/pingcap/tidb/privilege" | ||
"github.com/pingcap/tidb/session" | ||
"github.com/pingcap/tidb/sessionctx/variable" | ||
"github.com/pingcap/tidb/store/tikv" | ||
"github.com/pingcap/tidb/store/tikv/oracle" | ||
"github.com/pingcap/tidb/store/tikv/tikvrpc" | ||
|
@@ -320,6 +321,30 @@ func (w *GCWorker) checkPrepare(ctx context.Context) (bool, uint64, error) { | |
return true, oracle.ComposeTS(oracle.GetPhysical(*newSafePoint), 0), nil | ||
} | ||
|
||
// calculateNewSafePoint uses the current global transaction min start timestamp to calculate the new safe point. | ||
func (w *GCWorker) calSafePointByMinStartTS(safePoint time.Time) time.Time { | ||
kvs, err := w.store.GetSafePointKV().GetWithPrefix(domain.ServerMinStartTSPath) | ||
if err != nil { | ||
logutil.BgLogger().Warn("get all minStartTS failed", zap.Error(err)) | ||
return safePoint | ||
} | ||
|
||
safePointTS := variable.GoTimeToTS(safePoint) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not need to |
||
for _, v := range kvs { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
minStartTS, err := strconv.ParseUint(string(v.Value), 10, 64) | ||
if err != nil { | ||
logutil.BgLogger().Warn("parse minStartTS failed", zap.Error(err)) | ||
continue | ||
} | ||
if minStartTS < safePointTS { | ||
safePointTS = minStartTS | ||
} | ||
} | ||
safePoint = time.Unix(0, oracle.ExtractPhysical(safePointTS)*1e6) | ||
logutil.BgLogger().Debug("calSafePointByMinStartTS", zap.Time("safePoint", safePoint)) | ||
return safePoint | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's is possible that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we needn't, since for these transactions which run longer than |
||
} | ||
|
||
func (w *GCWorker) getOracleTime() (time.Time, error) { | ||
currentVer, err := w.store.CurrentVersion() | ||
if err != nil { | ||
|
@@ -419,23 +444,16 @@ func (w *GCWorker) checkGCInterval(now time.Time) (bool, error) { | |
|
||
// validateGCLiftTime checks whether life time is small than min gc life time. | ||
func (w *GCWorker) validateGCLiftTime(lifeTime time.Duration) (time.Duration, error) { | ||
minLifeTime := gcMinLifeTime | ||
// max-txn-time-use value is less than gc_life_time - 10s. | ||
maxTxnTime := time.Duration(config.GetGlobalConfig().TiKVClient.MaxTxnTimeUse+10) * time.Second | ||
if minLifeTime < maxTxnTime { | ||
minLifeTime = maxTxnTime | ||
} | ||
|
||
if lifeTime >= minLifeTime { | ||
if lifeTime >= gcMinLifeTime { | ||
return lifeTime, nil | ||
} | ||
|
||
logutil.BgLogger().Info("[gc worker] invalid gc life time", | ||
zap.Duration("get gc life time", lifeTime), | ||
zap.Duration("min gc life time", minLifeTime)) | ||
zap.Duration("min gc life time", gcMinLifeTime)) | ||
|
||
err := w.saveDuration(gcLifeTimeKey, minLifeTime) | ||
return minLifeTime, err | ||
err := w.saveDuration(gcLifeTimeKey, gcMinLifeTime) | ||
return gcMinLifeTime, err | ||
} | ||
|
||
func (w *GCWorker) calculateNewSafePoint(now time.Time) (*time.Time, error) { | ||
|
@@ -452,7 +470,7 @@ func (w *GCWorker) calculateNewSafePoint(now time.Time) (*time.Time, error) { | |
if err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
safePoint := now.Add(-*lifeTime) | ||
safePoint := w.calSafePointByMinStartTS(now.Add(-*lifeTime)) | ||
// We should never decrease safePoint. | ||
if lastSafePoint != nil && safePoint.Before(*lastSafePoint) { | ||
logutil.BgLogger().Info("[gc worker] last safe point is later than current one."+ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any test that covers these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noop... Any suggestions? Maybe we need some intagration test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I'm not familiar with that part of code. Just thinking it will be nice if this is covered by tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test case added, PTAL @MyonKeminta .