server: add hooks to wait for background commit goroutines #55608

YangKeao · 2024-08-23T04:49:28Z

What problem does this PR solve?

Issue Number: close #55607

Problem Summary:

TiDB will leak some lock if the transaction is not committed/rollbacked successfully. The graceful shutdown routine didn't wait for the background async-commit (and secondary key lock cleanup) goroutines to finish, so it'd be better to also wait for them to avoid leaking the locks.

What changed and how does it work?

Use the hooks of the async-commit / secondary lock cleanup goroutines to track the lifecycle of them.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

TODO

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Fix the issue that stopping TiDB may leak the transaction lock.

YangKeao · 2024-08-23T04:50:03Z

Ref tikv/client-go#1432

YangKeao · 2024-08-26T07:35:28Z

/hold

client-go PR is not merged yet

YangKeao · 2024-09-05T04:58:22Z

/unhold

YangKeao · 2024-09-05T06:45:18Z

/retest

YangKeao · 2024-09-05T06:52:16Z

/retest

codecov · 2024-09-05T06:55:09Z

Codecov Report

Attention: Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.4990%. Comparing base (dc03726) to head (7c9bd86).
Report is 3 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55608        +/-   ##
================================================
+ Coverage   73.4160%   75.4990%   +2.0830%     
================================================
  Files          1623       1623                
  Lines        447913     448314       +401     
================================================
+ Hits         328840     338473      +9633     
+ Misses        98931      89312      -9619     
- Partials      20142      20529       +387

Flag	Coverage Δ
integration	`48.9418% <53.3333%> (?)`
unit	`72.6210% <96.6666%> (+0.1144%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.3153% <ø> (+6.7827%)`	⬆️

you06

rest LGTM

you06 · 2024-09-13T09:30:57Z

pkg/session/session.go

@@ -223,6 +223,9 @@ type session struct {
 	sandBoxMode bool

 	cursorTracker cursor.Tracker
+
+	// Used to wait for all async commit background jobs to finish.
+	asyncCommitWaitGroup sync.WaitGroup


May just name it commitWaitGroup, because not only async-commit txns are tracked.

ti-chi-bot · 2024-09-28T18:27:39Z

PR needs rebase.

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 · 2024-09-30T06:42:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, 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~~ [Defined2014,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 · 2024-09-30T06:42:55Z

[LGTM Timeline notifier]

Timeline:

2024-09-30 06:05:50.848509539 +0000 UTC m=+248506.268722546: ☑️ agreed by you06.
2024-09-30 06:42:54.368235751 +0000 UTC m=+250729.788448761: ☑️ agreed by Defined2014.

Signed-off-by: Yang Keao <[email protected]>

ti-chi-bot bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 23, 2024

YangKeao added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2024

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 23, 2024

YangKeao force-pushed the wait-async-commit-finish branch from d2cfbf3 to 7c4c2b6 Compare August 23, 2024 08:38

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2024

YangKeao mentioned this pull request Aug 26, 2024

stale lock during tidb rolling restart results in resolved ts lag and cdc lag increase #52108

Closed

YangKeao force-pushed the wait-async-commit-finish branch from 7c4c2b6 to 4dc340c Compare August 26, 2024 07:30

YangKeao changed the title ~~server: add hooks to wait for async commit goroutines (WIP)~~ server: add hooks to wait for background commit goroutines Aug 26, 2024

ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 26, 2024

YangKeao mentioned this pull request Aug 26, 2024

transaction: add hook for async commit to track the life cycle of the async-commit goroutine and secondary lock cleanup goroutine tikv/client-go#1432

Merged

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2024

YangKeao force-pushed the wait-async-commit-finish branch from 4dc340c to b126ae0 Compare August 26, 2024 07:52

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 26, 2024

YangKeao force-pushed the wait-async-commit-finish branch from b126ae0 to 0660748 Compare August 28, 2024 07:09

YangKeao force-pushed the wait-async-commit-finish branch 2 times, most recently from 1d810f9 to bd33e80 Compare September 5, 2024 04:57

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 5, 2024

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2024

YangKeao force-pushed the wait-async-commit-finish branch from bd33e80 to 20d1887 Compare September 5, 2024 05:15

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 5, 2024

YangKeao force-pushed the wait-async-commit-finish branch 2 times, most recently from 1bfb69b to 8ee42a8 Compare September 5, 2024 06:07

ti-chi-bot bot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 5, 2024

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 5, 2024

YangKeao force-pushed the wait-async-commit-finish branch from 8ee42a8 to 4f6e6f5 Compare September 5, 2024 07:19

YangKeao requested review from you06 and bb7133 September 5, 2024 08:23

you06 reviewed Sep 13, 2024

View reviewed changes

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2024

YangKeao force-pushed the wait-async-commit-finish branch 2 times, most recently from 95ed55b to ecae869 Compare September 30, 2024 04:57

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2024

YangKeao requested a review from you06 September 30, 2024 05:59

you06 approved these changes Sep 30, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 30, 2024

YangKeao requested review from Defined2014 and removed request for bb7133 September 30, 2024 06:34

Defined2014 approved these changes Sep 30, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 30, 2024

YangKeao removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2024

add hooks to wait for async commit goroutines

7c9bd86

Signed-off-by: Yang Keao <[email protected]>

YangKeao force-pushed the wait-async-commit-finish branch from ecae869 to 7c9bd86 Compare September 30, 2024 06:49

ti-chi-bot bot merged commit cd33d79 into pingcap:master Sep 30, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: add hooks to wait for background commit goroutines #55608

server: add hooks to wait for background commit goroutines #55608

YangKeao commented Aug 23, 2024 •

edited by Defined2014

Loading

YangKeao commented Aug 23, 2024

YangKeao commented Aug 26, 2024

YangKeao commented Sep 5, 2024

YangKeao commented Sep 5, 2024

YangKeao commented Sep 5, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

you06 left a comment

you06 Sep 13, 2024

ti-chi-bot bot commented Sep 28, 2024

ti-chi-bot bot commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

server: add hooks to wait for background commit goroutines #55608

server: add hooks to wait for background commit goroutines #55608

Conversation

YangKeao commented Aug 23, 2024 • edited by Defined2014 Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

YangKeao commented Aug 23, 2024

YangKeao commented Aug 26, 2024

YangKeao commented Sep 5, 2024

YangKeao commented Sep 5, 2024

YangKeao commented Sep 5, 2024

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

you06 left a comment

Choose a reason for hiding this comment

you06 Sep 13, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 28, 2024

ti-chi-bot bot commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

[LGTM Timeline notifier]

YangKeao commented Aug 23, 2024 •

edited by Defined2014

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading