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,domain: introduce InfoSchemaV2, core data struct etc #51058

Merged
merged 18 commits into from
Feb 23, 2024

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Issue Number: ref #50959

Problem Summary:

What changed and how does it work?

This commit introduce the infoschema_v2.go file, include the core data struct, implement some of the APIs.

It's not a completed work, the code can barely run some basic operations.
There is still a long way to make the code smooth, but I fire this PR early and it is ready for review now. Why?
Because I hope we can provide infoschema v2 in tidb 8.0, GA or at least RC, time is limited.
This commit provide a solid foundation that we can work on together.

With the core data struct introduced, we can fullfill the missing things and split the tasks parallelly.
Later, I, @GMHDBJD and other team members will continue the development.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Enable infoschema v2 here:

https://github.com/pingcap/tidb/compare/master...tiancaiamao:infoschema-v2-pr?expand=1#diff-d82e5bb6499a8d8c00f3d55ec95bccda3fab6d4cc45529cecf06552bfa973b1bR21

func init() {
    enableV2.Store(true)
}

Start tidb, create some tabe, insert data, drop table, verify the basic operations etc...

This commit doesn't break anyting because the new code is not used now.
We need prepare a test plan document and add tests later.

  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 7, 2024
@tiancaiamao tiancaiamao requested a review from GMHDBJD February 7, 2024 11:49
Copy link

tiprow bot commented Feb 7, 2024

Hi @tiancaiamao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tiancaiamao
Copy link
Contributor Author

This is extracted from #50591
The differ is that this pull request clean up the code and keep no breaking things, infoschema v1 is used.
In that branch, infoschema v2 is used and tested, and v1 does not load the data so we can see how much memory are used:

tidb/pkg/infoschema/builder.go

Lines 1140 to 1144 in 350b887

if di.Name.L == "information_schema" || di.Name.L == "performance_schema" {
schTbls.tables[t.Name.L] = tbl
sortedTbls := b.is.sortedTablesBuckets[tableBucketIdx(t.ID)]
b.is.sortedTablesBuckets[tableBucketIdx(t.ID)] = append(sortedTbls, tbl)
}

@tiancaiamao
Copy link
Contributor Author

/retest

Copy link

tiprow bot commented Feb 7, 2024

@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@hawkingrei
Copy link
Member

/ok-to-test

@ti-chi-bot ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Feb 18, 2024
@hawkingrei
Copy link
Member

/retest

@tiancaiamao
Copy link
Contributor Author

/retest-required

Copy link

codecov bot commented Feb 18, 2024

Codecov Report

Merging #51058 (64e7969) into master (65d57c0) will decrease coverage by 17.0222%.
Report is 46 commits behind head on master.
The diff coverage is 9.9688%.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #51058         +/-   ##
=================================================
- Coverage   72.2960%   55.2738%   -17.0222%     
=================================================
  Files          1467       1576        +109     
  Lines        361760     609583     +247823     
=================================================
+ Hits         261538     336940      +75402     
- Misses        80904     249544     +168640     
- Partials      19318      23099       +3781     
Flag Coverage Δ
integration 36.6175% <9.3457%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 53.9957% <ø> (-2.3172%) ⬇️
parser ∅ <ø> (∅)
br 49.9479% <ø> (+3.6136%) ⬆️

@tiancaiamao tiancaiamao requested a review from zimulala February 19, 2024 08:33
Copy link
Contributor

@GMHDBJD GMHDBJD left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Feb 20, 2024
@tiancaiamao tiancaiamao requested a review from tangenta February 21, 2024 07:01
Copy link
Contributor

@D3Hunter D3Hunter 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

pkg/infoschema/infoschema_v2.go Outdated Show resolved Hide resolved
pkg/infoschema/infoschema_v2.go Outdated Show resolved Hide resolved
pkg/infoschema/infoschema_v2.go Outdated Show resolved Hide resolved
pkg/infoschema/infoschema_v2.go Outdated Show resolved Hide resolved
pkg/infoschema/infoschema_v2.go Outdated Show resolved Hide resolved
func (is *infoschemaV2) SchemaExists(schema model.CIStr) bool {
var ok bool
// TODO: support different version
is.schemaMap.Scan(func(item schemaItem) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

use Descend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a full memory Scan, it does not matter whether we Descend or Ascend

Copy link
Contributor

@D3Hunter D3Hunter Feb 23, 2024

Choose a reason for hiding this comment

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

seems Descend can only search items from [pivot, first], no need a full scan, ok for now

pkg/infoschema/infoschema_v2.go Show resolved Hide resolved
pkg/infoschema/infoschema_v2.go Show resolved Hide resolved
pkg/infoschema/infoschema_v2.go Outdated Show resolved Hide resolved
pkg/infoschema/infoschema_v2.go Outdated Show resolved Hide resolved
func (is *infoschemaV2) SchemaExists(schema model.CIStr) bool {
var ok bool
// TODO: support different version
is.schemaMap.Scan(func(item schemaItem) bool {
Copy link
Contributor

@D3Hunter D3Hunter Feb 23, 2024

Choose a reason for hiding this comment

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

seems Descend can only search items from [pivot, first], no need a full scan, ok for now

@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 23, 2024
Copy link

ti-chi-bot bot commented Feb 23, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-02-20 11:27:17.145364859 +0000 UTC m=+356525.892987970: ☑️ agreed by GMHDBJD.
  • 2024-02-23 02:28:33.646226408 +0000 UTC m=+583402.393849519: ☑️ agreed by D3Hunter.

Copy link
Contributor

@YuJuncen YuJuncen left a comment

Choose a reason for hiding this comment

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

LGTM for BR part

Comment on lines 412 to 413
// Try to avoid repeated concurrency loading.
res, err, _ := loadTableSF.Do(fmt.Sprintf("%d-%d", dbID, tblID), func() (ret any, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be concurrent queries using different ts on same dbID-tblID?

Copy link

ti-chi-bot bot commented Feb 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, GMHDBJD, tangenta, YuJuncen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Feb 23, 2024
@ti-chi-bot ti-chi-bot bot merged commit e06dc99 into pingcap:master Feb 23, 2024
27 checks passed
@tiancaiamao tiancaiamao deleted the infoschema-v2-pr branch February 23, 2024 05:35
@tiancaiamao tiancaiamao mentioned this pull request Feb 23, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants