*: remove limiting process id for auto analyze #54902

hawkingrei · 2024-07-25T06:55:48Z

What problem does this PR solve?

Issue Number: ref #53460

Problem Summary:

What changed and how does it work?

Add GlobalAutoAnalyzeProcessList to collect all auto analyze process IDs.
Remove the restriction of having only one process ID for auto analyze and release the used process ID.

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.

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

codecov · 2024-07-25T08:03:29Z

Codecov Report

Attention: Patch coverage is 89.39394% with 7 lines in your changes missing coverage. Please review.

Project coverage is 73.3113%. Comparing base (7e73ddc) to head (2358073).
Report is 27 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #54902        +/-   ##
================================================
+ Coverage   72.7907%   73.3113%   +0.5206%     
================================================
  Files          1558       1568        +10     
  Lines        438406     446381      +7975     
================================================
+ Hits         319119     327248      +8129     
+ Misses        99643      98700       -943     
- Partials      19644      20433       +789

Flag	Coverage Δ
integration	`20.6010% <13.8461%> (?)`

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.1135% <ø> (+6.2092%)`	⬆️

hawkingrei · 2024-07-25T09:11:50Z

/retest

hawkingrei · 2024-07-25T09:17:26Z

pkg/executor/analyze_col_v2.go

@@ -854,7 +855,9 @@ func readDataAndSendTask(ctx sessionctx.Context, handler *tableResultHandler, me
 	for {
 		failpoint.Inject("mockKillRunningV2AnalyzeJob", func() {


Here are the tests.

pkg/domain/domain.go

pkg/statistics/handle/util/auto_analyze_proc_id_generator.go

Signed-off-by: Weizhen Wang <[email protected]>

pingyu

reservedConnAnalyze can be removed. Rest LGTM~

ti-chi-bot · 2024-07-27T02:21:19Z

[LGTM Timeline notifier]

Timeline:

2024-07-26 04:32:28.888426399 +0000 UTC m=+1192370.879367870: ☑️ agreed by xhebox.
2024-07-27 02:21:18.180276505 +0000 UTC m=+58762.307336681: ☑️ agreed by pingyu.

Signed-off-by: Weizhen Wang <[email protected]>

hawkingrei · 2024-07-29T03:07:58Z

reservedConnAnalyze can be removed. Rest LGTM~

I have removed it. Thanks.

hawkingrei · 2024-07-29T08:21:44Z

pkg/statistics/handle/util/auto_analyze_proc_id_generator.go

+var GlobalAutoAnalyzeProcessList = newGlobalAutoAnalyzeProcessList()
+
+type globalAutoAnalyzeProcessList struct {
+	processes map[uint64]struct{}


We use global thread-safe map to record the running auto analyze task.

why not use a sync.map

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

so two common use cases are not the same as us.

hawkingrei · 2024-07-29T08:32:54Z

@winoros PTAL

Signed-off-by: Weizhen Wang <[email protected]>

ti-chi-bot · 2024-07-29T10:05:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, pingyu, qw4990, xhebox

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~~ [lance6716,qw4990,xhebox]
~~pkg/domain/OWNERS~~ [lance6716]
~~pkg/planner/OWNERS~~ [qw4990]
~~pkg/statistics/OWNERS~~ [qw4990]

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

Signed-off-by: Weizhen Wang <[email protected]>

ref pingcap#53460

ti-chi-bot bot removed do-not-merge/needs-tests-checked do-not-merge/needs-linked-issue labels Jul 25, 2024

hawkingrei commented Jul 25, 2024

View reviewed changes

hawkingrei changed the title *: remove limiting process id for analyze *: remove limiting process id for auto analyze Jul 25, 2024

hawkingrei requested review from pingyu, breezewish and winoros July 25, 2024 09:29

winoros reviewed Jul 25, 2024

View reviewed changes

pkg/domain/domain.go Outdated Show resolved Hide resolved

pkg/statistics/handle/util/auto_analyze_proc_id_generator.go Show resolved Hide resolved

hawkingrei added 8 commits July 26, 2024 11:04

*: remove limiting process id for analyze

e81d9a2

Signed-off-by: Weizhen Wang <[email protected]>

update

49fcc42

Signed-off-by: Weizhen Wang <[email protected]>

update

fe68215

Signed-off-by: Weizhen Wang <[email protected]>

update

9e5b51e

Signed-off-by: Weizhen Wang <[email protected]>

update

ec513b4

Signed-off-by: Weizhen Wang <[email protected]>

update

be16f95

Signed-off-by: Weizhen Wang <[email protected]>

update

d47567a

Signed-off-by: Weizhen Wang <[email protected]>

update

2a68900

Signed-off-by: Weizhen Wang <[email protected]>

hawkingrei force-pushed the remove_limit_for_autoanalyze_in_procs_id branch from 8855661 to 2a68900 Compare July 26, 2024 03:19

hawkingrei added 2 commits July 26, 2024 11:40

update

bf1c5bb

Signed-off-by: Weizhen Wang <[email protected]>

update

a6eb9e9

Signed-off-by: Weizhen Wang <[email protected]>

xhebox approved these changes Jul 26, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 26, 2024

hawkingrei requested a review from winoros July 26, 2024 06:44

pingyu approved these changes Jul 27, 2024

View reviewed changes

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

update

4409d44

Signed-off-by: Weizhen Wang <[email protected]>

lance6716 approved these changes Jul 29, 2024

View reviewed changes

hawkingrei commented Jul 29, 2024

View reviewed changes

update

bdede1f

Signed-off-by: Weizhen Wang <[email protected]>

qw4990 approved these changes Jul 29, 2024

View reviewed changes

ti-chi-bot bot added the approved label Jul 29, 2024

update

2358073

Signed-off-by: Weizhen Wang <[email protected]>

ti-chi-bot bot merged commit 273ca66 into pingcap:master Jul 29, 2024
23 checks passed

hawkingrei deleted the remove_limit_for_autoanalyze_in_procs_id branch July 29, 2024 16:06

hawkingrei added a commit to hawkingrei/tidb that referenced this pull request Aug 1, 2024

*: remove limiting process id for auto analyze (pingcap#54902)

4e7c007

ref pingcap#53460

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: remove limiting process id for auto analyze #54902

*: remove limiting process id for auto analyze #54902

hawkingrei commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

hawkingrei commented Jul 25, 2024

hawkingrei Jul 25, 2024

pingyu left a comment

ti-chi-bot bot commented Jul 27, 2024

hawkingrei commented Jul 29, 2024

hawkingrei Jul 29, 2024 •

edited

Loading

AilinKid Jul 30, 2024

hawkingrei Jul 30, 2024

hawkingrei commented Jul 29, 2024

ti-chi-bot bot commented Jul 29, 2024

		@@ -854,7 +855,9 @@ func readDataAndSendTask(ctx sessionctx.Context, handler *tableResultHandler, me
		for {
		failpoint.Inject("mockKillRunningV2AnalyzeJob", func() {

*: remove limiting process id for auto analyze #54902

*: remove limiting process id for auto analyze #54902

Conversation

hawkingrei commented Jul 25, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Jul 25, 2024 • edited Loading

Codecov Report

hawkingrei commented Jul 25, 2024

hawkingrei Jul 25, 2024

Choose a reason for hiding this comment

pingyu left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 27, 2024

[LGTM Timeline notifier]

hawkingrei commented Jul 29, 2024

hawkingrei Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

AilinKid Jul 30, 2024

Choose a reason for hiding this comment

hawkingrei Jul 30, 2024

Choose a reason for hiding this comment

hawkingrei commented Jul 29, 2024

ti-chi-bot bot commented Jul 29, 2024

hawkingrei commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

hawkingrei Jul 29, 2024 •

edited

Loading