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.

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

planner: disable SQL_CALC_FOUND_ROWS/LOCK IN SHARE MODE by default #19506

Merged
merged 15 commits into from Sep 18, 2020
Merged

planner: disable SQL_CALC_FOUND_ROWS/LOCK IN SHARE MODE by default #19506

merged 15 commits into from Sep 18, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 26, 2020

What problem does this PR solve?

Issue Number: Fixes #14448 but leaves #8235 open to (eventually) add the functionality

Also fixes #19383

Problem Summary:

Unless you are reading every page of the TiDB manual, you may not notice that a query using SQL_CALC_FOUND_ROWS will return wrong results without a warning or error. This is unsafe, as described why in #14448.

Similarly, SELECT LOCK IN SHARE MODE is also described in the manual as a noop. But that's not obvious to new users.

This protects against incorrect usage via the enable_noop_functions flag; meaning that applications that want to execute the query and receive broken results have a way out.

What is changed and how it works?

What's Changed:

Queries using SQL_CALC_FOUND_ROWS or LOCK IN SHARE MODE now get an error by default.

Related changes

Docs PR pingcap/docs#3898

Check List

Tests

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

Side effects

  • Breaking backward compatibility (although there is an easy way back by changing the enable noop functions setting).

Release note

  • TiDB now provides an error for statements that use the syntax SQL_CALC_FOUND_ROWS or LOCK IN SHARE MODE. This helps alert users that this feature is not implemented by TiDB, but a workaround to restore the previous behavior is available by setting tidb_enable_noop_functions=1.

@ghost ghost requested review from a team as code owners August 26, 2020 22:39
@ghost ghost requested review from wshwsh12, hanfei1991 and zz-jason and removed request for a team August 26, 2020 22:39
@ghost ghost added the sig/planner SIG: Planner label Aug 26, 2020
@ghost
Copy link
Author

ghost commented Aug 26, 2020

I noticed after I wrote this, that I could also implement this in planner/core/preprocessor.go. Would this be the preference?

@ghost ghost mentioned this pull request Aug 26, 2020
@ghost ghost changed the title planner: disable SQL_CALC_FOUND_ROWS by default planner: disable SQL_CALC_FOUND_ROWS/LOCK IN SHARE MODE by default Aug 30, 2020
@ghost
Copy link
Author

ghost commented Aug 30, 2020

I have updated this PR to fix LOCK IN SHARE MODE at the same time.

@ghost ghost requested a review from cfzjywxk September 1, 2020 02:18
@ghost
Copy link
Author

ghost commented Sep 2, 2020

/run-all-tests

@ghost
Copy link
Author

ghost commented Sep 2, 2020

/run-unit-test

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-all-tests tidb-test=pr/1087

@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-all-tests tidb-test=pr/1087

@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-integration-ddl-test tidb-test=pr/1087

1 similar comment
@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-integration-ddl-test tidb-test=pr/1087

@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-unit-test tidb-test=pr/1087

1 similar comment
@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-unit-test tidb-test=pr/1087

@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-integration-ddl-test tidb-test=pr/1087

@ghost
Copy link
Author

ghost commented Sep 15, 2020

/run-unit-test

@ghost
Copy link
Author

ghost commented Sep 15, 2020

Please fix the test case.

Thanks! Should be fixed now.

@jackysp
Copy link
Member

jackysp commented Sep 18, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@nullnotnil merge failed.

@jackysp
Copy link
Member

jackysp commented Sep 18, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 9876de8 into pingcap:master Sep 18, 2020
@ghost ghost deleted the sql-calc-found-rows branch September 21, 2020 17:42
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 12, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #21005

ti-srebot added a commit that referenced this pull request Nov 17, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SELECT LOCK IN SHARE MODE is unsafe SQL_CALC_FOUND_ROWS behavior leads to data truncation
5 participants