txn: fix the lock behaviours when for update is used outside transactions | tidb-test=pr/2365 #54694

cfzjywxk · 2024-07-17T13:14:01Z

What problem does this PR solve?

Issue Number: close #54684 #54705

Problem Summary:

What changed and how does it work?

1. When `tidb_low_resolution_tso` is enabled, the in-transction `select for update` should be rejected.
2. When `for update` is used outside transactions, the lock operator should be skipped as there is 
no pessimsitic lock phase.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix the execution behaviours when tidb_low_resolution_tso is enabled and for update is used outside transactions

tiprow · 2024-07-17T13:14:28Z

Hi @cfzjywxk. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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-sigs/prow repository.

codecov · 2024-07-17T13:52:33Z

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.0474%. Comparing base (26378cb) to head (3c174c8).

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54694         +/-   ##
=================================================
- Coverage   72.6796%   56.0474%   -16.6323%     
=================================================
  Files          1551       1667        +116     
  Lines        437176     604993     +167817     
=================================================
+ Hits         317738     339083      +21345     
- Misses        99819     242723     +142904     
- Partials      19619      23187       +3568

Flag	Coverage Δ
integration	`36.5774% <52.9411%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.5434% <ø> (+6.7030%)`	⬆️

…tions Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>

Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>

cfzjywxk · 2024-07-18T04:08:52Z

/retest

tiprow · 2024-07-18T04:09:13Z

@cfzjywxk: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

pkg/sessionctx/variable/session.go

Co-authored-by: ekexium <eke@fastmail.com>

Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>

ti-chi-bot · 2024-07-18T05:24:50Z

@cfzjywxk: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/unit-test	`3c174c8`	link	true	`/test unit-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

ti-chi-bot · 2024-07-18T06:04:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ekexium, zyguan
Once this PR has been reviewed and has the lgtm label, please assign elsa0520, yudongusa for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-07-18T06:04:47Z

[LGTM Timeline notifier]

Timeline:

2024-07-18 04:37:47.416629532 +0000 UTC m=+501489.407571002: ☑️ agreed by ekexium.
2024-07-18 06:04:46.792658326 +0000 UTC m=+506708.783599797: ☑️ agreed by zyguan.

winoros · 2024-07-18T06:13:23Z

pkg/sessionctx/variable/session.go

@@ -1091,6 +1091,7 @@ type SessionVars struct {
 	TxnMode string

 	// LowResolutionTSO is used for reading data with low resolution TSO which is updated once every two seconds.
+	// Do not use it directly, use the `UseLowResolutionTSO` method below.


can we unexport it?

MyonKeminta · 2024-07-18T06:06:57Z

pkg/executor/adapter.go

+		if sctx.GetSessionVars().UseLowResolutionTSO() {
+			return nil, errors.New("can not execute select for update statement when 'tidb_low_resolution_tso' is set")
+		}


When tidb_low_resolution_tso is enabled, the in-transction select for update should be rejected.

Is here sure to be in transaction?

MyonKeminta · 2024-07-18T06:18:49Z

pkg/planner/core/logical_plan_builder.go

-		if err != nil {
-			return nil, err
+		lock, _ := getLockWaitTime(b.ctx, l)
+		if lock {


If !lock, is the above for loop still necessary?

cfzjywxk · 2024-07-18T06:57:51Z

It's complex to handle the plan with lock operator or executor in the same PR, would split them into differnt PRs.

cfzjywxk added type/bugfix This PR fixes a bug. sig/planner SIG: Planner sig/transaction SIG:Transaction labels Jul 17, 2024

cfzjywxk requested review from zyguan, qw4990, MyonKeminta, crazycs520 and ekexium July 17, 2024 13:14

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 17, 2024

cfzjywxk added 3 commits July 18, 2024 11:43

fix the lock behaviour when select for update is used outside transac…

6165674

…tions Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>

fix planner test

56f0e8a

Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>

fix planner test

d1f7dda

Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>

cfzjywxk force-pushed the low_resolution_tso branch from 3edb7bb to d1f7dda Compare July 18, 2024 03:43

cfzjywxk changed the title ~~txn: fix the lock behaviours when for update is used outside transactions~~ txn: fix the lock behaviours when for update is used outside transactions | tidb-test=pr/2365 Jul 18, 2024

pingcap deleted a comment from tiprow bot Jul 18, 2024

ekexium approved these changes Jul 18, 2024

View reviewed changes

pkg/sessionctx/variable/session.go Outdated Show resolved Hide resolved

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 18, 2024

cfzjywxk and others added 2 commits July 18, 2024 12:42

Update pkg/sessionctx/variable/session.go

96524da

Co-authored-by: ekexium <eke@fastmail.com>

refactor

3c174c8

Signed-off-by: cfzjywxk <cfzjywxk@gmail.com>

zyguan approved these changes Jul 18, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 18, 2024

winoros reviewed Jul 18, 2024

View reviewed changes

MyonKeminta reviewed Jul 18, 2024

View reviewed changes

cfzjywxk closed this Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: fix the lock behaviours when for update is used outside transactions | tidb-test=pr/2365 #54694

txn: fix the lock behaviours when for update is used outside transactions | tidb-test=pr/2365 #54694

cfzjywxk commented Jul 17, 2024 •

edited

Loading

tiprow bot commented Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

cfzjywxk commented Jul 18, 2024

tiprow bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

winoros Jul 18, 2024

MyonKeminta Jul 18, 2024

MyonKeminta Jul 18, 2024

cfzjywxk commented Jul 18, 2024

txn: fix the lock behaviours when for update is used outside transactions | tidb-test=pr/2365 #54694

txn: fix the lock behaviours when for update is used outside transactions | tidb-test=pr/2365 #54694

Conversation

cfzjywxk commented Jul 17, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 17, 2024

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

cfzjywxk commented Jul 18, 2024

tiprow bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

[LGTM Timeline notifier]

winoros Jul 18, 2024

Choose a reason for hiding this comment

MyonKeminta Jul 18, 2024

Choose a reason for hiding this comment

MyonKeminta Jul 18, 2024

Choose a reason for hiding this comment

cfzjywxk commented Jul 18, 2024

cfzjywxk commented Jul 17, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading