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

executor: Fix pessimistic lock doesn't work on the partition table for subquery/joins #21641

Closed
wants to merge 13 commits into from

Conversation

blacktear23
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #21618, close #21509

Problem Summary:

In SelectLockExec.Next the second parameter for GetPartitionByRow function is not contains only partitioned table's columns data, it is all selected table's result. So it will return wrong partition table ID.

What is changed and how it works?

What's Changed:
Add a projection step to filter row data to partitioned table related Datum array, then call GetPartitionByRow function.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

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

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • executor: fix pessimistic lock doesn't work on the partition table for subquery or joins

@blacktear23 blacktear23 requested a review from a team as a code owner December 10, 2020 08:13
@blacktear23 blacktear23 requested review from lzmhhh123 and removed request for a team December 10, 2020 08:13
@blacktear23
Copy link
Contributor Author

@bb7133 @zyguan PTAL

@github-actions github-actions bot added the sig/execution SIG execution label Dec 10, 2020
@bb7133
Copy link
Member

bb7133 commented Dec 10, 2020

PTAL @crazycs520 @tiancaiamao

return errors.Trace(errors.Errorf("Cannot get schema info for table %s", tblInfo.Name.O))
}
colNamePrefix := fmt.Sprintf("%s.%s.", dbInfo.Name.L, tblInfo.Name.L)
cols := pt.Cols()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to use VisibleCols instead of Cols? @bb7133

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think so.

@ichn-hu ichn-hu mentioned this pull request Dec 10, 2020
@blacktear23
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Dec 11, 2020
@youjiali1995
Copy link
Contributor

What's the status of this PR? @blacktear23

@blacktear23
Copy link
Contributor Author

@youjiali1995 Still active, waiting for review.

@youjiali1995
Copy link
Contributor

@bb7133 @lzmhhh123 PTAL

@youjiali1995 youjiali1995 requested a review from lysu January 27, 2021 05:58
@tiancaiamao
Copy link
Contributor

Fix #20028

@tiancaiamao
Copy link
Contributor

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 27, 2021
@tiancaiamao
Copy link
Contributor

I have a fix for "select lock on join" in another PR #21148, but it's lack of reviewers.
Although fixing the bug by this approach is worse in performance, I'd like to get things done ASAP.

PTAL @eurekaka

@qw4990 qw4990 self-requested a review April 22, 2021 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

*: pessimistic lock doesn't work on the partition for subquery/joins Unexpected constant overflow error
6 participants