*: global runaway watch by system table and impl exector for `query watch` #45465

CabinfeverB · 2023-07-19T07:45:55Z

What problem does this PR solve?

Issue Number: ref #43691

Problem Summary:

What is changed and how it works?

global runaway watch by system table

create table mysql.tidb_runaway_watch and table mysql.tidb_runaway_watch_done to persist runaway watch and deletion of runaway watch. And all TiDb servers read both tables periodically to sync runaway watch among TiDBs.

impl exector for query watch

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
About runaway watch sync:
Execute query on TiDB1, and then execute query on TiDB2

We can see TiDB1 returns error 8253, and TiDB2 returns error 8254. And mysql.tidb_runaway_watch has the record.
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>

tiprow · 2023-07-19T07:46:21Z

Hi @CabinfeverB. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

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

CabinfeverB · 2023-07-19T08:42:22Z

cc @nolouch @glorv @Connor1996

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

session/bootstrap.go

ddl/util/util.go

domain/resourcegroup/runaway.go

Connor1996 · 2023-07-27T21:31:16Z

domain/resourcegroup/runaway.go

+		rm.watchList.Set(key, record, ttl)
+		rm.queryLock.Unlock()
+	} else {
+		if rm.watchList.Get(key) == nil {


Should it be in lock?

This is first check, because we generally believe that in most cases, we will not add a watch list to a key repeatedly

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

Connor1996

LGTM

ti-chi-bot · 2023-07-31T06:52:25Z

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

In response to this:

LGTM

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.

glorv · 2023-07-31T06:59:11Z

domain/resourcegroup/runaway.go

+
+	force := false
+	// The manual record replaces the old record.
+	force = record.Source == ManualSource || record.Source == rm.serverID


What is the priority between manually added records and watched records?

manually added records has higher priority. I will change it.

glorv · 2023-07-31T07:01:11Z

domain/resourcegroup/runaway.go

+	rm.addWatchList(record, ttl, force)
+}
+
+// RemoveWatch is used to remove watch items from system table.


misleading comment, this function only remove records from in-memory cache

Well, it does have ambiguity

glorv · 2023-07-31T07:02:43Z

domain/resourcegroup/runaway.go

 func (rm *RunawayManager) markRunaway(resourceGroupName, originalSQL, planDigest string, action string, matchType RunawayMatchType, now *time.Time) {
+	source := rm.serverID
+	if len(source) > 128 {


Why need truncate here?

I think the server IP in k8s will be domain name, so the length of it will be very long. And the length of feild is 128, do we need to change it to blob?

You should set a field length that is long enough to store the data

OK, let's set 512 same as analyze job

glorv · 2023-07-31T07:11:07Z

domain/runaway.go

+			if r.ID > 0 {
+				return r.ID, nil
+			}
+		case err := <-do.runawaySyncer.doneChan:


What if multiple add watches happen at the same time, how can you ensure you get the right message from this chan?

Can't ensure. If keys of records are different and it gets wrong message , It can't get watch and will try wo get watch later. If keys are same, refer to https://github.com/pingcap/tidb/pull/45465/files#r1277100060

As you only send a message to notifyChan only once, one routine receives another routine's return message and drops it, then the other routine is blocked, this is a more severe situation.

glorv · 2023-07-31T07:14:30Z

domain/runaway.go

+}
+
+func (do *Domain) AddRunawayWatch(record *resourcegroup.QuarantineRecord) (int64, error) {
+	if err := do.handleRunawayWatch(record); err != nil {


Don't get what you want to explain, but it's still clear that the current logic cannot ensure the procedure to be atomic, so it can deliver wrong result

glorv · 2023-07-31T07:33:33Z

executor/internal/querywatch/query_watch.go

+		return infoschema.ErrResourceGroupNotExists.GenWithStackByArgs(record.ResourceGroupName)
+	}
+	if record.Action == rmpb.RunawayAction_NoneAction {
+		if rg.RunawaySettings == nil {


What happens if when the RunawaySettings is not nil when adding the record and then changed to none, then what is the action of the record

This watch will be uselss but not be removed from watch list. You can see BeforeExecutor, If no match action, it will do nothing.

glorv · 2023-07-31T07:35:57Z

session/bootstrap.go

-		start_time TIMESTAMP NOT NULL,
-		end_time TIMESTAMP NOT NULL,
-		watch varchar(12) NOT NULL,
+		start_time datetime(6) NOT NULL,


So what is the benefit for this change, I didn't see any other table to use datetime to represent time

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

glorv · 2023-07-31T12:25:44Z

domain/resourcegroup/runaway.go

+	if item == nil {
+		return false, 0
+	}
+	return true, item.Action


For a manual add item without action, the action is none, in this case you should use the setting's action instead. BTW, if we support let manual record reflect the change of query limit's action change, I would expect watch record reflect this change too.

You can see L461-463.
make sense, I will update it.

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

CabinfeverB · 2023-07-31T17:00:30Z

session/bootstrap.go

@@ -2773,10 +2777,16 @@ func upgradeToVer171(s Session, ver int64) {
 	if ver >= version171 {
 		return
 	}
+	mustExecute(s, "ALTER TABLE mysql.tidb_runaway_queries CHANGE COLUMN `tidb_server` `tidb_server` varchar(512)")


Due to TestUpgradeVersionForResumeJob, I try to pass it by add two upgrade functions. Maybe it's not a good method, I just want to pass test first.

glorv · 2023-08-01T01:56:25Z

domain/runaway.go

+	failpoint.Inject("FastRunawayGC", func() {
+		expiredDuration = time.Second * 1
+	})
+	expiredTime := time.Now().Add(-expiredDuration)


Does the timezone offset also impact this?

Now gc clean up loop only affects tidb_runaway_queries, it does not affect correctness.

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

ti-chi-bot · 2023-08-01T04:39:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Connor1996, glorv, 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-08-01T04:39:36Z

[LGTM Timeline notifier]

Timeline:

2023-07-31 15:15:35.61207922 +0000 UTC m=+112019.554427751: ☑️ agreed by qw4990.
2023-08-01 04:39:35.271048607 +0000 UTC m=+160259.213397137: ☑️ agreed by glorv.

CabinfeverB · 2023-08-01T04:45:04Z

/retest-required

tiprow · 2023-08-01T04:45:36Z

@CabinfeverB: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest-required

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.

global runaway watch by system table

77ccef4

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

CabinfeverB requested a review from a team as a code owner July 19, 2023 07:45

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2023

global runaway watch by system table

0d25493

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

CabinfeverB marked this pull request as draft July 19, 2023 07:46

don't delete

ab2e5be

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

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

CabinfeverB added 2 commits July 26, 2023 21:31

merge master

0466ef4

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

merge master

2e09b41

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

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2023

CabinfeverB marked this pull request as ready for review July 26, 2023 14:25

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. do-not-merge/needs-linked-issue labels Jul 26, 2023

CabinfeverB changed the title ~~domain: global runaway watch by system table~~ domain: global runaway watch by system table and impl exector for query watch Jul 26, 2023

CabinfeverB changed the title ~~domain: global runaway watch by system table and impl exector for query watch~~ *: global runaway watch by system table and impl exector for query watch Jul 26, 2023

add test

b555f6b

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

Connor1996 mentioned this pull request Jul 26, 2023

Runaway query control based on resource group #43691

Closed

28 tasks

add a pre check

b5cdfe8

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

Connor1996 reviewed Jul 27, 2023

View reviewed changes

session/bootstrap.go Outdated Show resolved Hide resolved

Connor1996 reviewed Jul 27, 2023

View reviewed changes

session/bootstrap.go Show resolved Hide resolved

Connor1996 reviewed Jul 27, 2023

View reviewed changes

ddl/util/util.go Show resolved Hide resolved

Connor1996 reviewed Jul 27, 2023

View reviewed changes

domain/resourcegroup/runaway.go Outdated Show resolved Hide resolved

Connor1996 reviewed Jul 27, 2023

View reviewed changes

CabinfeverB added 2 commits July 31, 2023 13:15

address comment

0f9d330

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

address comment

0f700e6

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

Connor1996 approved these changes Jul 31, 2023

View reviewed changes

glorv reviewed Jul 31, 2023

View reviewed changes

address comment

4280700

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

glorv reviewed Jul 31, 2023

View reviewed changes

CabinfeverB added 3 commits July 31, 2023 20:52

address comment

8e7e7c3

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

address comment

a18f290

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

address comment and add test

e5a2050

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

qw4990 approved these changes Jul 31, 2023

View reviewed changes

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

CabinfeverB added 2 commits July 31, 2023 23:42

address comment

72f6e46

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

address comment

d62fee4

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

CabinfeverB commented Jul 31, 2023

View reviewed changes

glorv reviewed Aug 1, 2023

View reviewed changes

CabinfeverB added 3 commits August 1, 2023 10:10

address comment

ac02abc

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

address comment

7d243a5

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

fix data race

ea61855

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

glorv approved these changes Aug 1, 2023

View reviewed changes

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

ti-chi-bot bot merged commit c46da07 into pingcap:master Aug 1, 2023
7 of 12 checks passed

Defined2014 mentioned this pull request Aug 24, 2023

TestBootstrapWithError hang forever #46385

Closed

CabinfeverB mentioned this pull request Aug 24, 2023

test: remove useless code #46408

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: global runaway watch by system table and impl exector for `query watch` #45465

*: global runaway watch by system table and impl exector for `query watch` #45465

CabinfeverB commented Jul 19, 2023 •

edited

Loading

tiprow bot commented Jul 19, 2023

CabinfeverB commented Jul 19, 2023

Connor1996 Jul 27, 2023

CabinfeverB Jul 28, 2023

Connor1996 left a comment

ti-chi-bot bot commented Jul 31, 2023

glorv Jul 31, 2023

CabinfeverB Jul 31, 2023

glorv Jul 31, 2023

CabinfeverB Jul 31, 2023

glorv Jul 31, 2023

CabinfeverB Jul 31, 2023

glorv Jul 31, 2023

CabinfeverB Jul 31, 2023

glorv Jul 31, 2023

CabinfeverB Aug 1, 2023

glorv Aug 1, 2023

glorv Jul 31, 2023

glorv Jul 31, 2023

CabinfeverB Jul 31, 2023

glorv Jul 31, 2023

glorv Jul 31, 2023

CabinfeverB Jul 31, 2023

CabinfeverB Jul 31, 2023

CabinfeverB Jul 31, 2023

glorv Aug 1, 2023

CabinfeverB Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

CabinfeverB commented Aug 1, 2023

tiprow bot commented Aug 1, 2023

*: global runaway watch by system table and impl exector for query watch #45465

*: global runaway watch by system table and impl exector for query watch #45465

Conversation

CabinfeverB commented Jul 19, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Jul 19, 2023

CabinfeverB commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 31, 2023

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

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

ti-chi-bot bot commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

[LGTM Timeline notifier]

CabinfeverB commented Aug 1, 2023

tiprow bot commented Aug 1, 2023

*: global runaway watch by system table and impl exector for `query watch` #45465

*: global runaway watch by system table and impl exector for `query watch` #45465

CabinfeverB commented Jul 19, 2023 •

edited

Loading