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

bindinfo, record how bindings are created in SQL bindings. #17254

Merged
merged 15 commits into from
May 27, 2020

Conversation

danmay319
Copy link
Contributor

@danmay319 danmay319 commented May 18, 2020

What problem does this PR solve?

Issue Number: close #16748

Problem Summary:

What is changed and how it works?

What's Changed:

  • add domain CreateWay in Binding to indicate the way binding created.
  • show CreateWay when execute show [global] bindings

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more MEM

Release note

  • add domain CreateWay in Binding to help know the way binding created.

@danmay319 danmay319 requested review from a team as code owners May 18, 2020 05:06
@ghost ghost requested review from SunRunAway and removed request for a team May 18, 2020 05:06
@github-actions github-actions bot added sig/execution SIG execution sig/planner SIG: Planner labels May 18, 2020
@danmay319
Copy link
Contributor Author

PTAL~ @eurekaka @lzmhhh123

@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@e92f15d). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #17254   +/-   ##
===========================================
  Coverage          ?   80.2397%           
===========================================
  Files             ?        521           
  Lines             ?     142579           
  Branches          ?          0           
===========================================
  Hits              ?     114405           
  Misses            ?      19164           
  Partials          ?       9010           

@@ -240,6 +240,7 @@ const (
update_time timestamp(3) NOT NULL,
charset text NOT NULL,
collation text NOT NULL,
create_way text NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd better use alter table mysql.bind_info add column ... and add a new version for bootstrap to avoid upgrade compatibility problems.

Copy link
Contributor

@lzmhhh123 lzmhhh123 May 19, 2020

Choose a reason for hiding this comment

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

BTW, a default value unknown for create_way is also necessary. Because the lower version of tidb-server doesn't have this column.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done now, PTAL @lzmhhh123

@danmay319 danmay319 requested a review from lzmhhh123 May 19, 2020 08:51
@danmay319
Copy link
Contributor Author

/run-unit-tests

@@ -240,6 +240,7 @@ const (
update_time timestamp(3) NOT NULL,
charset text NOT NULL,
collation text NOT NULL,
create_way varchar(20) NOT NULL default 'unknow',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
create_way varchar(20) NOT NULL default 'unknow',
create_way varchar(10) NOT NULL default 'unknown',

After we execute the alter column in bootstrap, it's better not to create the column when create 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.

@lzmhhh123 upgradeToVer47 will only be done when the version code is lower than 47, while current version is 47, so create the column when create table is necessary.
I checked the implementation of other upgradeToVerX functions, they did it in the same way.

Copy link
Contributor Author

@danmay319 danmay319 May 26, 2020

Choose a reason for hiding this comment

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

for instance, upgradeToVer29 adds an index, and a relevant index is created when create table, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lzmhhh123 and , sql_created is more than 10 chars, so we need varchar(20). 😆 (I didn't realized it first, either.)

@danmay319 danmay319 requested a review from lzmhhh123 May 26, 2020 06:01
@lzmhhh123
Copy link
Contributor

/rebuild

@danmay319
Copy link
Contributor Author

PTAL @eurekaka

bindinfo/cache.go Outdated Show resolved Hide resolved
session/bootstrap.go Outdated Show resolved Hide resolved
@danmay319
Copy link
Contributor Author

comment addressed @eurekaka

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

bindinfo/cache.go Outdated Show resolved Hide resolved
@danmay319 danmay319 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 26, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 26, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 26, 2020

@lawyerphx merge failed.

@danmay319
Copy link
Contributor Author

/run-integration-copr-tests

@danmay319
Copy link
Contributor Author

/run-integration-br-tests

@lzmhhh123
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 27, 2020

/run-all-tests

@sre-bot sre-bot merged commit 7ca3d9c into pingcap:master May 27, 2020
@danmay319 danmay319 deleted the issue16748 branch May 27, 2020 01:58
@danmay319
Copy link
Contributor Author

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 2, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Jun 2, 2020

cherry pick to release-4.0 in PR #17587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL binding should record how bindings are created.
4 participants