Skip to content

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

Merged
ti-srebot merged 15 commits intomasterfrom
unknown repository
Sep 18, 2020
Merged

planner: disable SQL_CALC_FOUND_ROWS/LOCK IN SHARE MODE by default#19506
ti-srebot merged 15 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@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 hanfei1991, wshwsh12 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

ghost commented Sep 2, 2020

/run-all-tests

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 2, 2020

/run-unit-test

Copy link
Copy Markdown
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
Copy Markdown
Author

ghost commented Sep 15, 2020

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

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

1 similar comment
@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

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

1 similar comment
@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

/run-unit-test

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 15, 2020

Please fix the test case.

Thanks! Should be fixed now.

@jackysp
Copy link
Copy Markdown
Contributor

jackysp commented Sep 18, 2020

/merge

@ti-srebot
Copy link
Copy Markdown
Contributor

/run-all-tests

@ti-srebot
Copy link
Copy Markdown
Contributor

@nullnotnil merge failed.

@jackysp
Copy link
Copy Markdown
Contributor

jackysp commented Sep 18, 2020

/merge

@ti-srebot
Copy link
Copy Markdown
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
Copy Markdown
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