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

tables: fix insert ignore on duplicate with dup prefix 2nd index #25905

Merged
merged 1 commit into from
Aug 23, 2021
Merged

tables: fix insert ignore on duplicate with dup prefix 2nd index #25905

merged 1 commit into from
Aug 23, 2021

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Jul 2, 2021

What problem does this PR solve?

Issue Number: close #25809

Problem Summary:

now it relies on modified array to decide whether skip modified index dup check after duplicate updated.

but when clustered primary key has same column with secondary index,and the column is prefix, changed in table doesn't indicate it be change in index.

What is changed and how it works?

What's Changed, How it Works:

truncate datum to index length and re-check modified for prefix column

Related changes

  • Need to cherry-pick to the release branch 4.0, 3.0

Check List

Tests

  • Unit test

Side effects

  • n/a

Release note

  • Fix the wrong result after "insert ignore on duplicate update" in the secondary index has the same column in primary key but has prefix length

@lysu lysu requested a review from a team as a code owner July 2, 2021 09:10
@lysu lysu requested review from lzmhhh123 and removed request for a team July 2, 2021 09:10
@ti-chi-bot ti-chi-bot added do-not-merge/cherry-pick-not-approved size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 2, 2021
@lysu lysu linked an issue Jul 2, 2021 that may be closed by this pull request
clustered string primary with dup prefixed uk, insert ignore ... on duplicate key update return wrong result #25809
Closed
@lysu lysu requested review from cfzjywxk and jackysp and removed request for lzmhhh123 July 2, 2021 09:11
@lysu lysu added backport-5.0.3 type/bugfix This PR fixes a bug. labels Jul 2, 2021
@ichn-hu ichn-hu mentioned this pull request Jul 2, 2021
Closed
@github-actions github-actions bot added the sig/execution SIG execution label Jul 2, 2021
cfzjywxk
cfzjywxk reviewed Jul 2, 2021
View reviewed changes
table/tables/tables.go
}
for _, c := range idx.Meta().Columns {
if modified[c.Offset] {
return false
if c.Length != types.UnspecifiedLength && (newRow[c.Offset].Kind() == types.KindString || newRow[c.Offset].Kind() == types.KindBytes) {
Copy link
Contributor

@cfzjywxk cfzjywxk Jul 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Besides KindString, will other prefix index column types need similar processing like this?

Copy link
Member

Choose a reason for hiding this comment

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

Agree, feel that there may still be problems later, but this type of SQL execution is rare, so it may okay for this release branch.

jackysp
jackysp approved these changes Jul 5, 2021
View reviewed changes
Copy link
Member

@jackysp jackysp 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 the status/LGT1 Indicates that a PR has LGTM 1. label Jul 5, 2021
cfzjywxk
cfzjywxk approved these changes Jul 5, 2021
View reviewed changes
Copy link
Contributor

@cfzjywxk cfzjywxk 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
Copy link
Member

@cfzjywxk: Please use GitHub review feature instead of /lgtm [cancel] when you want to submit review to the pull request.
For how to use GitHub review feature, see also this document provided by GitHub.

For the reason we drop support to the commands, see also this page.
This reply is being used as a temporary reply during the migration of review process and will be removed on July 1st.

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
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • cfzjywxk
  • jackysp

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

cfzjywxk commented Jul 5, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 307a55d

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

tisonkun commented Jul 29, 2021 •
edited
Loading

@lysu shall we pick this PR to master? It is interesting we first fix the issue on a release branch.

FOUND: #25809 (comment)

@zhouqiang-cl zhouqiang-cl added the cherry-pick-approved Cherry pick PR approved by release team. label Aug 12, 2021
@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/cherry-pick-not-approved labels Aug 12, 2021
@lysu
Copy link
Contributor Author

lysu commented Aug 23, 2021

@lysu shall we pick this PR to master? It is interesting we first fix the issue on a release branch.

FOUND: #25809 (comment)

hi just as #25809 (comment) said, the master branch use a different mechanism and doesn't has this bug

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2021
@lysu
Copy link
Contributor Author

lysu commented Aug 23, 2021

/merge

@lysu
Copy link
Contributor Author

lysu commented Aug 23, 2021

/run-unit-test

@ti-chi-bot ti-chi-bot merged commit 0e278a1 into pingcap:release-5.0 Aug 23, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 23, 2021
@lysu @ti-srebot
cherry pick pingcap#25905 to release-3.0
b850e86 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 23, 2021
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #27510

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 23, 2021
@lysu @ti-srebot
cherry pick pingcap#25905 to release-4.0
cdfe3c0 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 23, 2021
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #27511

@zhouqiang-cl zhouqiang-cl added this to the v5.0.4 milestone Sep 10, 2021
This was referenced Sep 15, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfzjywxk cfzjywxk cfzjywxk approved these changes

@jackysp jackysp jackysp approved these changes

Assignees
No one assigned
Labels
cherry-pick-approved Cherry pick PR approved by release team. sig/execution SIG execution 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. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
v5.0.4
Development

Successfully merging this pull request may close these issues.

clustered string primary with dup prefixed uk, insert ignore ... on duplicate key update return wrong result
7 participants
@lysu @ti-chi-bot @cfzjywxk @tisonkun @ti-srebot @jackysp @zhouqiang-cl