store/tikv: fix duplicated lock keys caused data inconsistency. #16752
Conversation
coocood
added
type/bug-fix
ghost
requested review from
| @@ -3259,7 +3259,7 @@ func (b *PlanBuilder) buildUpdate(ctx context.Context, update *ast.UpdateStmt) ( | |||
| } | |||
| } | |||
| if b.ctx.GetSessionVars().TxnCtx.IsPessimistic { | |||
| if !update.MultipleTable { | |||
There was a problem hiding this comment.
Maybe remove these unused fields to avoid using mistakenly in the future, so dangerous
There was a problem hiding this comment.
It's in another repo, we can do it later.
| }) | ||
| deduped := keys[:1] | ||
| for i := 1; i < len(keys); i++ { | ||
| if !bytes.Equal(deduped[len(deduped)-1], keys[i]) { |
There was a problem hiding this comment.
Should we add warn logs here to remind there could be possible bugs?
There was a problem hiding this comment.
Maybe we need to support lock duplicated keys in the future.
| @@ -3259,7 +3259,7 @@ func (b *PlanBuilder) buildUpdate(ctx context.Context, update *ast.UpdateStmt) ( | |||
| } | |||
| } | |||
| if b.ctx.GetSessionVars().TxnCtx.IsPessimistic { | |||
| if !update.MultipleTable { | |||
| if update.TableRefs.TableRefs.Right == nil { | |||
There was a problem hiding this comment.
It is better to add some comments.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, it works as expected.
There was a problem hiding this comment.
The world is too dangerous...
Codecov Report
@@ Coverage Diff @@
## master #16752 +/- ##
===========================================
Coverage 80.4432% 80.4432%
===========================================
Files 507 507
Lines 137477 137477
===========================================
Hits 110591 110591
Misses 18282 18282
Partials 8604 8604 |
| @@ -357,6 +359,7 @@ func (txn *tikvTxn) LockKeys(ctx context.Context, lockCtx *kv.LockCtx, keysInput | |||
| if len(keys) == 0 { | |||
| return nil | |||
| } | |||
| keys = deduplicateKeys(keys) | |||
There was a problem hiding this comment.
Is it necessary after
https://github.com/pingcap/tidb/pull/16752/files#diff-b9e75a99170ea35a67074d5d3d3305adR3262
changed?
There was a problem hiding this comment.
It's the second line of defense.
There was a problem hiding this comment.
the proportion would be tiny.
|
/merge |
sre-bot
added
the
status/can-merge
|
/run-all-tests |
|
@coocood merge failed. |
bb7133
added
the
priority/release-blocker
|
/run-integration-copr-test |
Signed-off-by: sre-bot <sre-bot@pingcap.com>
|
cherry pick to release-4.0 in PR #16769 |
What problem does this PR solve?
Problem Summary:
Update
MultipleTablefield is not correct, adding SelectLock operator for multiple table update causesLockKeyscontains duplicated keys.When LockKeys contains duplicated keys, the transaction may overwrite Put operation with Lock operation, cause admin check table failure.
What is changed and how it works?
What's Changed:
LockKeysmethod.Related changes
Check List
Tests
Side effects
Release note
fix duplicated lock keys caused data inconsistency.