*: add context.Context parameter to infoschema TableByName #54479

tiancaiamao · 2024-07-05T10:03:23Z

What problem does this PR solve?

Issue Number: ref #50959

Problem Summary:

What changed and how does it work?

In infoschema v1, all schema meta are in-memory, so TableByName are fast enough.
In infoschema v2, there may be cache miss and loading from remote network, that could be slower.

Add a context.Context parameter so it is easier to trace and debug the cache miss cases.

Check List

Tests

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

curl http://127.0.0.1:10080/debug/pprof/trace?seconds=3s > trace.out
go tool trace -http=:6060 trace.out

The API responsive duration distribution:

The execute trace of TableByName:

With those trace information, we can analyze the reason of some of the p999 slowness.

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

Release note

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

None

tiprow · 2024-07-05T10:03:45Z

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-sigs/prow repository.

codecov · 2024-07-05T10:48:26Z

Codecov Report

Attention: Patch coverage is 95.23810% with 9 lines in your changes missing coverage. Please review.

Project coverage is 57.0108%. Comparing base (49f1427) to head (d170390).
Report is 12 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54479         +/-   ##
=================================================
- Coverage   74.7101%   57.0108%   -17.6994%     
=================================================
  Files          1545       1692        +147     
  Lines        362047     619328     +257281     
=================================================
+ Hits         270486     353084      +82598     
- Misses        71923     241912     +169989     
- Partials      19638      24332       +4694

Flag	Coverage Δ
integration	`39.6894% <72.4867%> (?)`
unit	`72.0615% <91.0052%> (-1.5597%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (-2.2339%)`	⬇️
parser	`∅ <ø> (∅)`
br	`62.9004% <100.0000%> (+15.2190%)`	⬆️

lance6716

Using Context.Background() doesn't make thing worse so LGTM. And I hope we can use correct context in this PR or the following PR.

br/pkg/restore/log_client/client.go

pkg/ddl/ddl_api.go

pkg/ddl/foreign_key.go

pkg/domain/extract.go

lance6716 · 2024-07-08T02:20:48Z

pkg/domain/plan_replayer_dump.go

@@ -157,7 +157,7 @@ func (tne *tableNameExtractor) handleIsView(t *ast.TableName) (bool, error) {
 	if !isView {
 		return false, nil
 	}
-	viewTbl, err := tne.is.TableByName(schema, table)
+	viewTbl, err := tne.is.TableByName(context.Background(), schema, table)


receiver has ctx

ti-chi-bot · 2024-07-09T08:31:23Z

[LGTM Timeline notifier]

Timeline:

2024-07-08 02:23:12.796316699 +0000 UTC m=+237890.031550811: ☑️ agreed by lance6716.
2024-07-09 08:31:22.460075793 +0000 UTC m=+346379.695309910: ☑️ agreed by tangenta.

tiprow · 2024-07-09T08:45:31Z

@tiancaiamao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`d170390`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

bb7133

LGTM

ti-chi-bot · 2024-07-09T11:15:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, AilinKid, bb7133, lance6716, tangenta, XuHuaiyu

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:

~~OWNERS~~ [3pointer,AilinKid,XuHuaiyu,bb7133,lance6716,tangenta]
~~br/OWNERS~~ [3pointer]
~~pkg/autoid_service/OWNERS~~ [bb7133]
~~pkg/ddl/OWNERS~~ [tangenta]
~~pkg/disttask/OWNERS~~ [lance6716,tangenta]
~~pkg/domain/OWNERS~~ [lance6716,tangenta]
~~pkg/executor/importer/OWNERS~~ [lance6716]
~~pkg/expression/OWNERS~~ [XuHuaiyu]
~~pkg/infoschema/OWNERS~~ [tangenta]
~~pkg/lock/OWNERS~~ [lance6716,tangenta]
~~pkg/planner/OWNERS~~ [AilinKid]
~~pkg/statistics/OWNERS~~ [AilinKid]
~~pkg/table/OWNERS~~ [tangenta]

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

tiancaiamao · 2024-07-09T13:10:04Z

/test pull-lightning-integration-test
/test check-dev2

tiprow · 2024-07-09T13:10:26Z

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

In response to this:

/test pull-lightning-integration-test
/test check-dev2

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-sigs/prow repository.

lance6716 · 2024-07-09T14:45:20Z

/retest

tiprow · 2024-07-09T14:45:44Z

@lance6716: 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-sigs/prow repository.

ti-chi-bot · 2024-07-09T15:35:33Z

@tiancaiamao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/check_dev_2	`d170390`	link	unknown	`/test check-dev2`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

tiancaiamao · 2024-07-09T15:55:05Z

/test check-dev2

tiprow · 2024-07-09T15:55:30Z

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

In response to this:

/test check-dev2

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-sigs/prow repository.

tiancaiamao added 3 commits July 5, 2024 08:41

add a context parameter to TableByName

817a7d9

add trace

ef702d3

add a test to make sure it works

6e2c9f8

ti-chi-bot bot added release-note-none sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 5, 2024

Merge branch 'master' into trace

cfc7123

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2024

lance6716 approved these changes Jul 8, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label Jul 8, 2024

tiancaiamao added 2 commits July 8, 2024 20:55

address comment

af5ca05

Merge branch 'master' into trace

324146c

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024

tiancaiamao added 2 commits July 9, 2024 14:40

Merge branch 'master' into trace

50946a6

fix build

d170390

tangenta approved these changes Jul 9, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm labels Jul 9, 2024

3pointer approved these changes Jul 9, 2024

View reviewed changes

AilinKid approved these changes Jul 9, 2024

View reviewed changes

XuHuaiyu approved these changes Jul 9, 2024

View reviewed changes

bb7133 approved these changes Jul 9, 2024

View reviewed changes

ti-chi-bot bot added the approved label Jul 9, 2024

ti-chi-bot bot merged commit f490a98 into pingcap:master Jul 9, 2024
31 of 33 checks passed

tiancaiamao deleted the trace branch July 10, 2024 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add context.Context parameter to infoschema TableByName #54479

*: add context.Context parameter to infoschema TableByName #54479

tiancaiamao commented Jul 5, 2024

tiprow bot commented Jul 5, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading

lance6716 left a comment

lance6716 Jul 8, 2024

ti-chi-bot bot commented Jul 9, 2024

tiprow bot commented Jul 9, 2024

bb7133 left a comment

ti-chi-bot bot commented Jul 9, 2024

tiancaiamao commented Jul 9, 2024

tiprow bot commented Jul 9, 2024

lance6716 commented Jul 9, 2024

tiprow bot commented Jul 9, 2024

ti-chi-bot bot commented Jul 9, 2024

tiancaiamao commented Jul 9, 2024

tiprow bot commented Jul 9, 2024

*: add context.Context parameter to infoschema TableByName #54479

*: add context.Context parameter to infoschema TableByName #54479

Conversation

tiancaiamao commented Jul 5, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 5, 2024

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Jul 8, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 9, 2024

[LGTM Timeline notifier]

tiprow bot commented Jul 9, 2024

bb7133 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 9, 2024

tiancaiamao commented Jul 9, 2024

tiprow bot commented Jul 9, 2024

lance6716 commented Jul 9, 2024

tiprow bot commented Jul 9, 2024

ti-chi-bot bot commented Jul 9, 2024

tiancaiamao commented Jul 9, 2024

tiprow bot commented Jul 9, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading