txn: add http api to change the unique index lock behaviour #25722
Conversation
cfzjywxk
requested review from
lysu,
you06,
MyonKeminta,
youjiali1995 and
sticnarf
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
the
size/L
github-actions
bot
added
sig/execution
cfzjywxk
force-pushed
the
switch_for_unique_key_lock
branch
from
29fb82b
to
9891389
Compare
cfzjywxk
force-pushed
the
switch_for_unique_key_lock
branch
from
9891389
to
1ba58a4
Compare
| writeError(w, errors.New("illegal argument, please use 0 or 1 for this")) | ||
| return | ||
| } | ||
| } | ||
| } else { | ||
| writeData(w, config.GetGlobalConfig()) |
There was a problem hiding this comment.
maybe we need a way to get the current configuration value..
for example: someone reports a oncall issue and we want to confirm is it running in false or true status~?
or maybe we can write some log to indicate someone changed it to help to do this
There was a problem hiding this comment.
select @@tidb_lock_unique_key won't work?
There was a problem hiding this comment.
Logs are added changing the config, it may need more work to track the status infomation in the tidb log file.
There was a problem hiding this comment.
I see, tidb_lock_unique_key is not a variable.
There was a problem hiding this comment.
@jackysp
It's a http api
curl -X POST -d "tidb_lock_unique_key=1" http://127.0.0.1:10080/settings
As it's a temporary solution, better not to add system variables or configurations.
There was a problem hiding this comment.
So it is not necessary to update the docs of http api?
There was a problem hiding this comment.
Not quite sure, as it's temporary solution better not to make it very public. Also it's expected to be optimized or solved in the next release if possible.
executor/point_get.go
Outdated
| if !e.ctx.GetSessionVars().IsPessimisticReadConsistency() { | ||
| if !keyExist { | ||
| lockIndexKey = true | ||
| } else if keyExist && enableLockIdxKey { |
There was a problem hiding this comment.
in else the keyExist always true.
| @@ -751,6 +751,7 @@ var ( | |||
| ReportIntervalSeconds: atomic.NewInt64(DefTiDBTopSQLReportIntervalSeconds), | |||
| } | |||
| EnableLocalTxn = atomic.NewBool(DefTiDBEnableLocalTxn) | |||
| LockUniqueKeys = atomic.NewBool(true) | |||
There was a problem hiding this comment.
@jackysp @coocood @youjiali1995 @lysu
Or do we need to change it to false to change the behaviour back with v4.0.9 and earlier versions, but this could affect the users using v5.0 release versions if they rely on the new behaviours.
|
@cfzjywxk: PR needs rebase. 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 kubernetes/test-infra repository. |
ti-chi-bot
added
the
needs-rebase
|
#25730 has been merged, close this. |
What problem does this PR solve?
Issue Number:
Related to #25659.
Problem Summary:
Since #21229, all the unique index keys will be locked if they are used in the for update read access path. The for update read with point get on non-clusetered unique index keys may leave many
LOCKrecords which affect the performance a lot.What is changed and how it works?
What's Changed:
Add a http switch to control the behaviour, it's possible to turn off the change introduced by #21229 by setting the variable to
0.This is a temporary solution to workaround the performance issue, to solve it it's still needed to optimize the
LOCKandROLLBACKrecords in the tikv-side. It could be used as a hotfix patch if the users encounter such performance issues.How it Works:
Related changes
Check List
Tests
With the point/batch point get unique index for update read, check the mvcc record.
Side effects
Release note