gc: Add `GCStateManager` which is planned to be the replacement of `SafePointManager` #9169

MyonKeminta · 2025-03-24T10:17:01Z

What problem does this PR solve?

Issue Number: Ref #8978

What is changed and how does it work?

Add `GCStateManager` which is planned to be the replacement of `SafePointManager`.

This is part of the refactor task #8978 . The new module is not yet used until the next PR updates the gRPC service interfaces.

This PR is split out from #9134 to reduce the single PR size a little.

Check List

Tests

Unit test

Code changes

Side effects

Possible performance regression
Increased code complexity

Related changes

Release note

None.

Signed-off-by: MyonKeminta <[email protected]>

…torage

Signed-off-by: MyonKeminta <[email protected]>

…torage

Signed-off-by: MyonKeminta <[email protected]>

JmPotato · 2025-03-28T07:26:53Z

pkg/gc/gc_state_manager.go

+	var oldTxnSafePoint uint64
+	newTxnSafePoint := target
+	minBlocker := target
+	var blockingBarrier *endpoint.GCBarrier
+	var blockingMinStartTSOwner *string


Suggested change

var oldTxnSafePoint uint64

newTxnSafePoint := target

minBlocker := target

var blockingBarrier *endpoint.GCBarrier

var blockingMinStartTSOwner *string

var (

minBlocker = target

newTxnSafePoint = target

oldTxnSafePoint uint64

blockingBarrier *endpoint.GCBarrier

blockingMinStartTSOwner *string

)

JmPotato · 2025-03-28T07:32:04Z

pkg/gc/gc_state_manager.go

+				continue
+			}
+
+			if barrier.IsExpired(now) {


I noticed that this comparison occurs during/after the etcd transcation. If certain disk failures cause the read/write operation to take an extremely long time, would this affect the judgment of the barrier time?

I didn't get it. Can you explain the bad case you are concerning about in detail?
In my consideration, it's safe that the actual expiration time is a little longer than the specified expiration time, and guarantee a GC barrier to be expired once it's regarded expired (delete immediately in the currrent transaction if IsExpired returns true). It should not affect the correctness or safety when a transaction runs too long.

okJiang · 2025-03-28T09:02:33Z

pkg/gc/gc_state_manager.go

+
+// saturatingDuration returns a duration calculated by multiplying the given `ratio` and `base`, truncated within the
+// range [0, math.MaxInt64] to avoid negative value and overflowing.
+func saturatingDuration(ratio int64, base time.Duration) time.Duration {


You don't use this function in this pr. How about removing it now until you use it?

Oh..
I will move it to the next PR where it's used

okJiang · 2025-03-28T09:06:41Z

pkg/gc/gc_state_manager.go

+	// Regard it as NullKeyspaceID if the given one is invalid (exceeds the valid range of keyspace id), no matter
+	// whether it exactly matches the NullKeyspaceID.
+	if keyspaceID & ^constant.ValidKeyspaceIDMask != 0 {
+		return constant.NullKeyspaceID, nil


How about adding a warning log here?

For keyspaces where the keyspace-level GC is not enabled, this might constantly happen and should not be treated as a warning.

okJiang · 2025-03-28T09:13:41Z

pkg/gc/gc_state_manager.go

+	return m.advanceGCSafePointImpl(keyspaceID, target, false)
+}
+
+func (m *GCStateManager) advanceGCSafePointImpl(keyspaceID uint32, target uint64, compatible bool) (oldGCSafePoint uint64, newGCSafePoint uint64, err error) {


Where is compatible=true?

Not yet included in this PR.
See: #9134

okJiang · 2025-03-28T09:24:24Z

pkg/gc/gc_state_manager.go

+	downgradeCompatibleMode := false
+
+	var oldTxnSafePoint uint64
+	newTxnSafePoint := target


It seems that this assignment is meaningless

okJiang · 2025-03-28T09:26:46Z

pkg/gc/gc_state_manager.go

+		if downgradeCompatibleMode {
+			err1 = wb.SetGCBarrier(keyspaceID, endpoint.NewGCBarrier(keypath.GCWorkerServiceSafePointID, newTxnSafePoint, nil))
+			if err1 != nil {
+				return err1
+			}


I am not sure. Do we need reset downgradeCompatibleMode=false here?

Looks not necessary as the transaction will exit soon. And the variable is also needed in the logs later.

okJiang · 2025-03-28T09:34:02Z

pkg/gc/gc_state_manager.go

+				zap.Uint64("newTxnSafePoint", newTxnSafePoint), zap.String("blocker", blockerDesc),
+				zap.Bool("downgradeCompatibleMode", downgradeCompatibleMode))
+		} else {
+			log.Info("txn safe point advancement unable to be blocked by the minimum blocker",


If I understand you correctly, when newTxnSafePoint != minBlocker, newTxnSafePoint = oldTxnSafePoint. So is this better?

Suggested change

log.Info("txn safe point advancement unable to be blocked by the minimum blocker",

log.Info("txn safe point can't be advanced due to the minimum blocker is less than old txn safe point",

Is this same as txn safe point is remaining unchanged?

In this log I emphasized the fact that there is something that's expected to block the txn safe point at a smaller value, but the txn safe point has exceeded it already. In this case, the major risk is that some blocker isn't effective, instead of txn safe point not being advanced.

okJiang · 2025-03-28T09:35:54Z

pkg/gc/gc_state_manager.go

+				zap.Uint64("newTxnSafePoint", newTxnSafePoint), zap.String("blocker", blockerDesc),
+				zap.Uint64("minBlockerTS", minBlocker), zap.Bool("downgradeCompatibleMode", downgradeCompatibleMode))
+		}
+	} else if newTxnSafePoint > oldTxnSafePoint {


Do we need the else?

Suggested change

} else if newTxnSafePoint > oldTxnSafePoint {

}

if newTxnSafePoint > oldTxnSafePoint {

Yes, as it's possible that it's called with a target that equals to the current value.

okJiang · 2025-03-28T09:39:45Z

pkg/gc/gc_state_manager.go

+			panic("unreachable")
+		}
+		if newTxnSafePoint == minBlocker {
+			log.Info("txn safe point advancement is being blocked",


Why we not print log once minBlocker update? Such a log is more accurate and clear

It's possible that minBlocker is updated multiple times when iterating over all potential blockers, producing too much log. We only care about the final result here.

Signed-off-by: MyonKeminta <[email protected]>

ystaticy · 2025-03-31T10:41:01Z

pkg/gc/gc_state_manager.go

+	result := GCState{
+		KeyspaceID: keyspaceID,
+	}
+	if keyspaceID != constant.NullKeyspaceID {


In this code, why do we set result.IsKeyspaceLevel = true when checking if keyspaceID != constant.NullKeyspaceID?

If it's not keyspace level GC, the call to this function must have been redirected to the null keyspace. I'll add comments to explain this later.

rleungx · 2025-04-01T05:24:34Z

pkg/gc/gc_state_manager.go

+//       deprecated when these work are all done.
+
+// GCStateManager is the manager for all kinds of states of TiKV's GC for MVCC data.
+// nolint:revive


Why not remove it?

The linter suggests removing the GC prefix which is repeative with the packagename gc, but the name StateManager looks making nonsense to me.

rleungx · 2025-04-01T05:37:19Z

pkg/gc/gc_state_manager.go

+// GCStateManager is the manager for all kinds of states of TiKV's GC for MVCC data.
+// nolint:revive
+type GCStateManager struct {
+	lock            syncutil.RWMutex


Suggested change

lock syncutil.RWMutex

syncutil.RWMutex

Is this the recommended style? It makes the Lock and Unlock methods exported and visible in external packages.

It makes sense. How about changing it to mu so that we don't need to write lock.Lock.

rleungx · 2025-04-01T05:38:44Z

pkg/gc/gc_state_manager.go

+// Returns a struct AdvanceTxnSafePointResult, which contains the old txn safe point, the target, and the new
+// txn safe point it finally made it to advance to. If there's something blocking the txn safe point from being
+// advanced to the given target, it may finally be advanced to a smaller value or remains the previous value, in which
+// case the BlockerDescription field of the AdvanceTxnSafePointResult will be set to a non-empty string describing


Do blocker and barrier have the same meaning? If so, how about unifying them?

Apart from the GC barriers, The blocker can also be a TiDB min start TS from an earlier version of TiDB that didn't follow up the refactoring. It need to be considered for compatibility concerns.

rleungx · 2025-04-01T05:53:48Z

pkg/gc/gc_state_manager.go

+		return err1
+	})
+
+	if err != nil {


return result, err

rleungx · 2025-04-01T05:54:12Z

pkg/gc/gc_state_manager.go

+
+// GCState represents the GC state of a keyspace, and additionally its keyspaceID and whether the keyspace-level GC is
+// enabled in this keyspace.
+// nolint:revive


How about removing it?

Signed-off-by: MyonKeminta <[email protected]>

MyonKeminta · 2025-04-01T07:53:03Z

/retest

rleungx · 2025-04-01T09:32:00Z

pkg/gc/gc_state_manager.go

+		}
+		if newTxnSafePoint == minBlocker {
+			log.Info("txn safe point advancement is being blocked",
+				zap.Uint64("oldTxnSafePoint", oldTxnSafePoint), zap.Uint64("target", target),


Suggested change

zap.Uint64("oldTxnSafePoint", oldTxnSafePoint), zap.Uint64("target", target),

zap.Uint64("old-txn-safe-point", oldTxnSafePoint), zap.Uint64("target", target),

Signed-off-by: MyonKeminta <[email protected]>

ti-chi-bot · 2025-04-07T10:25:55Z

@ystaticy: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot · 2025-04-08T02:39:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, rleungx, ystaticy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JmPotato,rleungx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2025-04-08T02:39:21Z

[LGTM Timeline notifier]

Timeline:

2025-04-03 07:23:54.170339939 +0000 UTC m=+1722727.854576032: ☑️ agreed by rleungx.
2025-04-08 02:39:20.697125846 +0000 UTC m=+2137654.381361942: ☑️ agreed by JmPotato.

MyonKeminta · 2025-04-08T05:49:06Z

/retest

MyonKeminta added 30 commits February 27, 2025 17:07

Add chagnes to the path definition

c70bd8e

Signed-off-by: MyonKeminta <[email protected]>

Add gc_states.go and gc_states_test.go

84b09c4

Signed-off-by: MyonKeminta <[email protected]>

fix test

70c0ce9

Signed-off-by: MyonKeminta <[email protected]>

Add more comments

ba36b14

Signed-off-by: MyonKeminta <[email protected]>

fix lint

26fc18c

Signed-off-by: MyonKeminta <[email protected]>

Add more comments

2622255

Signed-off-by: MyonKeminta <[email protected]>

Remove unnecessary commented code

db9fa7b

Signed-off-by: MyonKeminta <[email protected]>

Add more comments

364cf6d

Signed-off-by: MyonKeminta <[email protected]>

Address comments

c5653a2

Signed-off-by: MyonKeminta <[email protected]>

Update file headers

2f0c663

Signed-off-by: MyonKeminta <[email protected]>

Renaming

c1731dd

Signed-off-by: MyonKeminta <[email protected]>

Merge path functions

d9b944f

Signed-off-by: MyonKeminta <[email protected]>

rename keypath/safe_point.go to gc_states.go

f1647bf

Signed-off-by: MyonKeminta <[email protected]>

Merge branch 'master' of https://github.com/tikv/pd into m/gc-state-s…

b913e1e

…torage

Add comments explaining keyspace-level GC and unified GC

11a002e

Signed-off-by: MyonKeminta <[email protected]>

Address comments

8e4bd77

Signed-off-by: MyonKeminta <[email protected]>

Address comments

0a5acfd

Signed-off-by: MyonKeminta <[email protected]>

fix lint

42cae00

Signed-off-by: MyonKeminta <[email protected]>

Add related files

632a99d

Signed-off-by: MyonKeminta <[email protected]>

Address comments

397a53f

Signed-off-by: MyonKeminta <[email protected]>

Add test for GCBarrer.IsExpired

6445fa4

Signed-off-by: MyonKeminta <[email protected]>

Merge branch 'master' of https://github.com/tikv/pd into m/gc-state-s…

106639f

…torage

Merge branch 'm/gc-state-storage' into m/gc-state-manager

236ed1c

Merge commit 'c1c681a' into m/gc-state-manager

11080fd

fix lint

1530f53

Signed-off-by: MyonKeminta <[email protected]>

address comments from tikv#9109

8b9c144

Signed-off-by: MyonKeminta <[email protected]>

fix lint

589b58f

Signed-off-by: MyonKeminta <[email protected]>

Fix lint

75b7fda

Signed-off-by: MyonKeminta <[email protected]>

Add test case for GetGCState & GetAllKeyspacesGCStates

d36b277

Signed-off-by: MyonKeminta <[email protected]>

fix errordoc

ed5afd2

Signed-off-by: MyonKeminta <[email protected]>

JmPotato reviewed Mar 28, 2025

View reviewed changes

okJiang reviewed Mar 28, 2025

View reviewed changes

MyonKeminta added 4 commits March 31, 2025 13:07

Address comments

a1776b3

Signed-off-by: MyonKeminta <[email protected]>

Address comments

66cf8e9

Signed-off-by: MyonKeminta <[email protected]>

Remove saturatingDuration

051eb23

Signed-off-by: MyonKeminta <[email protected]>

Rename keyspace GC management type configuration values

cc2c527

Signed-off-by: MyonKeminta <[email protected]>

ystaticy reviewed Mar 31, 2025

View reviewed changes

rleungx reviewed Apr 1, 2025

View reviewed changes

MyonKeminta added 2 commits April 1, 2025 14:43

Add comments

4841da6

Signed-off-by: MyonKeminta <[email protected]>

Address partial of the comments

355d092

Signed-off-by: MyonKeminta <[email protected]>

rleungx reviewed Apr 1, 2025

View reviewed changes

MyonKeminta added 5 commits April 1, 2025 17:46

Address comments

369726b

Signed-off-by: MyonKeminta <[email protected]>

Move log printing into a separated function

8553c0a

Signed-off-by: MyonKeminta <[email protected]>

fix cases in logs

51e8561

Signed-off-by: MyonKeminta <[email protected]>

fix lint

63747ef

Signed-off-by: MyonKeminta <[email protected]>

fix lint

885e1fc

Signed-off-by: MyonKeminta <[email protected]>

rleungx approved these changes Apr 3, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Apr 3, 2025

ystaticy approved these changes Apr 7, 2025

View reviewed changes

JmPotato approved these changes Apr 8, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 8, 2025

Merge branch 'master' into m/gc-state-manager-partial

fbcfcb8

ti-chi-bot bot merged commit 9092ccc into tikv:master Apr 8, 2025
25 checks passed

	log.Info("txn safe point advancement unable to be blocked by the minimum blocker",
	log.Info("txn safe point can't be advanced due to the minimum blocker is less than old txn safe point",

	} else if newTxnSafePoint > oldTxnSafePoint {
	}
	if newTxnSafePoint > oldTxnSafePoint {

	zap.Uint64("oldTxnSafePoint", oldTxnSafePoint), zap.Uint64("target", target),
	zap.Uint64("old-txn-safe-point", oldTxnSafePoint), zap.Uint64("target", target),

gc: Add GCStateManager which is planned to be the replacement of SafePointManager #9169

gc: Add GCStateManager which is planned to be the replacement of SafePointManager #9169

Conversation

MyonKeminta commented Mar 24, 2025

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

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

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 Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ystaticy Mar 31, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta commented Apr 1, 2025

Choose a reason for hiding this comment

ti-chi-bot bot commented Apr 7, 2025

ti-chi-bot bot commented Apr 8, 2025

ti-chi-bot bot commented Apr 8, 2025

[LGTM Timeline notifier]

MyonKeminta commented Apr 8, 2025

gc: Add `GCStateManager` which is planned to be the replacement of `SafePointManager` #9169

gc: Add `GCStateManager` which is planned to be the replacement of `SafePointManager` #9169

MyonKeminta Mar 31, 2025 •

edited

Loading

ystaticy Mar 31, 2025 •

edited

Loading