transaction: add hook for async commit to track the life cycle of the async-commit goroutine and secondary lock cleanup goroutine

[YangKeao]

This PR adds two hooks to execute before the start of async-commit goroutine and after the finish of async-commit goroutines.

It can be used to add some external logic to track the lifecycle of the async-commit goroutines. For example, in TiDB we'll need to wait for the background goroutines to finish before shutdown.

[you06]

What about the non-async-commit 2PC, maybe such secondaries commit goroutines also need to be track in TiDB.

[YangKeao]

What about the non-async-commit 2PC, maybe such secondaries commit goroutines also need to be track in TiDB.

@you06

TiDB has a mechanism to wait for all connections to finish the current executing statement/transactions (ref (*Server).DrainClients, and (*Server).inShutdownMode). I think it can guarantee that the non-async-commit 2PC has committed (or rollbacked) before killing connections.

(Or did I have some misunderstandings about the transactions?)

[you06]

I took a look at (*Server).DrainClients, seems it will wait all the connections quit.

In common 2pc transactions, the txn commit success is responded to user once the primary key is committed, and the secondaries are committed in background(note the backoffer is created from store's context, it's not related to user connection's context).

client-go/txnkv/transaction/2pc.go

Lines 1012 to 1022 in 41d133b

	// Already spawned a goroutine for async commit transaction.
	if actionIsCommit && !actionCommit.retry && !c.isAsyncCommit() {
	secondaryBo := retry.NewBackofferWithVars(c.store.Ctx(), CommitSecondaryMaxBackoff, c.txn.vars)
	if c.store.IsClose() {
	logutil.Logger(bo.GetCtx()).Warn("the store is closed",
	zap.Uint64("startTS", c.startTS), zap.Uint64("commitTS", c.commitTS),
	zap.Uint64("sessionID", c.sessionID))
	return nil
	}
	c.store.WaitGroup().Add(1)
	err = c.store.Go(func() {

If the store is killed before the secondaries are commited, the prewrite locks of secondaries will be leave, need to be cleaned by other read transactions or GC.

[YangKeao]

I took a look at (*Server).DrainClients, seems it will wait all the connections quit.

In common 2pc transactions, the txn commit success is responded to user once the primary key is committed, and the secondaries are committed in background(note the backoffer is created from store's context, it's not related to user connection's context).

client-go/txnkv/transaction/2pc.go

Lines 1012 to 1022 in 41d133b

	// Already spawned a goroutine for async commit transaction.
	if actionIsCommit && !actionCommit.retry && !c.isAsyncCommit() {
	secondaryBo := retry.NewBackofferWithVars(c.store.Ctx(), CommitSecondaryMaxBackoff, c.txn.vars)
	if c.store.IsClose() {
	logutil.Logger(bo.GetCtx()).Warn("the store is closed",
	zap.Uint64("startTS", c.startTS), zap.Uint64("commitTS", c.commitTS),
	zap.Uint64("sessionID", c.sessionID))
	return nil
	}
	c.store.WaitGroup().Add(1)
	err = c.store.Go(func() {

If the store is killed before the secondaries are commited, the prewrite locks of secondaries will be leave, need to be cleaned by other read transactions or GC.

@you06 Oh! I got it. They'll also need to be tracked. I'll try to update this PR.

[YangKeao]

/cc @you06

PTAL

[YangKeao]

TiDB side PR: pingcap/tidb#55608

[YangKeao]

/retest

[you06]

One more thing is the max backoff duration is 40s, I think the default graceful shutdown wait time should larger than it.

client-go/txnkv/transaction/2pc.go

Line 91 in 41d133b

CommitMaxBackoff = uint64(40000)

[YangKeao]

One more thing is the max backoff duration is 40s, I think the default graceful shutdown wait time should larger than it.

@you06 Now, TiDB waits for at most 15s and it's hard coded and not configurable.

It's a trade-off between the rolling-restart speed and the regression caused by leaking locks. Someone thinks it's better to rolling restart faster and they can accept a little regression, but someone may think it doesn't matter if the rolling restart takes longer time, but it shouldn't affect any metrics.

At least, it's still not a big issue if some locks leak when these keys take too much time to commit 🤔.

[you06]

At least, it's still not a big issue if some locks leak when these keys take too much time to commit 🤔.

Agree. My point is that the default configuration is more understandable(and make sense) if the max graceful wait time is larger than commit backoff time. But it's not a big issue because if commit takes more than 15s, there must be a more serious issue rather than the unresolved locks.

[you06]

This implementation LGTM
@cfzjywxk what's your opinion of the new added hooks.

[YangKeao]

/cc @cfzjywxk

[cfzjywxk]

To avoid hook at all the goroutines, another approach is to hook at the beginning of execution function in 2pc.go and the defer block of it, using c.store.Waitgroup().wait() to ensure the background goroutines are finished in the defer hook.

In the current implementation, the cleanup goroutine needs to be hooked too.

[YangKeao]

Done. I've added a new spawn method for the txn as a helper function to track all goroutines. It'll maintain the lifecycle hooks and the inc/dec of txn.store.WaitGroup().

Also provide a spawnWithStorePool to call txn.store.Go internally. I don't modify the existing logic about whether to use the store pool.

[YangKeao]

(It's called spawn but not go, because I want it to be a private function, but go is a keyword so I cannot use it as a function name.)

[YangKeao]

/hold

Let me do some refactor 😃

[YangKeao]

/unhold

[cfzjywxk]

LGTM

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, you06

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 [cfzjywxk,you06]

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]

[LGTM Timeline notifier]

Timeline:

• 2024-09-04 04:29:15.243630492 +0000 UTC m=+418679.761683413: ☑️ agreed by you06.
• 2024-09-04 09:18:03.253766875 +0000 UTC m=+436007.771819798: ☑️ agreed by cfzjywxk.