session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path #36347

SpadeA-Tang · 2022-07-20T01:41:14Z

What problem does this PR solve?

Issue Number: close #36311

Problem Summary:

What is changed and how it works?

This PR solves the problem of extra tso cmd requests after the PR #35158.

We have an optimization for not fetch tso from PD for point get for update related operations including operating includes them (update, delete and so on). However, it is missed in prepare-pattern. This PR supplements it.

Tso cmd request counts are not related to the correctness of the programme and have not been covered by the current unit tests. This PR also adds the related unit test to avoid similar mistakes in the future.

Check List

Tests

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

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

ti-chi-bot · 2022-07-20T01:41:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

ekexium
lcwangchao

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: SpadeA-Tang <[email protected]>

ekexium · 2022-07-20T09:59:17Z

session/session.go

@@ -2267,6 +2267,9 @@ func (s *session) preparedStmtExec(ctx context.Context, execStmt *ast.ExecuteStm

 	is := sessiontxn.GetTxnManager(s).GetTxnInfoSchema()
 	st, tiFlashPushDown, tiFlashExchangePushDown, err := executor.CompileExecutePreparedStmt(ctx, s, execStmt, is)
+	if err == nil {
+		err = sessiontxn.OptimizeWithPlanAndThenWarmUp(s, st.Plan)


Why not put it after the if err != nil below, so that the if err == nill is not needed?

It's the same. In that way, we have to deal with another if err != nil.

I think it would be more idiomatic?

Actually, it is written this way in text sql path in ExecuteStmt.

sre-bot · 2022-07-20T10:02:12Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/1b0bebeed0f784415353d517bebf781a3ca0a673

lcwangchao

Rest LGTM

sessiontxn/txn_context_test.go

Signed-off-by: SpadeA-Tang <[email protected]>

sessiontxn/txn_context_test.go

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao · 2022-07-21T05:54:49Z

/merge

ti-chi-bot · 2022-07-21T05:54:52Z

This pull request has been accepted and is ready to merge.

Commit hash: 78b43ad

SpadeA-Tang · 2022-07-21T06:51:02Z

/run-unit-test

SpadeA-Tang · 2022-07-21T06:51:14Z

/run-check_dev_2

SpadeA-Tang · 2022-07-21T08:24:08Z

/run-check_dev_2

SpadeA-Tang · 2022-07-21T10:48:11Z

/run-check_dev_2

sre-bot · 2022-07-21T14:57:57Z

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/tics-test	🟥 failed 1, success 0, total 1	7 min 53 sec	New failing
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 2, success 9, total 11	49 min	Existing failure
idc-jenkins-ci-tidb/integration-ddl-test	🔴 failed 1, success 5, total 6	7 min 24 sec	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 36 tests passed	30 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 12 tests passed	19 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	8 min 42 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	6 min 47 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 27 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 16 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

…ip-init * upstream/master: (125 commits) infoschema: fix PromQL for `tidb_distsql_copr_cache` (pingcap#36450) test: stabilize TestTopSQLCPUProfile (pingcap#36468) parser: add support of 'ADMIN SHOW DDL JOB QUERIES LIMIT m OFFSET n' transferring to AST (pingcap#36285) *: enable flaky test for all test (pingcap#36385) expression: fix return type of agg func `bit_or` when handling varbinary column (pingcap#36415) executor: fix aggregating enum zero value gets different results from mysql (pingcap#36208) server: skip check tiflash version (pingcap#36451) *: Minor update to SECURITY.md to improved clarity (pingcap#36346) table partition: add telemetry for partition table (pingcap#36204) ddl: invalid multiple MAXVALUE partitions (pingcap#36329) (pingcap#36345) planner: Fixed `Merge` hint in nested CTE (pingcap#36432) metric: impove concurrency ddl metrics (pingcap#36405) planner: add more test cases for leading outer join (pingcap#36409) ddl: only set concurrent variable if no error (pingcap#36437) ddl: fix update panic in the middle of multi-schema change (pingcap#36421) session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path (pingcap#36347) executor,metrics: add a metric for observing execution phases (pingcap#35906) br: unified docker image align with tidb (pingcap#36016) ddl: skip to close nil sessPool (pingcap#36425) log-backup: remove the timezone from log-date (pingcap#36369) ...

* upstream/master: (280 commits) infoschema: fix PromQL for `tidb_distsql_copr_cache` (pingcap#36450) test: stabilize TestTopSQLCPUProfile (pingcap#36468) parser: add support of 'ADMIN SHOW DDL JOB QUERIES LIMIT m OFFSET n' transferring to AST (pingcap#36285) *: enable flaky test for all test (pingcap#36385) expression: fix return type of agg func `bit_or` when handling varbinary column (pingcap#36415) executor: fix aggregating enum zero value gets different results from mysql (pingcap#36208) server: skip check tiflash version (pingcap#36451) *: Minor update to SECURITY.md to improved clarity (pingcap#36346) table partition: add telemetry for partition table (pingcap#36204) ddl: invalid multiple MAXVALUE partitions (pingcap#36329) (pingcap#36345) planner: Fixed `Merge` hint in nested CTE (pingcap#36432) metric: impove concurrency ddl metrics (pingcap#36405) planner: add more test cases for leading outer join (pingcap#36409) ddl: only set concurrent variable if no error (pingcap#36437) ddl: fix update panic in the middle of multi-schema change (pingcap#36421) session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path (pingcap#36347) executor,metrics: add a metric for observing execution phases (pingcap#35906) br: unified docker image align with tidb (pingcap#36016) ddl: skip to close nil sessPool (pingcap#36425) log-backup: remove the timezone from log-date (pingcap#36369) ...

…rimary-key * upstream/master: (104 commits) br: fix compatibility issue with concurrent ddl (pingcap#36474) infoschema: fix PromQL for `tidb_distsql_copr_cache` (pingcap#36450) test: stabilize TestTopSQLCPUProfile (pingcap#36468) parser: add support of 'ADMIN SHOW DDL JOB QUERIES LIMIT m OFFSET n' transferring to AST (pingcap#36285) *: enable flaky test for all test (pingcap#36385) expression: fix return type of agg func `bit_or` when handling varbinary column (pingcap#36415) executor: fix aggregating enum zero value gets different results from mysql (pingcap#36208) server: skip check tiflash version (pingcap#36451) *: Minor update to SECURITY.md to improved clarity (pingcap#36346) table partition: add telemetry for partition table (pingcap#36204) ddl: invalid multiple MAXVALUE partitions (pingcap#36329) (pingcap#36345) planner: Fixed `Merge` hint in nested CTE (pingcap#36432) metric: impove concurrency ddl metrics (pingcap#36405) planner: add more test cases for leading outer join (pingcap#36409) ddl: only set concurrent variable if no error (pingcap#36437) ddl: fix update panic in the middle of multi-schema change (pingcap#36421) session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path (pingcap#36347) executor,metrics: add a metric for observing execution phases (pingcap#35906) br: unified docker image align with tidb (pingcap#36016) ddl: skip to close nil sessPool (pingcap#36425) ...

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 20, 2022

update

d9fea81

Signed-off-by: SpadeA-Tang <[email protected]>

SpadeA-Tang force-pushed the test_tso branch from bb42064 to d9fea81 Compare July 20, 2022 01:45

ti-chi-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 20, 2022

update

c11de62

Signed-off-by: SpadeA-Tang <[email protected]>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/needs-triage-completed and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/needs-linked-issue labels Jul 20, 2022

Merge branch 'master' into test_tso

7783fd8

ti-chi-bot removed the do-not-merge/needs-triage-completed label Jul 20, 2022

ekexium reviewed Jul 20, 2022

View reviewed changes

lcwangchao reviewed Jul 20, 2022

View reviewed changes

sessiontxn/txn_context_test.go Outdated Show resolved Hide resolved

sessiontxn/txn_context_test.go Outdated Show resolved Hide resolved

SpadeA-Tang added 2 commits July 20, 2022 18:18

update

f145e13

Signed-off-by: SpadeA-Tang <[email protected]>

fmt

bafdcfa

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao reviewed Jul 20, 2022

View reviewed changes

sessiontxn/txn_context_test.go Show resolved Hide resolved

sessiontxn/txn_context_test.go Show resolved Hide resolved

update

0b3e536

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao approved these changes Jul 21, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 21, 2022

SpadeA-Tang and others added 2 commits July 21, 2022 10:22

Merge branch 'master' into test_tso

5f28984

update

609d1d6

Signed-off-by: SpadeA-Tang <[email protected]>

ekexium approved these changes Jul 21, 2022

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 21, 2022

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 21, 2022

Merge branch 'master' into test_tso

78b43ad

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 21, 2022

Merge branch 'master' into test_tso

445d602

Merge branch 'master' into test_tso

108b481

Merge branch 'master' into test_tso

3bd671d

ti-chi-bot added 2 commits July 21, 2022 17:37

Merge branch 'master' into test_tso

aa40a49

Merge branch 'master' into test_tso

adc3d89

ti-chi-bot and others added 4 commits July 21, 2022 18:48

Merge branch 'master' into test_tso

b2e137d

Merge branch 'master' into test_tso

a8cf02c

Merge branch 'master' into test_tso

1121b37

Merge branch 'master' into test_tso

1b0bebe

ti-chi-bot merged commit b377810 into pingcap:master Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path #36347

session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path #36347

SpadeA-Tang commented Jul 20, 2022 •

edited by ekexium

Loading

ti-chi-bot commented Jul 20, 2022 •

edited

Loading

ekexium Jul 20, 2022

SpadeA-Tang Jul 20, 2022

ekexium Jul 20, 2022

SpadeA-Tang Jul 20, 2022

sre-bot commented Jul 20, 2022 •

edited

Loading

lcwangchao left a comment

lcwangchao commented Jul 21, 2022

ti-chi-bot commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

sre-bot commented Jul 21, 2022

session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path #36347

session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path #36347

Conversation

SpadeA-Tang commented Jul 20, 2022 • edited by ekexium Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 20, 2022 • edited Loading

ekexium Jul 20, 2022

Choose a reason for hiding this comment

SpadeA-Tang Jul 20, 2022

Choose a reason for hiding this comment

ekexium Jul 20, 2022

Choose a reason for hiding this comment

SpadeA-Tang Jul 20, 2022

Choose a reason for hiding this comment

sre-bot commented Jul 20, 2022 • edited Loading

lcwangchao left a comment

Choose a reason for hiding this comment

lcwangchao commented Jul 21, 2022

ti-chi-bot commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

SpadeA-Tang commented Jul 21, 2022

sre-bot commented Jul 21, 2022

TiDB MergeCI notify

SpadeA-Tang commented Jul 20, 2022 •

edited by ekexium

Loading

ti-chi-bot commented Jul 20, 2022 •

edited

Loading

sre-bot commented Jul 20, 2022 •

edited

Loading