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

infoschema: add table_id to table and add tidb_indexes table. #9183

Merged
merged 7 commits into from
Jan 31, 2019
Merged

infoschema: add table_id to table and add tidb_indexes table. #9183

merged 7 commits into from
Jan 31, 2019

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Jan 25, 2019

What problem does this PR solve?

now, we can query table_id and index_id from http-api, but in some enviorment, it'
s more convient to use sql.

final usage can be sawed in test case https://github.com/pingcap/tidb/pull/9183/files#diff-9cb2947f24dfdff2e6f6cec84b53aa56R212

What is changed and how it works?

  • add tidb_table_id column to information_schema.tables
  • add new tidb_indexes table that contains index_id column

let use query table_id and index_id via sql.

Check List

Tests

  • Unit test

Code changes

  • infomation schema: column and table

Side effects

  • N/A

Related changes

  • N/A

This change is Reviewable

@lysu lysu added type/enhancement The issue or PR belongs to an enhancement. component/server labels Jan 25, 2019
@lysu
Copy link
Contributor Author

lysu commented Jan 25, 2019

/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

@codecov-io
Copy link

codecov-io commented Jan 27, 2019

Codecov Report

Merging #9183 into master will increase coverage by 0.03%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9183      +/-   ##
==========================================
+ Coverage   67.21%   67.24%   +0.03%     
==========================================
  Files         371      371              
  Lines       77052    77104      +52     
==========================================
+ Hits        51789    51850      +61     
+ Misses      20634    20629       -5     
+ Partials     4629     4625       -4
Impacted Files Coverage Δ
infoschema/tables.go 51.68% <88.46%> (+2.45%) ⬆️
infoschema/infoschema.go 76.31% <0%> (-2.64%) ⬇️
executor/index_lookup_join.go 77.28% <0%> (-0.64%) ⬇️
expression/schema.go 94.95% <0%> (+0.84%) ⬆️
store/tikv/lock_resolver.go 42.65% <0%> (+0.94%) ⬆️
executor/join.go 78.9% <0%> (+1.04%) ⬆️
store/tikv/scan.go 77.31% <0%> (+3.36%) ⬆️
ddl/delete_range.go 80.42% <0%> (+5.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3bdc5d...65c5d55. Read the comment docs.

@jackysp
Copy link
Member

jackysp commented Jan 28, 2019

/run-all-tests

infoschema/tables.go Outdated Show resolved Hide resolved
tb.Name.O, // TABLE_NAME
0, // NON_UNIQUE
"PRIMARY", // KEY_NAME
1, // SEQ_IN_INDEX
Copy link
Contributor

@alivxxx alivxxx Jan 28, 2019

Choose a reason for hiding this comment

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

Is 0 better? Or it may coincide with indices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's follow

1, // Seq_in_index
, so there are bug in show index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should better let seq start from 1 https://dev.mysql.com/doc/refman/8.0/en/show-index.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so it is better to make the seq_in_index be len(rows)+1 for index.

{"KEY_NAME", mysql.TypeVarchar, 64, 0, nil, nil},
{"SEQ_IN_INDEX", mysql.TypeLonglong, 21, 0, nil, nil},
{"COLUMN_NAME", mysql.TypeVarchar, 64, 0, nil, nil},
{"SUB_PART", mysql.TypeLonglong, 21, 0, nil, nil},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not name it as LENGTH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because show index use this name

record := types.MakeDatums(
schema.Name.O, // TABLE_SCHEMA
tb.Name.O, // TABLE_NAME
0, // NON_UNIQUE
Copy link
Contributor

Choose a reason for hiding this comment

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

May this 0 confuse others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just follow show index

0, // Non_unique

{"TABLE_SCHEMA", mysql.TypeVarchar, 64, 0, nil, nil},
{"TABLE_NAME", mysql.TypeVarchar, 64, 0, nil, nil},
{"NON_UNIQUE", mysql.TypeLonglong, 21, 0, nil, nil},
{"KEY_NAME", mysql.TypeVarchar, 64, 0, nil, nil},
Copy link
Contributor

Choose a reason for hiding this comment

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

put this column before NON_UNIQUE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's following show index

infoschema/tables.go Outdated Show resolved Hide resolved
infoschema/tables_test.go Outdated Show resolved Hide resolved
infoschema/tables_test.go Outdated Show resolved Hide resolved
@@ -1003,6 +1019,68 @@ func dataForTables(ctx sessionctx.Context, schemas []*model.DBInfo) ([][]types.D
return rows, nil
}

func dataForIndexes(ctx sessionctx.Context, schemas []*model.DBInfo) ([][]types.Datum, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like duplicate code with fetchShowIndex? can we extract it to a separate function for reuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, current show table and tables is same to but maybe we can do this in future?

infoschema/tables.go Show resolved Hide resolved
infoschema/tables.go Show resolved Hide resolved
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

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants