*: refactor ExecuteInternal to return single resultset (#22546) by ti-srebot · Pull Request #22640 · pingcap/tidb

ti-srebot · 2021-02-01T06:07:04Z

cherry-pick #22546 to release-4.0
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr https://github.com/pingcap/tidb/pull/22640

After apply modifications, you can push your change to this PR via:

git push git@github.com:ti-srebot/tidb.git pr/22640:release-4.0-7ca1629d1a51

What problem does this PR solve?

Problem Summary:

Both Execute and ExecuteInternal no longer permit multi-statement, but the return type was maintained the same for backward compatibility.

This changes ExecuteInternal to return a single result-set.

Update: I've removed Execute from this PR, we can handle it in followup PRs.

What is changed and how it works?

What's Changed:

This changes the return type of ExecuteInternal to be a non-array value. ~~All uses of Execute are changed to use ExecuteInternal~~ (for future PRs - it does not need to be handled all at once).

Related changes

Check List

Tests

Covered by existing unit test

Side effects

More difficult cherry-picks of code

Release note

No release note

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-02-01T06:07:07Z

/run-all-tests

ti-srebot · 2021-02-01T06:07:16Z

@morgo please accept the invitation then you can push to the cherry-pick pull requests.
https://github.com/ti-srebot/tidb/invitations

tiancaiamao · 2021-02-04T03:09:20Z

Wow, so many conflicts...

morgo · 2021-02-04T03:11:36Z

This depends on #22548 - I will wait for it to merge first.

xhebox · 2021-02-04T03:12:56Z

But that is blocked on merge. We may need to apply for cherry-picking.

xhebox

Rest LGTM

xhebox · 2021-03-02T03:13:28Z

bindinfo/handle.go

 	sctx.GetSessionVars().UsePlanBaselines = oriVals
-	if len(recordSets) > 0 {
-		defer terror.Log(recordSets[0].Close())
+	if rs != nil {


Same to the 5.0-rc, we can remove the check, and move the defer after err != nil check.

xhebox · 2021-03-02T03:13:52Z

bindinfo/handle.go

 	if err != nil {
-		if len(recordSets) > 0 {
-			terror.Call(recordSets[0].Close)
+		if rs != nil {


xhebox · 2021-03-02T03:14:17Z

session/bootstrap.go

 	}
-	if len(recordSets) > 0 {
-		defer terror.Call(recordSets[0].Close)
+	if rs != nil {


xhebox · 2021-03-02T03:14:29Z

session/session.go

-	}()
-	if err != nil {
+	rs, err := se.ExecuteInternal(ctx, sql)
+	if rs != nil {


xhebox · 2021-03-03T03:23:45Z

/lgtm

AilinKid

/lgtm

AilinKid · 2021-03-05T03:28:29Z

/lgtm

ti-chi-bot · 2021-03-05T03:28:32Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AilinKid
xhebox

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.

Details

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

xhebox · 2021-03-05T04:44:19Z

/merge

ti-chi-bot · 2021-03-05T04:44:20Z

@xhebox: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

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 ti-community-infra/tichi repository.

xhebox · 2021-03-05T04:48:16Z

/merge cancel

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox · 2021-03-05T05:09:06Z

/merge

ti-chi-bot · 2021-03-05T05:09:07Z

@xhebox: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

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 ti-community-infra/tichi repository.

ti-chi-bot · 2021-03-05T05:09:10Z

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

Details

Commit hash: 56519e6

cherry pick pingcap#22546 to release-4.0

0e6794e

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Feb 1, 2021

*: refactor ExecuteInternal to return single resultset #22546

Merged

ti-srebot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra sig/planner SIG: Planner type/4.0-cherry-pick labels Feb 1, 2021

ti-srebot requested review from AilinKid, XuHuaiyu, tiancaiamao and xhebox February 1, 2021 06:07

ti-srebot added this to the v4.0.11 milestone Feb 1, 2021

ti-srebot assigned morgo Feb 1, 2021

morgo mentioned this pull request Feb 3, 2021

privilege: remove any string concat (#22523) #22689

Merged

github-actions bot added the sig/infra label Feb 22, 2021

ti-chi-bot removed the sig/infra label Feb 22, 2021

morgo mentioned this pull request Mar 1, 2021

session, util: update session to use new APIs (#22652) #22804

Merged

morgo added 3 commits March 1, 2021 14:02

wip

84b3b82

Merge remote-tracking branch 'upstream/release-4.0' into pr/22640

c8e0666

change executeInternal to single return value

14e8d4c

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 1, 2021

xhebox reviewed Mar 2, 2021

View reviewed changes

Address PR feedback

4dde70f

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

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 3, 2021

Merge remote-tracking branch 'upstream/release-4.0' into pr/22640

68c5c9c

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

AilinKid reviewed Mar 5, 2021

View reviewed changes

Merge branch 'release-4.0' into release-4.0-7ca1629d1a51

0488382

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 5, 2021

AilinKid approved these changes Mar 5, 2021

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 5, 2021

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 5, 2021

*: fix built

56519e6

Signed-off-by: xhe <xw897002528@gmail.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 5, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 5, 2021

ti-chi-bot merged commit 649d0e0 into pingcap:release-4.0 Mar 5, 2021

zhouqiang-cl modified the milestones: v4.0.11, v4.0.12 Mar 26, 2021

Conversation

ti-srebot commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-srebot commented Feb 1, 2021

Uh oh!

ti-srebot commented Feb 1, 2021

Uh oh!

tiancaiamao commented Feb 4, 2021

Uh oh!

morgo commented Feb 4, 2021

Uh oh!

xhebox commented Feb 4, 2021

Uh oh!

xhebox left a comment

Choose a reason for hiding this comment

Uh oh!

xhebox Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

xhebox Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

xhebox Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

xhebox Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

xhebox commented Mar 3, 2021

Uh oh!

AilinKid left a comment

Choose a reason for hiding this comment

Uh oh!

AilinKid commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

xhebox commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

xhebox commented Mar 5, 2021

Uh oh!

xhebox commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ti-srebot commented Feb 1, 2021 •

edited

Loading