domain: support GC runaway record #44784

CabinfeverB · 2023-06-19T07:50:39Z

What problem does this PR solve?

Issue Number: ref #43691 close #44804
As RFC shown, tidb_runaway_quarantined_watch only keeps active quarantined queries. So we should support gc.

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

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

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

glorv · 2023-06-19T07:58:47Z

domain/domain.go

+		}
+	}
+	gcRunawayRecords := func() {
+		sql := "DELETE FROM mysql.tidb_runaway_queries WHERE time < CONVERT_TZ(%?, '+00:00', @@TIME_ZONE)"


I think the runaway records should be kept for a period of time. And, btw, it is possible that there are too many outdated records, then delete all of them in a single txn can exceed tidb's txn max size, so I suggest manually select and delete the records in batch.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB · 2023-06-19T14:17:43Z

cc @nolouch
@lcwangchao Could u take a lock at the modification about ttl/cache/table.go

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB · 2023-06-19T14:53:01Z

/retest

Connor1996 · 2023-06-19T16:35:16Z

domain/domain.go

 func (do *Domain) runawayRecordFlushLoop() {
 	defer util.Recover(metrics.LabelDomain, "runawayRecordFlushLoop", nil, false)
+
+	quarantineRecordGCInterval := time.Second * 15   //time.Minute * 15


Connor1996

And don't forget to update doc as well

glorv

Please add necessary test cases to cover the new logic

glorv · 2023-06-20T02:42:19Z

domain/domain.go

+	var leftRows [][]types.Datum
+	for {
+		sql := ""
+		limit := int(variable.TTLScanBatchSize.Load())


Better not depend on variable of other feature, you can define a const instead

glorv · 2023-06-20T02:43:09Z

domain/domain.go

+	tbCIStr := model.NewCIStr(tableName)
+	tbl, err := do.InfoSchema().TableByName(systemSchemaCIStr, tbCIStr)
+	if err != nil {
+		logutil.BgLogger().Info("delete system table failed", zap.String("table", tableName), zap.Error(err))


Suggested change

logutil.BgLogger().Info("delete system table failed", zap.String("table", tableName), zap.Error(err))

logutil.BgLogger().Error("delete system table failed", zap.String("table", tableName), zap.Error(err))

glorv · 2023-06-20T02:59:57Z

domain/domain.go

 func (do *Domain) runawayRecordFlushLoop() {
 	defer util.Recover(metrics.LabelDomain, "runawayRecordFlushLoop", nil, false)
+
+	quarantineRecordGCInterval := time.Second * 15   //time.Minute * 15
+	runawayRecordGCInterval := time.Second * 15      //time.Hour * 24


Why this small duration?

glorv · 2023-06-20T03:02:52Z

domain/domain.go

@@ -1294,6 +1389,10 @@ func (do *Domain) runawayRecordFlushLoop() {
 			// flush as soon as possible.
 			if len(quarantineRecordCh) == 0 || len(quarantineRecords) >= flushThrehold {
 				flushQuarantineRecords()
+				if owner.IsOwner() {


Why need gc here, don't get the point

Because when the watch is updated, it may be a new identify, at this time, I think I can delete the old one immediately.

glorv · 2023-06-20T03:03:33Z

domain/domain.go

+				runawayRecordFluashTimer.Reset(time.Second)
+			}
+		case <-runawayRecordGCTicker.C:
+			if owner.IsOwner() {


better to move this check into gcSystemTable to avoid this check everywhere

glorv · 2023-06-20T03:04:56Z

domain/domain.go

+			}
+		case <-runawayRecordGCTicker.C:
+			if owner.IsOwner() {
+				do.gcSystemTable("tidb_runaway_queries", "time", runawayRecordExpiredDuration)


As gcSystemTable can be very slow, to avoid blocking handling other events, better spawn a new routine to do this job.

glorv · 2023-06-20T03:06:26Z

domain/domain.go

+		}
+
+		for len(leftRows) > 0 {
+			maxBatch := 100


Since the previous select is already paged, why do paging again here?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

domain/domain.go

glorv · 2023-06-20T10:00:29Z

/retest

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB · 2023-06-20T10:08:50Z

/test build

CabinfeverB · 2023-06-20T10:55:20Z

/test unit-test

ti-chi-bot · 2023-06-20T10:55:23Z

@CabinfeverB: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test canary-scan-security
/test check-dev
/test check-dev2
/test mysql-test
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-integration-br-test
/test pull-integration-mysql-test
/test unit-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test unit-test

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/test-infra repository.

CabinfeverB · 2023-06-20T10:55:48Z

/test unit-test

glorv · 2023-06-20T11:25:38Z

/retest

ti-chi-bot · 2023-06-20T13:36:57Z

[LGTM Timeline notifier]

Timeline:

2023-06-20 09:59:36.107444779 +0000 UTC m=+96941.508695226: ☑️ agreed by glorv.
2023-06-20 13:36:56.52654347 +0000 UTC m=+109981.927793919: ☑️ agreed by nolouch.

Connor1996 · 2023-06-20T22:26:12Z

domain/domain.go

@@ -1248,11 +1251,116 @@ func (do *Domain) SetOnClose(onClose func()) {
 	do.onClose = onClose
 }

+const (


move it to resource group package?

ti-chi-bot · 2023-06-21T03:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glorv, nolouch, qw4990

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~~ [glorv,qw4990]

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 · 2023-06-21T04:02:15Z

In response to a cherrypick label: new pull request created to branch release-7.2: #44860.

ref #43691, close #44804

gc runaway record

5221b4b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB requested a review from a team as a code owner June 19, 2023 07:50

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2023

glorv reviewed Jun 19, 2023

View reviewed changes

gc runaway record

1136254

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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 Jun 19, 2023

CabinfeverB added 3 commits June 19, 2023 21:44

gc runaway record

92e0e25

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

gc runaway record

971040a

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

gc runaway record

7d79127

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB added 2 commits June 19, 2023 22:24

gc runaway record

ee61a30

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

gc runaway record

c0a73f8

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'master' into resource_manager/runaway_gc

25a25fc

ti-chi-bot bot added the do-not-merge/needs-triage-completed label Jun 19, 2023

Connor1996 reviewed Jun 19, 2023

View reviewed changes

glorv reviewed Jun 20, 2023

View reviewed changes

add test

11e8c95

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jun 20, 2023

add test

c7effa2

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch reviewed Jun 20, 2023

View reviewed changes

domain/domain.go Outdated Show resolved Hide resolved

domain/domain.go Outdated Show resolved Hide resolved

glorv approved these changes Jun 20, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 20, 2023

address comment

644ff7f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch approved these changes Jun 20, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 20, 2023

Connor1996 reviewed Jun 20, 2023

View reviewed changes

qw4990 approved these changes Jun 21, 2023

View reviewed changes

glorv added the needs-cherry-pick-release-7.2 label Jun 21, 2023

ti-chi-bot bot merged commit 47101e8 into pingcap:master Jun 21, 2023

ti-chi-bot mentioned this pull request Jun 21, 2023

domain: support GC runaway record (#44784) #44860

Merged

12 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jun 21, 2023

domain: support GC runaway record (#44784) (#44860)

847f543

ref #43691, close #44804

Connor1996 mentioned this pull request Jun 21, 2023

Runaway query control based on resource group #43691

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

domain: support GC runaway record #44784

domain: support GC runaway record #44784

CabinfeverB commented Jun 19, 2023 •

edited

Loading

glorv Jun 19, 2023 •

edited

Loading

CabinfeverB commented Jun 19, 2023

CabinfeverB commented Jun 19, 2023

Connor1996 Jun 19, 2023

Connor1996 left a comment

glorv left a comment

glorv Jun 20, 2023

glorv Jun 20, 2023

glorv Jun 20, 2023

glorv Jun 20, 2023

CabinfeverB Jun 20, 2023

glorv Jun 20, 2023

glorv Jun 20, 2023

glorv Jun 20, 2023

glorv commented Jun 20, 2023

CabinfeverB commented Jun 20, 2023

CabinfeverB commented Jun 20, 2023

ti-chi-bot bot commented Jun 20, 2023

/test unit-test

CabinfeverB commented Jun 20, 2023

glorv commented Jun 20, 2023

ti-chi-bot bot commented Jun 20, 2023

Connor1996 Jun 20, 2023

ti-chi-bot bot commented Jun 21, 2023

ti-chi-bot commented Jun 21, 2023

	logutil.BgLogger().Info("delete system table failed", zap.String("table", tableName), zap.Error(err))
	logutil.BgLogger().Error("delete system table failed", zap.String("table", tableName), zap.Error(err))

domain: support GC runaway record #44784

domain: support GC runaway record #44784

Conversation

CabinfeverB commented Jun 19, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

glorv Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

CabinfeverB commented Jun 19, 2023

CabinfeverB commented Jun 19, 2023

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

glorv left a 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

glorv commented Jun 20, 2023

CabinfeverB commented Jun 20, 2023

CabinfeverB commented Jun 20, 2023

/test unit-test

ti-chi-bot bot commented Jun 20, 2023

/test unit-test

CabinfeverB commented Jun 20, 2023

glorv commented Jun 20, 2023

ti-chi-bot bot commented Jun 20, 2023

[LGTM Timeline notifier]

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 21, 2023

ti-chi-bot commented Jun 21, 2023

CabinfeverB commented Jun 19, 2023 •

edited

Loading

glorv Jun 19, 2023 •

edited

Loading