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

*: fix a bug that the pessimistic lock doesn't work on a partition #14921

Merged
merged 9 commits into from
Mar 3, 2020
Merged

*: fix a bug that the pessimistic lock doesn't work on a partition #14921

merged 9 commits into from
Mar 3, 2020

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

'select ... for update' doesn't work right on a partitioned table.

txn1: begin
txn1:  select * from t where a = xx for update

txn2:  update t set b = yy where a = xx

In pessimistic txn mode, txn2 should be blocked.

What is changed and how it works?

The bug is that we use table ID to construct the lock key.
On a partitioned table, partition ID should be used.

Changes:

  • During column pruning, do not prune the partition columns for LogicalLock
  • In SelectLockExec, use table ID and partition columns to locate the partition, then use the partition ID to construct the lock key.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Write release note for bug-fix or new feature.

@tiancaiamao
*: fix a bug that 'select for update' doesn't lock a partition
22b3820 
physicalID should be used if 'select for update' works on a
partitioned table.
@tiancaiamao tiancaiamao requested a review from a team as a code owner February 24, 2020 14:36
@ghost ghost requested review from eurekaka and francis0407 and removed request for a team February 24, 2020 14:36
@tiancaiamao tiancaiamao added type/bugfix This PR fixes a bug. sig/execution SIG execution sig/planner SIG: Planner sig/transaction SIG:Transaction labels Feb 24, 2020
@tiancaiamao
Copy link
Contributor Author

PTAL @coocood @winoros

@tiancaiamao tiancaiamao removed the sig/transaction SIG:Transaction label Feb 24, 2020
@imtbkcat
Copy link

@tiancaiamao please fix check_dev

@codecov
Copy link

codecov bot commented Feb 25, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@5fce112). Click here to learn what that means.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master     #14921   +/-   ##
===========================================
  Coverage          ?   80.5881%           
===========================================
  Files             ?        502           
  Lines             ?     133269           
  Branches          ?          0           
===========================================
  Hits              ?     107399           
  Misses            ?      17500           
  Partials          ?       8370

@tiancaiamao
Copy link
Contributor Author

Done @imtbkcat @coocood @winoros

coocood
coocood reviewed Feb 26, 2020
View reviewed changes
executor/executor.go Outdated Show resolved Hide resolved
coocood
coocood reviewed Feb 26, 2020
View reviewed changes
session/session_test.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor Author

PTAL @coocood @winoros

@tiancaiamao tiancaiamao changed the title *: fix a bug that 'select for update' doesn't lock a partition *: fix a bug that the pessimistic lock doesn't work on a partition Feb 27, 2020
@coocood
Copy link
Member

coocood commented Feb 27, 2020

LGTM

@tiancaiamao
Copy link
Contributor Author

PTAL @winoros

1 similar comment
@tiancaiamao
Copy link
Contributor Author

PTAL @winoros

winoros
winoros reviewed Mar 3, 2020
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@winoros
Copy link
Member

winoros commented Mar 3, 2020

/merge

@tiancaiamao tiancaiamao removed the request for review from coocood March 3, 2020 06:18
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 3, 2020
@tiancaiamao
Copy link
Contributor Author

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 3, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

@tiancaiamao merge failed.

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao tiancaiamao merged commit b3469e7 into pingcap:master Mar 3, 2020
@tiancaiamao tiancaiamao deleted the pessimistic-on-partition branch March 3, 2020 07:06
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 3, 2020
@tiancaiamao @sre-bot
cherry pick pingcap#14921 to release-3.0
589e927 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Mar 3, 2020
Closed
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

cherry pick to release-3.0 in PR #15085

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 3, 2020
@tiancaiamao @sre-bot
*: fix a bug that the pessimistic lock doesn't work on a partition (p…
928101a 
…ingcap#14921)

physicalID should be used if 'select for update' works on a
partitioned table.
@sre-bot sre-bot mentioned this pull request Mar 3, 2020
Closed
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

cherry pick to release-4.0 in PR #15086

tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Mar 4, 2020
@tiancaiamao
*: fix a bug that the pessimistic lock doesn't work on a partition (p…
1aa5fb9 
…ingcap#14921)

physicalID should be used if 'select for update' works on a
partitioned table.
@tiancaiamao tiancaiamao mentioned this pull request Mar 4, 2020
Merged
sre-bot pushed a commit that referenced this pull request Mar 9, 2020
@tiancaiamao
*: fix a bug that the pessimistic lock doesn't work on a partition (#…
bd70523 
…14921) (#15114)
@tiancaiamao tiancaiamao mentioned this pull request Nov 19, 2020
Merged
This was referenced Jun 16, 2021
Merged
Merged
@ti-srebot ti-srebot mentioned this pull request Jun 30, 2021
Closed
@tiancaiamao tiancaiamao mentioned this pull request Oct 13, 2021
Closed
12 tasks
This was referenced Dec 26, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coocood coocood coocood left review comments

@winoros winoros winoros approved these changes

Assignees
No one assigned
Labels
sig/execution SIG execution sig/planner SIG: Planner 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
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tiancaiamao @imtbkcat @coocood @winoros @sre-bot