ddl: check delete range for some DDLs #33392

wjhuang2016 · 2022-03-24T08:18:00Z

Signed-off-by: wjhuang2016 huangwenjun1997@gmail.com

What problem does this PR solve?

Issue Number: ref #33078

Problem Summary:
Try to add some checks for DDL

What is changed and how it works?

Check delete range for some DDLs.

Check List

A bug was found by this PR: #33620

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

None

ti-chi-bot · 2022-03-24T08:18:02Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hawkingrei
zimulala

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

wjhuang2016 · 2022-03-25T10:24:57Z

/run-unit-test

wjhuang2016 · 2022-03-28T06:55:24Z

/run-unit-test

wjhuang2016 · 2022-03-28T07:15:42Z

/run-unit-test

wjhuang2016 · 2022-03-28T10:56:47Z

/run-unit-test

wjhuang2016 · 2022-03-28T11:26:28Z

/run-unit-test

sre-bot · 2022-03-28T11:36:18Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/a00031325dc93b4f7bffd25792aebf839048aeeb

wjhuang2016 · 2022-03-28T12:07:40Z

/run-unit-test

wjhuang2016 · 2022-03-28T12:22:16Z

/run-unit-test

wjhuang2016 · 2022-03-30T07:31:11Z

/run-unit-test

wjhuang2016 · 2022-03-30T08:10:06Z

/run-unit-test

wjhuang2016 · 2022-03-30T12:58:11Z

/run-unit-test

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

wjhuang2016 · 2022-03-31T10:29:34Z

ddl/ddl_tiflash_test.go

@@ -566,6 +566,9 @@ func TestSetPlacementRuleWithGCWorker(t *testing.T) {
 	// Make SetPdLoop take effects.
 	time.Sleep(time.Second)

+	fCancel := TempDisableEmulatorGC()


We need to add this to fix this test, the adding check makes it unstable.

hawkingrei · 2022-03-31T11:54:39Z

ddl/sanity_check.go

+	"github.com/pingcap/tidb/util/sqlexec"
+)
+
+func checkRangeCntByTableIDs(physicalTableIDs []int64, cnt int64) {


if it is released in the production environment. panic info will be too simple for DBA to judge the error.

BTW, if it is just in our test environment, we can make a build tag to skip this check.

Could we put this file code to ddl_test.go？Or change the file name to test specifically. Because it's only used for testing.

// checkHistoryJobInTest does some sanity check to make sure something is correct after DDL complete.
// It's only check during the test environment, so it would panic directly.
// These checks may be controlled by configuration in the future.

It' could be enabled in our integration test environment, even in the production environment.
For now, it is just enabled in unit test environment, so I let it panic directly.

When will the production environment be used?

zimulala · 2022-04-01T03:02:57Z

ddl/sanity_check.go

+				break
+			}
+			fallthrough
+		case model.ActionDropSchema, model.ActionDropTable, model.ActionTruncateTable, model.ActionDropIndex, model.ActionDropPrimaryKey,


Do we need to add a function to identify tasks of this type？Because there are a few places to use, in case of missing.

I don't have a good idea, any suggestion?

Do we add a function like IsRollbackDone?

zimulala · 2022-04-01T03:05:26Z

ddl/sanity_check.go

+	select count(1) cnt from mysql.gc_delete_range_done where job_id = %?) as gdr;`
+	rs, err := s.ExecuteInternal(context.TODO(), query, job.ID, job.ID)
+	if err != nil {
+		if strings.Contains(err.Error(), "Not Supported") {


When we will go to this logic？

Mock session

ddl/sanity_check.go

zimulala · 2022-04-01T03:12:09Z

ddl/sanity_check.go

+	"github.com/pingcap/tidb/util/sqlexec"
+)
+
+func checkRangeCntByTableIDs(physicalTableIDs []int64, cnt int64) {


Could we put this file code to ddl_test.go？Or change the file name to test specifically. Because it's only used for testing.

ddl/sanity_check.go

zimulala · 2022-04-01T03:30:02Z

Excuse me, I saw in this PR description that you chose Unit Test, but there is no test code in this PR.

wjhuang2016 · 2022-04-01T05:58:42Z

Excuse me, I saw in this PR description that you chose Unit Test, but there is no test code in this PR.

TestSetPlacementRuleWithGCWorker is modified.

Co-authored-by: Lynn <zimu_xia@126.com>

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

zimulala

LGTM

hawkingrei · 2022-04-01T09:15:14Z

/run-check_dev_2

wjhuang2016 · 2022-04-01T09:24:21Z

/merge

ti-chi-bot · 2022-04-01T09:24:24Z

This pull request has been accepted and is ready to merge.

Commit hash: d8251d7

ti-chi-bot · 2022-04-01T09:41:23Z

@wjhuang2016: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

hawkingrei · 2022-04-01T09:50:39Z

ddl/ddl_worker.go

-		metrics.DDLWorkerHistogram.WithLabelValues(metrics.WorkerFinishDDLJob, job.Type.String(), metrics.RetLabel(err)).Observe(time.Since(startTime).Seconds())
-	}()
-
+func jobNeedGC(job *model.Job) bool {


It may be migrated into parser/model. it is easy for others to reuse.

It should be moved when other packages need it.

…35870) ref #33078, ref #33392

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 24, 2022

ti-chi-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 Mar 29, 2022

ti-chi-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 Mar 30, 2022

wjhuang2016 force-pushed the check_del_range branch 2 times, most recently from a257a55 to b865315 Compare March 30, 2022 09:26

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

done

bcb4e89

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

wjhuang2016 force-pushed the check_del_range branch from 5bb3a1f to bcb4e89 Compare March 31, 2022 06:58

Merge branch 'master' into check_del_range

36ad842

wjhuang2016 commented Mar 31, 2022

View reviewed changes

hawkingrei reviewed Mar 31, 2022

View reviewed changes

zimulala reviewed Apr 1, 2022

View reviewed changes

wjhuang2016 and others added 4 commits April 1, 2022 16:34

Update ddl/sanity_check.go

32eaae1

Co-authored-by: Lynn <zimu_xia@126.com>

Update ddl/sanity_check.go

567d161

Co-authored-by: Lynn <zimu_xia@126.com>

Update ddl/sanity_check.go

21befcb

Co-authored-by: Lynn <zimu_xia@126.com>

add jobNeedGC to make code simple

d8251d7

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>

zimulala approved these changes Apr 1, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 1, 2022

hawkingrei approved these changes Apr 1, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 1, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 1, 2022

ti-chi-bot added 2 commits April 1, 2022 17:24

Merge branch 'master' into check_del_range

7985315

Merge branch 'master' into check_del_range

a000313

hawkingrei reviewed Apr 1, 2022

View reviewed changes

ti-chi-bot merged commit d0bf65c into pingcap:master Apr 1, 2022

tangenta mentioned this pull request Jun 30, 2022

sanity_check: adapt checkDeleteRangeCnt() for multi-schema change #35870

Merged

12 tasks

ti-chi-bot pushed a commit that referenced this pull request Jul 1, 2022

sanity_check: adapt checkDeleteRangeCnt() for multi-schema change (#…

ea5b0ff

…35870) ref #33078, ref #33392

wjhuang2016 deleted the check_del_range branch November 17, 2022 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: check delete range for some DDLs #33392

ddl: check delete range for some DDLs #33392

wjhuang2016 commented Mar 24, 2022 •

edited

Loading

ti-chi-bot commented Mar 24, 2022 •

edited

Loading

wjhuang2016 commented Mar 25, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

sre-bot commented Mar 28, 2022 •

edited

Loading

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 30, 2022

wjhuang2016 commented Mar 30, 2022

wjhuang2016 commented Mar 30, 2022

wjhuang2016 Mar 31, 2022

hawkingrei Mar 31, 2022

zimulala Apr 1, 2022

wjhuang2016 Apr 1, 2022

zimulala Apr 1, 2022

zimulala Apr 1, 2022

wjhuang2016 Apr 1, 2022

zimulala Apr 1, 2022

zimulala Apr 1, 2022

wjhuang2016 Apr 1, 2022

zimulala Apr 1, 2022

zimulala commented Apr 1, 2022

wjhuang2016 commented Apr 1, 2022

zimulala left a comment

hawkingrei commented Apr 1, 2022

wjhuang2016 commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

hawkingrei Apr 1, 2022

wjhuang2016 Apr 1, 2022

ddl: check delete range for some DDLs #33392

ddl: check delete range for some DDLs #33392

Conversation

wjhuang2016 commented Mar 24, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 24, 2022 • edited Loading

wjhuang2016 commented Mar 25, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

sre-bot commented Mar 28, 2022 • edited Loading

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 28, 2022

wjhuang2016 commented Mar 30, 2022

wjhuang2016 commented Mar 30, 2022

wjhuang2016 commented Mar 30, 2022

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

zimulala commented Apr 1, 2022

wjhuang2016 commented Apr 1, 2022

zimulala left a comment

Choose a reason for hiding this comment

hawkingrei commented Apr 1, 2022

wjhuang2016 commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjhuang2016 commented Mar 24, 2022 •

edited

Loading

ti-chi-bot commented Mar 24, 2022 •

edited

Loading

sre-bot commented Mar 28, 2022 •

edited

Loading