Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ddl : fix concurrent add partition problem #8783

Merged
merged 5 commits into from
Dec 29, 2018

Conversation

ciscoxll
Copy link
Contributor

@ciscoxll ciscoxll commented Dec 24, 2018

What problem does this PR solve?

When adding partitions in concurrent, there is a problem if the values less than are not checked after the ddl job is enqueued.

What is changed and how it works?

  • We consider the concurrent scene, so we need to check after the ddl job is enqueued.
    checkAddPartitionValue This function should also be evaluated once in onAddTablePartition.

Check List

Tests

  • Unit test

This change is Reviewable

@ciscoxll
Copy link
Contributor Author

@zimulala @crazycs520 PTAL

@shenli
Copy link
Member

shenli commented Dec 24, 2018

s/parallel/concurrent/

@ciscoxll ciscoxll changed the title ddl : fix parallel add partition problem ddl : fix concurrent add partition problem Dec 24, 2018
@@ -673,6 +684,14 @@ func (s *testStateChangeSuite) testControlParallelExecSQL(c *C, sql1, sql2 strin
c.Assert(err, IsNil)
defer s.se.Execute(context.Background(), "drop table t")

_, err = s.se.Execute(context.Background(), "drop database if exists t_part")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add an argument to distinguish between using a partitioned table or a general table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimulala master branch has opened the range partition table by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimulala PTAL.

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the comment. Mostly LGTM.

@ciscoxll
Copy link
Contributor Author

@tiancaiamao PTAL

ddl/table.go Show resolved Hide resolved
ddl/db_change_test.go Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 29, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@winkyao
Copy link
Contributor

winkyao commented Dec 29, 2018

Please cherry-pick this PR to 2.1.

@ciscoxll ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 29, 2018
@ciscoxll
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ciscoxll ciscoxll added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Dec 29, 2018
@ciscoxll ciscoxll merged commit 680368f into pingcap:master Dec 29, 2018
@ciscoxll ciscoxll deleted the parallel-add-partition branch December 29, 2018 12:14
yu34po pushed a commit to yu34po/tidb that referenced this pull request Jan 2, 2019
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants