Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

br: remove alter-primary-key configuration#853

Merged
ti-srebot merged 10 commits into
pingcap:masterfrom
tangenta:rm-alter-pk-config
Mar 18, 2021
Merged

br: remove alter-primary-key configuration#853
ti-srebot merged 10 commits into
pingcap:masterfrom
tangenta:rm-alter-pk-config

Conversation

@tangenta
Copy link
Copy Markdown
Contributor

What problem does this PR solve?

pingcap/tidb#23270 will deprecate the alter-primary-key config.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test

Code changes

NA

Side effects

  • Breaking backward compatibility?

Related changes

NA

Release Note

  • No release note

@kennytm
Copy link
Copy Markdown
Collaborator

kennytm commented Mar 11, 2021

you need to change the tests/br_incompatible_config test case as well. it will start a TiDB using the alter-primary-key config.

also the sanity tests in tests/br_db and tests/br_full can probably be removed.

@tangenta
Copy link
Copy Markdown
Contributor Author

tangenta commented Mar 12, 2021

I notice that the option allow-auto-random is deprecated too and it is not removed. Should I remove both of them in tests/br_incompatible_config?

@kennytm
Copy link
Copy Markdown
Collaborator

kennytm commented Mar 12, 2021

@tangenta yes that too.

since now alter-pk is opt-in, some DDL statements in tests/br_incompatible_config should be changed to use that /*T![clustered_index] NONCLUSTERED */ syntax.

@tangenta tangenta requested a review from kennytm March 12, 2021 07:17
@lichunzhu
Copy link
Copy Markdown
Contributor

/lgtm

@ti-srebot ti-srebot added the status/LGT1 LGTM1 label Mar 12, 2021
@tangenta
Copy link
Copy Markdown
Contributor Author

mysql> create table t (a int primary key nonclustered, b int unique);
Query OK, 0 rows affected (0.02 sec)

mysql> insert into t values (42, 42);
Query OK, 1 row affected (0.01 sec)

mysql> alter table t drop primary key;
ERROR 8200 (HY000): Unsupported drop primary key when alter-primary-key is false

This is because pingcap/tidb#23270 is not merged. We should merge one first since these two PRs depend on each other.

@kennytm
Copy link
Copy Markdown
Collaborator

kennytm commented Mar 12, 2021

@tangenta why 23270 depends on this PR?

@tangenta
Copy link
Copy Markdown
Contributor Author

Because TiDB depends on BR and 23270 removes the AlterPrimaryKey field from Config. The build will fail.

OK, let me bring back AlterPrimaryKey field and merge 23270 first.

@lysu
Copy link
Copy Markdown
Collaborator

lysu commented Mar 18, 2021

@tangenta please help resolve conflict

@lysu
Copy link
Copy Markdown
Collaborator

lysu commented Mar 18, 2021

/cc @kennytm @overvenus PTAL if free thanks

@kennytm
Copy link
Copy Markdown
Collaborator

kennytm commented Mar 18, 2021

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 LGTM1 label Mar 18, 2021
@ti-srebot ti-srebot added the status/LGT2 LGTM2 label Mar 18, 2021
@kennytm
Copy link
Copy Markdown
Collaborator

kennytm commented Mar 18, 2021

/merge

@ti-srebot
Copy link
Copy Markdown
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 0b223bc into pingcap:master Mar 18, 2021
ti-srebot pushed a commit to ti-srebot/br that referenced this pull request Mar 18, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Copy Markdown
Contributor

cherry pick to release-5.0 in PR #897

overvenus pushed a commit that referenced this pull request Mar 19, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Co-authored-by: tangenta <tangenta@126.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants