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

add skip isolation level check variable #10065

Merged
merged 13 commits into from Apr 11, 2019

Conversation

@marsishandsome
Copy link
Contributor

marsishandsome commented Apr 8, 2019

What problem does this PR solve?

This patch (https://github.com/pingcap/tidb/pull/8625/files) breaks backward compatible.
When using tidb as hive's metastore, hive will set transaction isolation level to serializable, which will cause the following error:

ERROR 1105 (HY000): variable 'tx_isolation' does not yet support value: SERIALIZABLE

What is changed and how it works?

This PR provide a variable tidb_skip_isolation_level_check.
If tidb_skip_isolation_level_check == ON, tidb will not throw an error if hive call set session transaction isolation level serializable;. tidb_skip_isolation_level_check's default value is OFF.

Check List

Tests

  • Unit test

Related changes

  • Need to be included in the release note
  • Need to cherry-pick to the release branch
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Apr 8, 2019

CLA assistant check
All committers have signed the CLA.

@marsishandsome marsishandsome force-pushed the marsishandsome:feature/strict_compatibility branch 4 times, most recently from c2e06f3 to 1a101fd Apr 8, 2019

@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 8, 2019

/run-all-tests

Show resolved Hide resolved sessionctx/variable/session.go Outdated
Show resolved Hide resolved sessionctx/variable/session.go Outdated
@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 8, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@ee99570). Click here to learn what that means.
The diff coverage is 35.7142%.

@@             Coverage Diff             @@
##             master     #10065   +/-   ##
===========================================
  Coverage          ?   78.1129%           
===========================================
  Files             ?        405           
  Lines             ?      82044           
  Branches          ?          0           
===========================================
  Hits              ?      64087           
  Misses            ?      13254           
  Partials          ?       4703
@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 8, 2019

/run-all-tests

@marsishandsome marsishandsome force-pushed the marsishandsome:feature/strict_compatibility branch 4 times, most recently from c594105 to 6740581 Apr 8, 2019

@zhexuany zhexuany removed the status/WIP label Apr 8, 2019

@marsishandsome marsishandsome force-pushed the marsishandsome:feature/strict_compatibility branch from 6740581 to aa16017 Apr 8, 2019

@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 8, 2019

/run-all-tests

1 similar comment
@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 8, 2019

/run-all-tests

@marsishandsome marsishandsome force-pushed the marsishandsome:feature/strict_compatibility branch from aa16017 to 9be5a55 Apr 8, 2019

@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 8, 2019

@jackysp @zhexuany please review

@morgo

This comment has been minimized.

Copy link
Member

morgo commented Apr 8, 2019

@marsishandsome thank you for looking into this. I would like to suggest that we don't call it strict, since in other MySQL contexts strict means "more safe". Here it means "less safe" because an error is downgraded.

The MySQL-like name for this would be "ignore" or "skip". I would also prefer to have a purposeful name here, because it is a very specific feature being disabled which is unsafe (versus a generic name, which may be better if they are just general changes).

So perhaps skip-isolation-level-checks?

PTAL @kolbe

@zhexuany

This comment has been minimized.

Copy link
Member

zhexuany commented Apr 8, 2019

@morgo Agreed. I guess the intention of tidb_strict_compatibility is that when it on throws an error on unsupported isolation level. skip_isolation_level_check maybe a better name. /cc @marsishandsome WDUT?

@morgo

This comment has been minimized.

Copy link
Member

morgo commented Apr 10, 2019

if tidb_skip_isolation_level_check=1 and user set the isolation level to 'SERIALIZABLE', what should tidb do? throw a warning? what is the warning message?

I think it is fine to keep the same error message but degrade to a warning. The error message does says to 'skip this error', which is true - it is skipping the error.

@marsishandsome marsishandsome force-pushed the marsishandsome:feature/strict_compatibility branch from 514da01 to 470b0dd Apr 11, 2019

@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 11, 2019

tk.MustExec("SET SESSION TRANSACTION ISOLATION LEVEL SERIALIZABLE")
tk.MustQuery("show warnings").Check(testkit.Rows(
"Warning 1105 The isolation level 'SERIALIZABLE' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error",
"Warning 1105 The isolation level 'SERIALIZABLE' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error"))

This comment has been minimized.

Copy link
@morgo

morgo Apr 11, 2019

Member

I would like to see just one warning here.

This comment has been minimized.

Copy link
@marsishandsome

marsishandsome Apr 11, 2019

Author Contributor

@zhexuany @jackysp any suggestions about how to elegantly deduplicate two same warnings?

This comment has been minimized.

Copy link
@jackysp

jackysp Apr 11, 2019

Member

I think we should debug it, why the warning was appended twice.

This comment has been minimized.

Copy link
@marsishandsome

marsishandsome Apr 11, 2019

Author Contributor

SET TRANSACTION ISOLATION LEVEL will affect two internal variables:

  1. tx_isolation
  2. transaction_isolation

So the function ValidateSetSystemVar is called twice.

This comment has been minimized.

Copy link
@morgo

morgo Apr 11, 2019

Member

Because the error message does not mention which variable was set now, it is easy to fix by wrapping an if around transaction_isolation.

This comment has been minimized.

Copy link
@marsishandsome

marsishandsome Apr 11, 2019

Author Contributor

@morgo @jackysp PTAL again

tk.MustExec("SET GLOBAL TRANSACTION ISOLATION LEVEL READ UNCOMMITTED")
tk.MustQuery("show warnings").Check(testkit.Rows(
"Warning 1105 The isolation level 'READ-UNCOMMITTED' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error",
"Warning 1105 The isolation level 'READ-UNCOMMITTED' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error"))

This comment has been minimized.

Copy link
@morgo

morgo Apr 11, 2019

Member

And here too

@morgo
Copy link
Member

morgo left a comment

LGTM

@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 11, 2019

/run-all-tests

2 similar comments
@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 11, 2019

/run-all-tests

@marsishandsome

This comment has been minimized.

Copy link
Contributor Author

marsishandsome commented Apr 11, 2019

/run-all-tests

@jackysp jackysp merged commit 28e6294 into pingcap:master Apr 11, 2019

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/code_coverage Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.