Skip to content

session: add some compatibility checks for non-transactional DML#34127

Merged
ti-chi-bot merged 10 commits intopingcap:masterfrom
ekexium:nt-ignore-select-limit
Apr 25, 2022
Merged

session: add some compatibility checks for non-transactional DML#34127
ti-chi-bot merged 10 commits intopingcap:masterfrom
ekexium:nt-ignore-select-limit

Conversation

@ekexium
Copy link
Member

@ekexium ekexium commented Apr 20, 2022 •
edited
Loading

Signed-off-by: ekexium ekexium@gmail.com

What problem does this PR solve?

Issue Number: ref #33485

Problem Summary:

A Non-transactional DML is not a SELECT. sql_select_limit can limit the select query that is used to generate shard jobs and can lead to unexpected results for a "DML". So it should be ignored when executing the query.

What is changed and how it works?

  • Ignores sql_select_limit and tidb_read_staleness.
  • Returns an error when tidb_snapshot, batch-dml, or weak read consistency is used.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Apr 20, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • cfzjywxk
  • you06

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.

Details

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 release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 20, 2022
@ekexium ekexium requested review from cfzjywxk and you06 April 20, 2022 06:35
@ekexium ekexium force-pushed the nt-ignore-select-limit branch from 9f14213 to 008bd7a Compare April 20, 2022 06:37
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Apr 20, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/19f7b391305fc753d5a62d44b7cdd63a894a4b64

@cfzjywxk
Copy link
Contributor

cfzjywxk commented Apr 20, 2022

Would there be some other session variables which should be skipped too?

@ekexium
Copy link
Member Author

ekexium commented Apr 20, 2022 •
edited
Loading

Would there be some other session variables which should be skipped too?

Maybe tidb_read_staleness. If we consider NT-DML as an atomic write operation, then it should not be affected by the variable that is supposed to only affect SELECTs.

I didn't find other session variables that should be ignored. But there are some that I think are worth consideration:

  • tidb_read_consistency, I suppose we just don't allow NT-DML if it is set to weak. I would be hard to understand its meaning
  • The deprecated enable-batch-dml configuration. I prefer to forbid NT-DML when batch-dml is used.
  • tidb_snapshot. If it's set, an error should be returned like other DML.

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 20, 2022
@ekexium ekexium changed the title session: ignore sql_select_limit for non-transactional DML session: add some compatibility checks for non-transactional DML Apr 20, 2022
ekexium added 6 commits April 20, 2022 20:04
@ekexium
ignore select limit
5589c6d 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
ignore tidb_read_staleness
1141d4e 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
return error when tidb_snapshot is set
a3bacdb 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
return error when tidb_read_consistency is weak
acb2f67 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
return error when batch_dml is used
60ecfb1 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
fix test
f3cf670 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium ekexium force-pushed the nt-ignore-select-limit branch from 8ae652b to f3cf670 Compare April 20, 2022 12:05
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2022
you06
you06 reviewed Apr 22, 2022
View reviewed changes
ekexium added 2 commits April 22, 2022 13:49
@ekexium
move autocommit check
ed5b901 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
add a test of optimizer hints
4a7d681 
Signed-off-by: ekexium <ekexium@gmail.com>
@ekexium
Copy link
Member Author

ekexium commented Apr 24, 2022

PTAL @you06 @cfzjywxk

cfzjywxk
cfzjywxk reviewed Apr 24, 2022
View reviewed changes
@ekexium
test checkConstraint
6f052af 
Signed-off-by: ekexium <ekexium@gmail.com>
cfzjywxk
cfzjywxk approved these changes Apr 24, 2022
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 ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 24, 2022
@ekexium
Copy link
Member Author

ekexium commented Apr 24, 2022

/run-unit-test

@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 Apr 25, 2022
@cfzjywxk
Copy link
Contributor

cfzjywxk commented Apr 25, 2022

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Apr 25, 2022

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

DetailsCommit hash: 6f052af

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 25, 2022
@ti-chi-bot ti-chi-bot merged commit 65f329e into pingcap:master Apr 25, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Apr 25, 2022

TiDB MergeCI notify

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test 🟢 all 11 tests passed 12 min Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 8 min 47 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 7 min 10 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 6 min 37 sec Existing passed
idc-jenkins-ci-tidb/common-test 🟢 all 12 tests passed 6 min 0 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 5 min 40 sec Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 5 min 20 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 4 min 14 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

espresso98 pushed a commit to espresso98/tidb that referenced this pull request Apr 25, 2022
@ekexium @espresso98
session: add some compatibility checks for non-transactional DML (pin…
bbceabf 
…gcap#34127)

ref pingcap#33485
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

@you06 you06 you06 approved these changes

Assignees

No one assigned

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ekexium @ti-chi-bot @sre-bot @cfzjywxk @you06