ddl: delay before changing column from null to not null #23364

sticnarf · 2021-03-17T03:58:01Z

What problem does this PR solve?

Issue Number: close #23363

What is changed and how it works?

Before the double check of changing null to not null, we should delay like the reorg to make async commit safe.

After the delay, if an async commit writes a NULL, the double null check must detect it.

Check List

Tests

Integration test

Release note

No release note.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

cfzjywxk · 2021-03-17T08:44:04Z

/lgtm

wjhuang2016 · 2021-03-17T09:22:40Z

ddl/ddl.go

+// delayForAsyncCommit sleeps `SafeWindow + AllowedClockDrift` before a DDL job finishes.
+// It should be called before any DDL that could break data consistency.
+// This provides a safe window for async commit and 1PC to commit with an old schema.
+func delayForAsyncCommit() {


Do we need to check 1PC here?

1PC works in the same way with async commit.

"sleep before DDL finishes to make async commit safe" is not accurate?

OK. I just change the log without changing the function name. It looks too verbose to also put 1PC to the name

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

into delay-before-not-null

…null Signed-off-by: Yilin Chen <sticnarf@gmail.com>

cfzjywxk · 2021-03-18T06:46:56Z

@AilinKid @wjhuang2016 PTAL

wjhuang2016 · 2021-03-18T07:44:14Z

/lgtm

ti-chi-bot · 2021-03-18T07:44:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

cfzjywxk
wjhuang2016

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

sticnarf · 2021-03-18T09:16:31Z

/merge

ti-chi-bot · 2021-03-18T09:16:35Z

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

Commit hash: 0b0492d

AilinKid

Rest LGTM

AilinKid · 2021-03-18T09:19:32Z

go.sum

 cloud.google.com/go v0.50.0/go.mod h1:r9sluTvynVuxRIOHXQEHMFffphuXHOMZMycpNR5e6To=
 cloud.google.com/go v0.52.0/go.mod h1:pXajvRH/6o3+F9jDHZWQ5PbGhn+o8w9qiu/CffaVdO4=
 cloud.google.com/go v0.53.0 h1:MZQCQQaRwOrAcuKjiHWHrgKykt4fZyuwF2dtiG3fGW8=
 cloud.google.com/go v0.53.0/go.mod h1:fp/UouUEsRkN6ryDKNW/Upv/JBKnv6WDthjR6+vze6M=
 cloud.google.com/go/bigquery v1.0.1/go.mod h1:i/xbL2UlR5RvWAURpBYZTtm/cXjCha9lbfbpx4poX+o=
-cloud.google.com/go/bigquery v1.3.0 h1:sAbMqjY1PEQKZBWfbu6Y6bsupJ9c4QdHnzg/VvYTLcE=
 cloud.google.com/go/bigquery v1.3.0/go.mod h1:PjpwJnslEMmckchkHFfq+HTD2DmtT67aNFKH1/VBDHE=


go.sum should be changed?

…null

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

AilinKid · 2021-03-18T12:07:29Z

/lgtm

cfzjywxk · 2021-03-18T12:20:10Z

/merge

ti-chi-bot · 2021-03-18T12:20:14Z

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

Commit hash: 855dade

sticnarf · 2021-03-19T02:17:10Z

https://internal.pingcap.net/idc-jenkins/blue/rest/organizations/jenkins/pipelines/tidb_ghpr_unit_test/runs/68096/nodes/82/steps/345/log/?start=0

Unit test fails due to a reported race: #23280

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-03-19T02:19:31Z

cherry pick to release-5.0 in PR #23424

* ddl: delay before changing column from null to not null Signed-off-by: Yilin Chen <sticnarf@gmail.com> Co-authored-by: Ti Chi Robot <71242396+ti-chi-bot@users.noreply.github.com> Co-authored-by: Arenatlx <314806019@qq.com>

)

sticnarf added type/bugfix This PR fixes a bug. sig/transaction SIG:Transaction needs-cherry-pick-release-5.0 labels Mar 17, 2021

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 17, 2021

github-actions bot added the sig/sql-infra SIG: SQL Infra label Mar 17, 2021

ichn-hu mentioned this pull request Mar 17, 2021

Welcome to contribute #20804

Closed

sticnarf marked this pull request as draft March 17, 2021 07:24

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

sticnarf force-pushed the delay-before-not-null branch from 75dd84e to 92112f7 Compare March 17, 2021 07:39

ddl: delay before changing column from null to not null

105a8a2

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

sticnarf force-pushed the delay-before-not-null branch from 92112f7 to 105a8a2 Compare March 17, 2021 07:41

sticnarf marked this pull request as ready for review March 17, 2021 07:41

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

sticnarf requested review from AilinKid and cfzjywxk March 17, 2021 07:43

Merge branch 'master' into delay-before-not-null

597064f

cfzjywxk requested a review from wjhuang2016 March 17, 2021 08:39

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 17, 2021

wjhuang2016 reviewed Mar 17, 2021

View reviewed changes

sticnarf added 3 commits March 17, 2021 19:20

address comment: make log more accurate

383e6ea

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Merge branch 'delay-before-not-null' of https://github.com/sticnarf/tidb

9bd964a

into delay-before-not-null

Merge remote-tracking branch 'upstream/master' into delay-before-not-…

0b0492d

…null Signed-off-by: Yilin Chen <sticnarf@gmail.com>

cfzjywxk requested a review from wjhuang2016 March 18, 2021 03:10

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 Mar 18, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 18, 2021

Merge branch 'master' into delay-before-not-null

ab9f8cb

AilinKid reviewed Mar 18, 2021

View reviewed changes

sticnarf added 2 commits March 18, 2021 17:45

Merge remote-tracking branch 'upstream/master' into delay-before-not-…

57659f2

…null

do not modify go.sum

e507de6

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 18, 2021

ti-chi-bot and others added 2 commits March 18, 2021 17:48

Merge branch 'master' into delay-before-not-null

eb0d88a

Merge branch 'master' into delay-before-not-null

855dade

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 18, 2021

ti-chi-bot added 3 commits March 18, 2021 20:34

Merge branch 'master' into delay-before-not-null

4be762b

Merge branch 'master' into delay-before-not-null

2a81dab

Merge branch 'master' into delay-before-not-null

57e13ea

sticnarf merged commit e79ac3d into pingcap:master Mar 19, 2021

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Mar 19, 2021

cherry pick pingcap#23364 to release-5.0

491cd00

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Mar 19, 2021

ddl: delay before changing column from null to not null (#23364) #23424

Merged

ti-chi-bot pushed a commit that referenced this pull request Mar 29, 2021

ddl: delay before changing column from null to not null (#23364) (#23424

dfa361f

)

LittleFall mentioned this pull request Apr 12, 2021

Index creation in TiDB 5.0 is much slower than 4.0. pingcap/tiflash#1730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: delay before changing column from null to not null #23364

ddl: delay before changing column from null to not null #23364

sticnarf commented Mar 17, 2021 •

edited

Loading

cfzjywxk commented Mar 17, 2021

wjhuang2016 Mar 17, 2021

sticnarf Mar 17, 2021

wjhuang2016 Mar 17, 2021

sticnarf Mar 17, 2021

cfzjywxk commented Mar 18, 2021

wjhuang2016 commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

sticnarf commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

AilinKid left a comment

AilinKid Mar 18, 2021

AilinKid commented Mar 18, 2021

cfzjywxk commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

sticnarf commented Mar 19, 2021

ti-srebot commented Mar 19, 2021

ddl: delay before changing column from null to not null #23364

ddl: delay before changing column from null to not null #23364

Conversation

sticnarf commented Mar 17, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

cfzjywxk commented Mar 17, 2021

wjhuang2016 Mar 17, 2021

Choose a reason for hiding this comment

sticnarf Mar 17, 2021

Choose a reason for hiding this comment

wjhuang2016 Mar 17, 2021

Choose a reason for hiding this comment

sticnarf Mar 17, 2021

Choose a reason for hiding this comment

cfzjywxk commented Mar 18, 2021

wjhuang2016 commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

sticnarf commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

AilinKid left a comment

Choose a reason for hiding this comment

AilinKid Mar 18, 2021

Choose a reason for hiding this comment

AilinKid commented Mar 18, 2021

cfzjywxk commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

sticnarf commented Mar 19, 2021

ti-srebot commented Mar 19, 2021

sticnarf commented Mar 17, 2021 •

edited

Loading