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.

Sign up for GitHub

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

Jump to bottom

planner/core: fix index out of bound bug when empty dual table is remove for mpp query #28251

Merged
merged 3 commits into from
Sep 23, 2021
Merged

planner/core: fix index out of bound bug when empty dual table is remove for mpp query #28251

merged 3 commits into from
Sep 23, 2021

Conversation

windtalker
Copy link
Contributor

@windtalker windtalker commented Sep 22, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #28250

Problem Summary:
As described in the issue.

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:
The root cause is after #25218, union may contain less than 2 children, but in stringer.ToString, it still assume that union always contains at least 2 children. This pr refine the related code to consider the case that union contains less than 2 children.

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

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

Release note 

Fix index out of bound bug when empty dual table is remove for mpp query

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 22, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • fzhedu
  • wjhuang2016

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.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 22, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Sep 22, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

  • *: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

@windtalker windtalker changed the title fix index out of bound bug when empty dual table is remove for mpp query planner/core: fix index out of bound bug when empty dual table is remove for mpp query Sep 22, 2021
@windtalker
Copy link
Contributor Author

/label needs-cherry-pick-5.1

@windtalker
Copy link
Contributor Author

/label needs-cherry-pick-5.0

@windtalker
Copy link
Contributor Author

/label needs-cherry-pick-5.2

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 22, 2021
fzhedu
fzhedu reviewed Sep 22, 2021
View reviewed changes
planner/core/stringer.go
Comment on lines +33 to +36
case *LogicalUnionAll, *PhysicalUnionAll, *LogicalPartitionUnionAll:
// after https://github.com/pingcap/tidb/pull/25218, the union may contain less than 2 children,
// but we still wants to include its child plan's information when calling `toString` on union.
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

would this case happen in other cases？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean will the union may contain less than 2 children cause other bugs? I'm not sure about this, and if it does happen, we will fix it in other prs.

Copy link
Contributor

Choose a reason for hiding this comment

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

oncall 3751 can't reproduce, but maybe it's caused by the same bug? @windtalker

@yudongusa yudongusa mentioned this pull request Sep 22, 2021
Closed
fzhedu
fzhedu approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@fzhedu fzhedu 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 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 23, 2021
@fzhedu
Copy link
Contributor

fzhedu commented Sep 23, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 4ce4cdd

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 23, 2021
@fzhedu
Copy link
Contributor

fzhedu commented Sep 23, 2021

Reducing the dual table child of union may introduce other unknown issues, we should remove union if its children are less than 2.

@ti-chi-bot ti-chi-bot merged commit 452d34c into pingcap:master Sep 23, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 23, 2021
@windtalker @ti-srebot
cherry pick pingcap#28251 to release-5.0
da2ec3d 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Sep 23, 2021
Merged
12 tasks
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #28278

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 23, 2021
@windtalker @ti-srebot
cherry pick pingcap#28251 to release-5.1
cb50703 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Sep 23, 2021
Merged
12 tasks
@ti-srebot
Copy link
Contributor

cherry pick to release-5.1 in PR #28279

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 23, 2021
@windtalker @ti-srebot
cherry pick pingcap#28251 to release-5.2
7307d78 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Sep 23, 2021
Merged
12 tasks
@ti-srebot
Copy link
Contributor

cherry pick to release-5.2 in PR #28280

zhouqiang-cl pushed a commit that referenced this pull request Sep 26, 2021
@windtalker
planner/core: cherry pick #28251 to release-5.0-20210923 (#28344)
5bda4a8 
* ddl, executor: fix test race in terror (#25405) (#28198)

* cherry pick #28251 to release-5.0

Signed-off-by: ti-srebot <[email protected]>
ti-chi-bot pushed a commit that referenced this pull request Oct 13, 2021
@ti-srebot
planner/core: fix index out of bound bug when empty dual table is rem…
1ba43f9 
…ove for mpp query (#28251) (#28280)
@windtalker windtalker deleted the index_out_of_bound branch November 16, 2021 01:24
ti-chi-bot pushed a commit that referenced this pull request Dec 17, 2021
@ti-srebot
planner/core: fix index out of bound bug when empty dual table is rem…
66850a6 
…ove for mpp query (#28251) (#28278)
ti-chi-bot pushed a commit that referenced this pull request Feb 10, 2022
@ti-srebot
planner/core: fix index out of bound bug when empty dual table is rem…
b94d692 
…ove for mpp query (#28251) (#28279)

close #28250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiancaiamao tiancaiamao tiancaiamao left review comments

@wjhuang2016 wjhuang2016 wjhuang2016 approved these changes

@fzhedu fzhedu fzhedu approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

query failed with index out of range [-1]
7 participants
@windtalker @ti-chi-bot @sre-bot @fzhedu @ti-srebot @tiancaiamao @wjhuang2016