ddl: only use runningJobs to check job dependency #54003

lance6716 · 2024-06-13T09:06:42Z

What problem does this PR solve?

Issue Number: ref #52795, #53246

Problem Summary:

What changed and how does it work?

assuming there's one DDL owner at any time, we don't need to query persistent data of running DDL jobs. Instead, we can check the memory cache of running DDL jobs.

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

Signed-off-by: lance6716 <lance6716@gmail.com>

tiprow · 2024-06-13T09:07:00Z

Hi @lance6716. 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-sigs/prow repository.

pkg/ddl/ddl_api.go

Signed-off-by: lance6716 <lance6716@gmail.com>

codecov · 2024-06-13T11:25:46Z

Codecov Report

Attention: Patch coverage is 86.66667% with 22 lines in your changes missing coverage. Please review.

Project coverage is 56.1069%. Comparing base (2cea994) to head (a9fcd16).
Report is 13 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54003         +/-   ##
=================================================
- Coverage   74.5294%   56.1069%   -18.4226%     
=================================================
  Files          1516       1638        +122     
  Lines        360821     610295     +249474     
=================================================
+ Hits         268918     342418      +73500     
- Misses        72393     244462     +172069     
- Partials      19510      23415       +3905

Flag	Coverage Δ
integration	`37.0623% <64.8484%> (?)`
unit	`71.7640% <86.6666%> (-1.6452%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (-2.2339%)`	⬇️
parser	`∅ <ø> (∅)`
br	`52.2563% <ø> (+8.2840%)`	⬆️

Signed-off-by: lance6716 <lance6716@gmail.com>

pkg/ddl/ddl_running_jobs.go

lance6716 · 2024-06-17T08:18:09Z

/retest

tiprow · 2024-06-17T08:18:31Z

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

In response to this:

/retest

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.

D3Hunter · 2024-06-17T12:58:00Z

pkg/ddl/ddl_api.go

placement policy and resource group are out of our check currently, we need to make sure 2 jobs of same name doesn't run concurrently

we also need to merge InvolvingSchemaInfo which cames from foreign key for multiple schema change

create table with foreign key also need it, but i have add it in #54041 for some tests depends on it

For placement policy and resource group, do you think we can add another field for InvolvingSchemaInfo to record the dependency or other implementation? also cc @tangenta

I'll consider FK in next commit.

now placement policy and resource group will set its name to job.Schema, if they have same namespace as database, seems ok to set InvolvingSchemaInfo = xx.*

placement policy and resource group related ddl all have a none, none InvolvingSchemaInfo😓

Adding another field maybe better. It is weird if a policy job blocks a create-database job..

I found it's a bit complex and I only want to add a TODO in this PR. Because there're exclusive jobs and shared jobs. Like CreatePlacementPolicy that directly modifies PlacementPolicy, and AlterTablePlacement that referring PlacementPolicy. Two AlterTablePlacement jobs should not have dependency.

And I think CREATE TABLE LIKE and FK are simliar. I want to leave this PR to this situation:

When two jobs only referring to an object, in other words, does not modify it, the jobs have no dependency

A referring job and a modifying job to the same object have no dependency. (I'll add TODO and improve it in next PR)

Two modifying jobs have dependency.

I'll commit soon

When two jobs only referring to an object, in other words, does not modify it, the jobs have no dependency

we should take them as depends on each other now as when calculate dependency we don't consider skipped jobs now, suppose this

job 1: create table a ...foreign key refer test.t

job 2: alter table test.t

job 3: create table b ...foreign key refer test.t

job 1 can run, but not job 2, when we check job 3, we need consider job 2, not run it directly.

A referring job and a modifying job to the same object have no dependency.

do you mean job 1 create table a foreign key test.t; job 2 alter table test.t have no dependency? i think it does if job 2 modify the referred column

job 1: create table a ...foreign key refer test.t
job 2: alter table test.t
job 3: create table b ...foreign key refer test.t
job 1 can run, but not job 2, when we check job 3, we need consider job 2, not run it directly.

I think this is the case of fair lock. I'll keep this property in next PR

do you mean job 1 create table a foreign key test.t; job 2 alter table test.t have no dependency? i think it does if job 2 modify the referred column

It should have dependency, but I also want to leave it for next PR. In next PR I'll add exclusive and shared fields to runningJobs and InvolvingSchemaInfo to implement it.

D3Hunter · 2024-06-17T13:06:47Z

pkg/ddl/ddl_api.go

@@ -7949,7 +7969,17 @@ func (d *ddl) CreateForeignKey(ctx sessionctx.Context, ti ast.Ident, fkName mode
 		BinlogInfo:     &model.HistoryInfo{},
 		Args:           []any{fkInfo, fkCheck},
 		CDCWriteSource: ctx.GetSessionVars().CDCWriteSource,
-		SQLMode:        ctx.GetSessionVars().SQLMode,
+		InvolvingSchemaInfo: []model.InvolvingSchemaInfo{


drop foreign key also need it

I see that in the execution code dropForeignKey, it does not check the existence of RefSchema / RefTable, maybe in this way it's more rubust.

@crazycs520 Do you think we should check dependency for the reference table of drop foreign key?

pkg/ddl/ddl_api.go

Signed-off-by: lance6716 <lance6716@gmail.com>

pkg/ddl/multi_schema_change.go

pkg/ddl/ddl_running_jobs.go

pkg/parser/model/ddl.go

pkg/ddl/ddl_running_jobs.go

lance6716 · 2024-06-18T08:47:35Z

/unhold

D3Hunter · 2024-06-18T09:02:08Z

pkg/ddl/ddl_api.go

@@ -2829,31 +2829,16 @@ func (d *ddl) createTableWithInfoJob(
 		args = append(args, ctx.GetSessionVars().ForeignKeyChecks)
 	}

-	involvingSchemaInfo := make([]model.InvolvingSchemaInfo, 0, len(tbInfo.ForeignKeys)+1)


why it's removed? and below FK for multi schema change

I want to allow CREATE TABLE t1 FOREIGN KEY t0.c0 and CREATE TABLE t2 FOREIGN KEY t0.c0 running concurrently in this PR. See below.

#54003 (comment)

Oh, CI is failed because of this...

TestTruncateOrDropTableWithForeignKeyReferred2

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2024-06-18T11:22:45Z

/retest

tiprow · 2024-06-18T11:23:07Z

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

In response to this:

/retest

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.

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2024-06-19T01:30:35Z

/retest

tiprow · 2024-06-19T01:31:11Z

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

In response to this:

/retest

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.

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2024-06-19T02:39:00Z

/retest

tiprow · 2024-06-19T02:39:21Z

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

In response to this:

/retest

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.

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot · 2024-06-19T06:08:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, tangenta

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:

~~pkg/ddl/OWNERS~~ [tangenta]
~~pkg/parser/OWNERS~~ [D3Hunter,tangenta]

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-06-19T06:08:55Z

[LGTM Timeline notifier]

Timeline:

2024-06-18 08:13:38.991601459 +0000 UTC m=+102545.477090292: ☑️ agreed by D3Hunter.
2024-06-19 06:08:54.93598283 +0000 UTC m=+181461.421471662: ☑️ agreed by tangenta.

lance6716 added 2 commits June 13, 2024 16:45

ddl: only use runningJobs to check job dependency

895368b

Signed-off-by: lance6716 <lance6716@gmail.com>

add more TODO

2ffda1a

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 commented Jun 13, 2024

View reviewed changes

pkg/ddl/ddl_api.go Outdated Show resolved Hide resolved

lance6716 added 2 commits June 13, 2024 17:23

remove a TODO

2c44673

Signed-off-by: lance6716 <lance6716@gmail.com>

fix lint

d7b42bc

Signed-off-by: lance6716 <lance6716@gmail.com>

resolve TODO

1cc16f4

Signed-off-by: lance6716 <lance6716@gmail.com>

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

lance6716 changed the title ~~[WIP]ddl: only use runningJobs to check job dependency~~ ddl: only use runningJobs to check job dependency Jun 17, 2024

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 17, 2024

tangenta reviewed Jun 17, 2024

View reviewed changes

pkg/ddl/ddl_running_jobs.go Show resolved Hide resolved

D3Hunter reviewed Jun 17, 2024

View reviewed changes

lance6716 added 3 commits June 18, 2024 10:52

Merge branch 'master' of github.com:pingcap/tidb into running-jobs

5a7dd54

msc should consider add FK

9b2b386

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'master' of github.com:pingcap/tidb into running-jobs

3a7fac5

Signed-off-by: lance6716 <lance6716@gmail.com>

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 Jun 18, 2024

lance6716 added 2 commits June 18, 2024 12:59

fix fmt

3bd5796

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

0d9c105

Signed-off-by: lance6716 <lance6716@gmail.com>

tangenta reviewed Jun 18, 2024

View reviewed changes

pkg/ddl/multi_schema_change.go Outdated Show resolved Hide resolved

tangenta reviewed Jun 18, 2024

View reviewed changes

pkg/ddl/ddl_running_jobs.go Show resolved Hide resolved

D3Hunter approved these changes Jun 18, 2024

View reviewed changes

pkg/parser/model/ddl.go Show resolved Hide resolved

pkg/ddl/ddl_running_jobs.go Show resolved Hide resolved

pkg/ddl/ddl_running_jobs.go Show resolved Hide resolved

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 18, 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 Jun 18, 2024

D3Hunter reviewed Jun 18, 2024

View reviewed changes

lance6716 added 2 commits June 18, 2024 18:10

disable a UT

06f83e4

Signed-off-by: lance6716 <lance6716@gmail.com>

disable another test

0e098e0

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

448019a

Signed-off-by: lance6716 <lance6716@gmail.com>

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 Jun 18, 2024

lance6716 added 4 commits June 18, 2024 21:20

fix some UT

ac7d9c1

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

0af32f5

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

eb60bd0

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

b438b1d

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

9a02a89

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 added 2 commits June 19, 2024 11:13

fix UT

ebe256c

Signed-off-by: lance6716 <lance6716@gmail.com>

fix UT

a9fcd16

Signed-off-by: lance6716 <lance6716@gmail.com>

tangenta approved these changes Jun 19, 2024

View reviewed changes

ti-chi-bot bot added lgtm approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 19, 2024

ti-chi-bot bot merged commit 51ccce2 into pingcap:master Jun 19, 2024
23 checks passed

This was referenced Jun 19, 2024

optimize execution of general DDL #53246

Closed

ddl: query since min job id to mitigate slowness due to un-compacted deleted kv #54122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: only use runningJobs to check job dependency #54003

ddl: only use runningJobs to check job dependency #54003

lance6716 commented Jun 13, 2024 •

edited

Loading

tiprow bot commented Jun 13, 2024

codecov bot commented Jun 13, 2024 •

edited

Loading

lance6716 commented Jun 17, 2024

tiprow bot commented Jun 17, 2024

D3Hunter Jun 17, 2024

D3Hunter Jun 17, 2024

D3Hunter Jun 17, 2024

lance6716 Jun 18, 2024

D3Hunter Jun 18, 2024

D3Hunter Jun 18, 2024

tangenta Jun 18, 2024

lance6716 Jun 18, 2024 •

edited

Loading

D3Hunter Jun 18, 2024 •

edited

Loading

lance6716 Jun 18, 2024

D3Hunter Jun 17, 2024

lance6716 Jun 18, 2024

lance6716 commented Jun 18, 2024

D3Hunter Jun 18, 2024

lance6716 Jun 18, 2024

lance6716 Jun 18, 2024

lance6716 commented Jun 18, 2024

tiprow bot commented Jun 18, 2024

lance6716 commented Jun 19, 2024

tiprow bot commented Jun 19, 2024

lance6716 commented Jun 19, 2024

tiprow bot commented Jun 19, 2024

ti-chi-bot bot commented Jun 19, 2024

ti-chi-bot bot commented Jun 19, 2024

ddl: only use runningJobs to check job dependency #54003

ddl: only use runningJobs to check job dependency #54003

Conversation

lance6716 commented Jun 13, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jun 13, 2024

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

lance6716 commented Jun 17, 2024

tiprow bot commented Jun 17, 2024

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

lance6716 Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

D3Hunter Jun 18, 2024 • edited Loading

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

lance6716 commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Jun 18, 2024

tiprow bot commented Jun 18, 2024

lance6716 commented Jun 19, 2024

tiprow bot commented Jun 19, 2024

lance6716 commented Jun 19, 2024

tiprow bot commented Jun 19, 2024

ti-chi-bot bot commented Jun 19, 2024

ti-chi-bot bot commented Jun 19, 2024

[LGTM Timeline notifier]

lance6716 commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading

lance6716 Jun 18, 2024 •

edited

Loading

D3Hunter Jun 18, 2024 •

edited

Loading